Review Request 127559: BUG 360129 Do not fetch from csvimporterrc if it's empty

Łukasz Wojniłowicz lukasz.wojnilowicz at gmail.com
Sat Apr 9 13:01:18 UTC 2016



> On April 7, 2016, 7:59 p.m., Christian David wrote:
> > kmymoney/plugins/csvimport/investprocessing.cpp, line 1967
> > <https://git.reviewboard.kde.org/r/127559/diff/1/?file=455181#file455181line1967>
> >
> >     Should become
> >     ```m_shrsinList = profilesGroup.readEntry("ShrsinParam", m_shrsinList);```
> >     
> >     The if() is very long and not needed here. However, I still do not know if this is the issue. Also the ```i18nc()s``` from ```init()``` could go here if the readSettings method is always called, which I do not know either.
> 
> Allan Anderson wrote:
>     > Should become
>     > m_shrsinList = profilesGroup.readEntry("ShrsinParam", m_shrsinList);
>     
>     I'm not sure I understand this.  The second parameter is the default value to return if the key is not found.  What does it achieve in this case?
>     
>     > The if() is very long and not needed here.
>     
>     There are several ifs around here, but I don't see an unduly long one.
>     
>     > Also the i18nc()s from init() 
>     > could go here if the readSettings method is always called, which I do not know either.
>     
>     readSettings is called only once, from void InvestProcessing::slotFileDialogClicked(), so that code could be moved somewhere in void InvestProcessing::readSettings(), I think.
> 
> Łukasz Wojniłowicz wrote:
>     > Should become 
>     > m_shrsinList = profilesGroup.readEntry("ShrsinParam", m_shrsinList);
>     
>     I compiled KMyMoney code according to your change for every m_XXXList variable and ran my test case. Proposed line looks neat but doesn't work for me. SellParam= etc. are empty in my csvimporterrc after just created new profile.
>      
>     >readSettings is called only once, from void InvestProcessing::slotFileDialogClicked(), so that code could be moved somewhere in void InvestProcessing::readSettings(), I think.
>     
>     Please give a code and I'll test it.
>     
>     > I do not know the full conversation but I am pretty sure this patch will not solve the issue. If something in the newly created rc file is missing, the write method seems to fail, not the read method.
>     
>     My loose observation: Write method is called at the end of importing and read method is called after creating new importing profile for investment.
> 
> Christian David wrote:
>     > I do not know the full conversation but I am pretty sure this patch will not solve the issue. If something in the newly created rc file is missing, the write method seems to fail, not the read method.
>     
>     I withdrew this idea. You should not waste your time with it.
>     
>     > I compiled KMyMoney code according to your change for every m_XXXList variable and ran my test case. Proposed line looks neat but doesn't work for me. SellParam= etc. are empty in my csvimporterrc after just created new profile.
>     
>     You are right. The code recomended by me has a different behaviour. However, now I doubt that anything I wrote was actually helpfull. I just briefly inspected the code – now I see that is more complex than I thougt. So my recomendations are based on insufficent knowledge. Due to the description in the bug report I still think there is a high chance that the issue is in the write function.

Due to the description in the bug report I still think there is a high chance that the issue is in the write function.


And you may be right, look what I've found.
New entry of importing profile in `$HOME/.kde/share/config/csvimporterrc` is created in csvdialog.cpp by following routine

```c++
void CSVDialog::createProfile(QString newName)
{
  KSharedConfigPtr  config = KSharedConfig::openConfig(KStandardDirs::locateLocal("config", "csvimporterrc"));
  KConfigGroup bankProfilesGroup(config, "BankProfiles");

  bankProfilesGroup.writeEntry("BankNames", m_profileList);
  bankProfilesGroup.config()->sync();

  KConfigGroup bankGroup(config, "BankProfiles");
  QString txt = "Profiles-" + newName;

  KConfigGroup profilesGroup(config, "Profiles-New Profile###");

  KSharedConfigPtr  configBackup = KSharedConfig::openConfig(KStandardDirs::locate("config", "csvimporterrc"));
  KConfigGroup bkprofilesGroup(configBackup, "Profiles-New Profile###");

  KConfigGroup newProfilesGroup(config, txt);
  bkprofilesGroup.copyTo(&newProfilesGroup);
  newProfilesGroup.writeEntry("FileType", m_fileType);
  if (m_fileType == "Invest") {
    m_investProcessing->m_shrsinList = bkprofilesGroup.readEntry("ShrsinParam", QStringList());
    newProfilesGroup.writeEntry("ShrsinParam", m_investProcessing->m_shrsinList);
    m_investProcessing->m_divXList = bkprofilesGroup.readEntry("DivXParam", QStringList());
    newProfilesGroup.writeEntry("DivXParam", m_investProcessing->m_divXList);
    m_investProcessing->m_intIncList = bkprofilesGroup.readEntry("IntIncParam", QStringList());
    newProfilesGroup.writeEntry("IntIncParam", m_investProcessing->m_intIncList);
    m_investProcessing->m_brokerageList = bkprofilesGroup.readEntry("BrokerageParam", QStringList());
    newProfilesGroup.writeEntry("BrokerageParam", m_investProcessing->m_brokerageList);
    m_investProcessing->m_reinvdivList = bkprofilesGroup.readEntry("ReinvdivParam", QStringList());
    newProfilesGroup.writeEntry("ReinvdivParam", m_investProcessing->m_reinvdivList);
    m_investProcessing->m_buyList = bkprofilesGroup.readEntry("BuyParam", QStringList());
    newProfilesGroup.writeEntry("BuyParam", m_investProcessing->m_buyList);
    m_investProcessing->m_sellList = bkprofilesGroup.readEntry("SellParam", QStringList());
    newProfilesGroup.writeEntry("SellParam", m_investProcessing->m_sellList);
    m_investProcessing->m_removeList = bkprofilesGroup.readEntry("RemoveParam", QStringList());
    newProfilesGroup.writeEntry("RemoveParam", m_investProcessing->m_removeList);
  }
  newProfilesGroup.config()->sync();
}
```

