Review Request 127559: BUG 360129 Do not fetch from csvimporterrc if it's empty
Cristian Oneț
onet.cristian at gmail.com
Sun Apr 17 08:23:55 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)?
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.
- Cristian
-----------------------------------------------------------
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/d54ca332/attachment-0001.html>
More information about the KMyMoney-devel
mailing list