Fix relativePath calculation in KDesktopFile::locateLocal()
ClosedPublic

Authored by wbauer on Apr 19 2017, 10:13 AM.

Details

Summary

The "dir" and "path" variables are obviously swapped here by mistake.
This results in the relativePath always being empty, and makes the function return "~/.local/share/" (or "~/.config/") instead of the correct path.

BUG: 345100

Test Plan

Open kmenuedit, edit an existing sub menu and change the icon, the change is correctly applied now, it is written to ~/.local/share/desktop-directories/xxx.directory instead of ~/.local/share/xxx.directory.

Also create a new sub menu and choose a different icon immediately. The correct icon does show up in the application menu now.

The added tests pass too, they fail with the original code.

Diff Detail

Repository
R237 KConfig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
wbauer created this revision.Apr 19 2017, 10:13 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 19 2017, 10:13 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
mdawson requested changes to this revision.Apr 19 2017, 2:28 PM
mdawson added a subscriber: mdawson.

+1 This definitely looks like the correct fix.

Can you please add some unit tests for this, to ensure it doesn't break in the future? I think just three extra tests, one for a desktop file in a config directory, one in a data directory, and one present elsewhere would be enough.

This revision now requires changes to proceed.Apr 19 2017, 2:28 PM

Can you please add some unit tests for this, to ensure it doesn't break in the future? I think just three extra tests, one for a desktop file in a config directory, one in a data directory, and one present elsewhere would be enough.

Yes, I'll try.
Though it will take till tomorrow I suppose...

Can you please add some unit tests for this, to ensure it doesn't break in the future? I think just three extra tests, one for a desktop file in a config directory, one in a data directory, and one present elsewhere would be enough.

Yes, I'll try.
Though it will take till tomorrow I suppose...

No problem, feel free to ask for help! The tests can go in autotests/kdesktopfile.cpp, which already has several tests for KDesktopFile. Take a look at the existing tests in that file for an example.

wbauer updated this revision to Diff 13693.Apr 22 2017, 10:58 AM
wbauer edited edge metadata.
wbauer edited the test plan for this revision. (Show Details)

added some tests

mdawson requested changes to this revision.Apr 23 2017, 5:08 AM

Excellent! Could you add 4 more rows to the tests, to ensure a path without a folder (ex. systemConfigLocation + "/test.desktop") works correctly? Once that's done, it's a ship it from me.

This revision now requires changes to proceed.Apr 23 2017, 5:08 AM
wbauer updated this revision to Diff 13778.Apr 25 2017, 11:22 AM
wbauer edited edge metadata.

added more test cases as suggested

mdawson accepted this revision.Apr 25 2017, 6:41 PM

LGTM! Thanks for working on this.

This revision is now accepted and ready to land.Apr 25 2017, 6:41 PM
This revision was automatically updated to reflect the committed changes.