According to above routine, new entries in 
`$HOME/.kde/share/config/csvimporterrc`
supposedly should be created from template entry (Profiles-New Profile###) in
`$USR/share/config/csvimporterrc` (read-only-file).
Moreover all m_XXXList are defined here also and supposedly shouldn't be empty because template entry isn't empty.

On my system (Fedora 23, KDE Frameworks 5.21.0, Qt 5.5.1) it doesn't work though.
Snippets of code:

`KStandardDirs::locateLocal("config", "csvimporterrc")`
responsible for locating
`$HOME/.kde/share/config/csvimporterrc`

and
`KStandardDirs::locate("config", "csvimporterrc")`
responsible for locating
`$USR/share/config/csvimporterrc`

both point to the same file and that is
`$HOME/.kde/share/config/csvimporterrc`

Config file
`$HOME/.kde/share/config/csvimporterrc`
hasn't got "Profiles-New Profile###" so empty values are defined in m_XXXList and in new entry.


Do `KStandardDirs::locate("config", "csvimporterrc")` works correctly on my system? I think, yes.

According to http://api.kde.org `KStandardDirs::locate` is convenience function for `KGlobal::dirs()->findResource(type, filename)` where in our case, type is "config" and filename is "csvimporterrc". That function will


>    The filename should be a filename relative to the base dir for resources. So is a way to get the path to csvimporterrc to findResource("config", "csvimporterrc"). KStandardDirs will then look into the subdir config of all elements of all prefixes ($KDEDIRS) for a file csvimporterrc and return the path to the first one it finds (e.g. $HOME/.kde/share/config/csvimporterrc). You can use the program kde4-config to list all resource types.


My `kde4-config --path config` gives me
`$HOME/.kde/share/config/:/etc/kde/:$USR/share/kde-settings/kde-profile/default/share/config/:$USR/share/config/`

Notice that `$HOME/.kde/share/config/` is before `$USR/share/config/` so it will be quered first by `KStandardDirs::locate` and according to http://api.kde.org `KStandardDirs::locate` will return full path to the first one file it finds and in case of "csvimporterrc" it always will be `$HOME/.kde/share/config/csvimporterrc`

My thoughts:
1) `KStandardDirs` is deprecated and should be replaced with `QStandardPaths` but I don't think it can be done now since I suppose that KMyMoney is compiled against < Qt 5 now.
2) csvimporterrc in `$USR/share/config/` could have unique name, so `KStandardDirs` won't confuse it with the one in local dir but I don't think it's the best idea because "Profiles-New Profile###" is preset with all English types of operations and that won't work for non-English users
3) I still do believe that my original path is good here, because: it won't harm not to fetch empty values from csvimporterrc into KMyMoney, it's reported to work in non-English environment, it shouldn't break usage in English environment

What are your opinions?

@Allan
You've said that your csvimporterrc is initiated with all required values, oppositely to mine; what is your system configuration (OS, KDE, Qt)?


- Łukasz


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127559/#review94397
-----------------------------------------------------------


On April 3, 2016, 4:45 a.m., Łukasz Wojniłowicz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127559/
> -----------------------------------------------------------
> 
> (Updated April 3, 2016, 4:45 a.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Bugs: 360129
>     http://bugs.kde.org/show_bug.cgi?id=360129
> 
> 
> Repository: kmymoney
> 
> 
> Description
> -------
> 
> Fixes bug #360129. During creation of new investment statement
> template, transaction types are initialized in
> investprocessing.cpp, but then are overridden with empty fields
> from profile that was just created in csvimporterrc which results
> in every non-buy transaction unrecognized during the import.
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/csvimport/investprocessing.cpp 3879819 
> 
> Diff: https://git.reviewboard.kde.org/r/127559/diff/
> 
> 
> Testing
> -------
> 
> Tested using financial statement from bug #360129.
> 
> 
> Thanks,
> 
> Łukasz Wojniłowicz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20160409/0479cdb6/attachment-0001.html>


More information about the KMyMoney-devel mailing list