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

Łukasz Wojniłowicz lukasz.wojnilowicz at gmail.com
Sun Apr 17 17:32:08 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.
> 
> Łukasz Wojniłowicz wrote:
>     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)?
> 
> Cristian Oneț wrote:
>     I think that this patch should focus on clening up CSVDialog::createProfile. Here are a few hints:
>     - use only one [KSharedConfig::openConfig](http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKSharedConfig.html#a1e98b058e4f3996a3cc69fa4cb4a440a) because that already merges local and global configurations so the template should already be present in that object 
>     - remove unused variables
>     - maybe rename 'Profiles-New Profile###' into something more suggestive like 'Profiles-NewProfileTemplate' (think about how this change would affect a user updating the application)
>     
>     About the patch: I don't see why BuyParam should be read in a different way than ShrsinParam so it looks good to me.
> 
> Allan Anderson wrote:
>     Hi ?ukasz, unfortunately I'm going to have to leave you to continue with this.
>     I have no idea at this late stage why some of the share categories are handled differently.  It does not appear to impact performance with the untranslated version, but obviously, for you, things are not right.  I still do not understand why some of your transaction types are blank in your config file.  However, if the patch resolves your issue, then I'm happy with it.
>     Are you able also to pick up the points made by Cristian One?, pease.  Sorry to leave you in the lurch.
>     Allan

@Christian
I made an attempt to sort out `CSVDialog::createProfile`.
Pleas see attachment
https://bugsfiles.kde.org/attachment.cgi?id=98417
to bug #360129.
To answer your suggestions:
ad 1. 
Now only one `KSharedConfig::openConfig` is used

ad 2.
`KConfigGroup profilesGroup` has been removed as unused variable

ad 3.
I stopped relaying on 'Profiles-New Profile###' because I don't see how to facilitate translation of this template. To me this template profile is hardcoded English profile; see my second thought from my previous post.
Instead I moved whole 'Profiles-New Profile###' to KMM code which facilitates translation

What's your opinion on that patch?

As for the issue...
>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.

...you've opened. I think you may be right but that means more untested work will go into KMM. With current code flow it may be not perfect but it works somehow, so I think: let's leave code flow as is and have your suggestion in mind when we would like to rewrite the code.

@Allan
No problem Allan, you've already explained your situation to me and I understand it.
I hope you'll be able to find some time to chip in here or in bugzilla to warn me if my understanding of proposed patches will be questionable. I would welcome that.


- Ł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/20160417/be803db4/attachment-0001.html>


More information about the KMyMoney-devel mailing list