> 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 > > Łukasz Wojniłowicz wrote: > @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.
> @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. I'll do my best. - Allan ----------------------------------------------------------- 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 > >
