kconf_update needs to launch update for kf5 application not for kde4 application.
Review Request #121797 - Created Jan. 3, 2015 and submitted
This patch is necessary because:
When we use kf5 + kde4 application, kconf_update which launchs at the start when we launch kde.
But it will migrate some config file, for example it will show that we need to migrate konversation
so it will create a konversationrc in .config/
But when we launch konversation there is a kdelibs4migrator which wants to migrate settings and config in .config
but it shows a konversationrc in .config so it will never migrate and we will lose all settings.
So we can force to remove all .upd in kf5 but it will not fix problem during migration or when we have kde4 application
install in same directory as kf5.
So now I force for each upd file to have a "Version=5" so kconf_update (kf5) will migrate just kf5 upd file and it will fix my bugs.
I updated unittest.
Unit test adapted for this change.
I suggested the solution so I'm OK with it :-)
Just a few things about the implementation.
prefer a row name that doesn't contain spaces or special chars, so that you can call this specific row on the command line when launching the test.
Update comment, now that you also prepend Version
this line is now complex and with redundancy.
Better use a local QString (e.g. "header"), put Version=5\n first (in an if), then add Id=%1, then prepend the header to updContent.
that boolean is much more generic than a version thing then, it's "should the update have worked or not".
=> rename it to shouldUpdateWork
update comment to mention Version too
The big question is, what should we do with Version=6 files.
Solution 1: with the current code, they will not be run. This means an update to kconf_update from KF6 will be needed for such .upd files to run
Solution 2: the code extract the version number and check >= 5. Then KF6 upd files will run.
But I'm not sure what we will want to happen by then....
So Version has to be before Id. Is there a documentation of the .upd file format somewhere, other than just the example you updated?
Ah, hmm, my suggestion didn't exactly work for this part of the code.
The test_data() column should have two columns, useVersion5 and shouldUpdateWork.
This leaves room for other cases in the future where the update should not be applied.
In fact I'm surprised there is no unittest for trying to match something that doesn't match the actual file. Such a test would have useVersion5=true and shouldUpdateWork=false.
This is why I think two bools would be better, even if for now they would have the same value in all the existing tests.
Separating input values from expected results is always a good idea.
For kf5 -> Starting from KDE Frameworks 5,
we need to -> make sure to
upd file -> the upd file
will not parse -> will be skipped
config file -> the config file
updated -> be updated
LGTM. Since this is a break in behaviour, I'd like to have a warning for developers if there update files are skipped (noted below). After that I'm happy for it to go in. Please add a changelog tag to the commit message to ensure it gets noted at the next Frameworks release.
I just got noticed about this change and I think this is a bad solution. This breaks existing behavior and I would even go so far to say that it breaks the stable API promise (yes I consider the udp format as API). The udp files shipped with Plasma 5.2 will stop working after an update to the next framework release. That's quite uncool.