awesome icon indicating copy to clipboard operation
awesome copied to clipboard

Avoid loading duplicate .desktop files in menubar

Open dyfrgi opened this issue 2 years ago • 5 comments

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.

dyfrgi avatar Jun 14 '23 20:06 dyfrgi

Codecov Report

Merging #3822 (d87bb3e) into master (b13ac3e) will decrease coverage by 0.03%. The diff coverage is 67.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%> (ø)

... and 3 files with indirect coverage changes

codecov[bot] avatar Jun 17 '23 11:06 codecov[bot]

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.

dyfrgi avatar Jun 25 '23 18:06 dyfrgi

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

actionless avatar Jun 26 '23 01:06 actionless

@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.

Elv13 avatar Aug 30 '23 11:08 Elv13

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.

dyfrgi avatar Sep 15 '23 12:09 dyfrgi