Review Request 127559: BUG 360129 Do not fetch from csvimporterrc if it's empty
aga
agander93 at gmail.com
Sat Apr 9 13:40:21 UTC 2016
On 09/04/16 14:01, Łukasz Wojniłowicz wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127559/
>
>
> On April 7th, 2016, 7:59 p.m. UTC, *Christian David* wrote:
>
> kmymoney/plugins/csvimport/investprocessing.cpp
> <https://git.reviewboard.kde.org/r/127559/diff/1/?file=455181#file455181line1967>
> (Diff revision 1)
> 1967
>
> m_shrsinList = profilesGroup.readEntry("ShrsinParam", QStringList());
>
> 1967 Łukasz
>
> list = profilesGroup.readEntry("ShrsinParam", QStringList());
>
> 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.
>
> On April 8th, 2016, 1:52 p.m. UTC, *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.
>
> On April 8th, 2016, 4:36 p.m. UTC, *Ł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.
>
> On April 8th, 2016, 7:34 p.m. UTC, *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
>
>
> void CSVDialog::createProfile(QString newName)
> {
> KSharedConfigPtr config= KSharedConfig::openConfig(KStandardDirs::locateLocal("config","csvimporterrc"));
> KConfigGroupbankProfilesGroup(config,"BankProfiles");
>
> bankProfilesGroup.writeEntry("BankNames", m_profileList);
> bankProfilesGroup.config()->sync();
>
> KConfigGroupbankGroup(config,"BankProfiles");
> QString txt= "Profiles-" + newName;
>
> KConfigGroupprofilesGroup(config,"Profiles-New Profile###");
>
> KSharedConfigPtr configBackup= KSharedConfig::openConfig(KStandardDirs::locate("config","csvimporterrc"));
> KConfigGroupbkprofilesGroup(configBackup,"Profiles-New Profile###");
>
> KConfigGroupnewProfilesGroup(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
Hi Łukasz
I've only had the chance to glance at your posting, but was interested
to see - "On my system (Fedora 23, KDE Frameworks 5.21.0, Qt 5.5.1)".
That surprises me as my understanding is/was that KMM still requires
KDE4, and that porting to Frameworks is only partially completed.
It would be good to know what's what.
> @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
Until just a couple of days ago I was using qt4, but have now installed
a new distro which has both qt4 and qt5 installed. So far I've not
committed anything with it.
SMP PREEMPT Mon Oct 20 13:47:22 UTC 2014 (feb42ea) x86_64 x86_64 x86_64
GNU/Linux
KDE - Platform Version 4.14.9
I'm trying to get to the bottom of this action type problem, but am only
making very slow progress. There's just too much else going on here,
I'm afraid.
Allan
More information about the KMyMoney-devel
mailing list