Avoid loading duplicate .desktop files in menubar
Currently, if a user copies a .desktop file into some other directory, they will get duplicate entries in menubar. This PR changes that so that only one entry is kept.
The Desktop Entry Specification says that entries have an id derived from their path.
To determine the ID of a desktop file, make its full path relative to the $XDG_DATA_DIRS component in which the desktop file is installed, remove the "applications/" prefix, and turn '/' into '-'.
Only the first file found with a given id, following the order of directories in $XDG_DATA_DIRS, should be used. This PR changes the logic used by menubar to do exactly this. This allows users to e.g. override a system defined .desktop file by putting a changed one in ~/.local/share/applications/. Changing '/' into '-' allows overriding a file stored inside a directory hierarchy of the system without creating directories in .local.
To do this, the GIO Async context was hoisted above the iteration through directories as otherwise following directory precedence would be more complex.
There is one divergence from the spec in this implementation. The spec says:
If the desktop file is not installed in an applications subdirectory of one of the $XDG_DATA_DIRS components, it does not have an ID.
In this implementation, we scan through menu_bar.all_menu_dirs, and all these directories are treated the same, regardless of whether they are "an applications subdirectory of one of the $XDG_DATA_DIRS components". Some users set this, though it's pretty uncommon. I think this is unlikely to cause the few people who use this to add more directories any problems with accidentally-ignored .desktop files.
Fixes #1816.
Codecov Report
Merging #3822 (d87bb3e) into master (b13ac3e) will decrease coverage by
0.03%. The diff coverage is67.16%.
:exclamation: Current head d87bb3e differs from pull request most recent head 99db3a8. Consider uploading reports for the commit 99db3a8 to get more accurate results
@@ Coverage Diff @@
## master #3822 +/- ##
==========================================
- Coverage 90.99% 90.97% -0.03%
==========================================
Files 900 901 +1
Lines 57506 57535 +29
==========================================
+ Hits 52329 52340 +11
- Misses 5177 5195 +18
| Flag | Coverage Δ | |
|---|---|---|
| gcov | 90.97% <67.16%> (-0.03%) |
:arrow_down: |
| luacov | 93.65% <67.16%> (-0.06%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| lib/menubar/menu_gen.lua | 73.41% <43.75%> (-26.59%) |
:arrow_down: |
| lib/menubar/utils.lua | 88.11% <55.55%> (-3.11%) |
:arrow_down: |
| lib/awful/client/focus.lua | 92.04% <100.00%> (+0.18%) |
:arrow_up: |
| tests/test-focus-bydirection.lua | 100.00% <100.00%> (ø) |
I started working on some unit tests for this, but I gave up due to the lack of support for async() in busted 2.x. https://github.com/dyfrgi/awesome/compare/desktop-file-id...dyfrgi:awesome:menu_gen-unit-test?expand=1 shows the starting point, though. With busted 2.0, we'd need a custom test runner. I don't quite have it in me to dig into lgi + gio async work and busted custom test runners for this PR, so I hope that manual testing is sufficient.
It would probably be possible to refactor things so as to make this easier to test without handling async in the test runner, but it's not obvious to me.
there are several tests which have accommodation for that (via re-running test function few times), and using count argument as the counter of how many times test function have been attempted to execute, ex:
https://github.com/awesomeWM/awesome/blob/master/tests/test-client-shape.lua#L8
@dyfrgi Do you still plan to work on the test? As @actionless said, this is better done using an integration test suite since it has a full event loop and retry function if they return nil.
Whoops, I never actually sent the reply I drafted. Yes, I plan to work on it. Have done some work on it a couple weeks back but only got it to 95%. I'll post something for review soon.