Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: implement full desktop spec #120

Merged
merged 10 commits into from
Apr 9, 2024
Merged

Conversation

friedow
Copy link
Owner

@friedow friedow commented Apr 1, 2024

This PR implements a bunch of fixes around the applications plugin. This aligns the applications plugin closer with the freedesktop desktop entry specification. Furhtermore it fixes #110.

This PR includes:

  • Hiding desktop entries based on the following logic:
    • Type has to be "Application"
    • Hidden has to be false or unset
    • NoDisplay has to be false or unset
    • the XDG_CURRENT_DESKTOP has to be in OnlyShowIn and not in NotShowIn
  • Terminal desktop entries are now opened in the first terminal emulator that is found on the system (fixes things like btop not being able to launch)

@colemickens
Copy link

(random: I wonder if this code is useful to other projects. A rust "freedesktop-desktop-spec" crate or something for other launchers that do this? Curious if pop's launcher has similar code.)

Also, unfortunately I'm still seeing roughly the same result:
screenshot-1711991260

@friedow
Copy link
Owner Author

friedow commented Apr 1, 2024

Under the hhod we're using the https://crates.io/crates/freedesktop-desktop-entry crate here. This provides all of the parsing for .desktop entries and is actually kindly provided by pop ;). However, this library only contains the parsing of .desktop files and no utility functions for launchers like centerpiece.

I've actually tested this with the .desktop files you provided above. For me, centerpiece only shows the "Firefox Nightly Default" entry. Can you check if you have other .desktop entries for Firefox in your system?

Screen with your three .desktop files:
2024-04-01-195405_hyprshot

@friedow
Copy link
Owner Author

friedow commented Apr 1, 2024

Some context here: centerpiece looks for .desktop files in all directories specified in $XDG_DATA_DIRS and $XDG_DATA_HOME. It will only search for desktop entries in the subfolder applications of these directories.

(Library code related to searching .desktop files: https://github.com/pop-os/freedesktop-desktop-entry/blob/main/src/lib.rs#L329-L333.)

@colemickens
Copy link

Yes, that's where other launchers like Sirula load from as well.

And indeed, that's where they are:

╭ zeph  ~  13ms
╰🡒  ls $"($env.XDG_DATA_HOME)/applications" | find firefox
────┬───────────────────────────────────────────────────────────────────────┬──────┬───────┬────────────
  # │                                 name                                  │ type │ size  │  modified
────┼───────────────────────────────────────────────────────────────────────┼──────┼───────┼────────────
  0 │ /home/cole/.local/share/applications/firefox-nightly-default.desktop  │ file │ 744 B │ 5 months
    │                                                                       │      │       │ ago
  1 │ /home/cole/.local/share/applications/firefox-nightly.desktop          │ file │ 752 B │ 5 months
    │                                                                       │      │       │ ago
  2 │ /home/cole/.local/share/applications/firefox.desktop                  │ file │ 696 B │ 2 months
    │                                                                       │      │       │ ago
────┴───────────────────────────────────────────────────────────────────────┴──────┴───────┴────────────

So, as you can see, the extra entries are being loaded, it just seems that there still isn't complete support for the spec - the Name overrides, the hiding via Hidden=true/NoDisplay=true, etc, is still not supported or working right.

@friedow
Copy link
Owner Author

friedow commented Apr 1, 2024

Just for my sanity, can you please check whether you have other .desktop files for firefox in one of the directories under $XDG_DATA_DIRS.

Also: What is wrong with the Name overrides? To me, the names seem to be correct?

Copy link
Collaborator

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, everything except the log comment is a nit.

client/src/plugin/applications.rs Outdated Show resolved Hide resolved
client/src/plugin/applications.rs Outdated Show resolved Hide resolved
client/src/plugin/applications.rs Outdated Show resolved Hide resolved
@colemickens
Copy link

colemickens commented Apr 1, 2024

I surely do have entries in the XDG_DATA_DIRS directories since those include all applications I have installed into my environment.

I'm pretty sure the XDG_DATA_HOME are meant to act as overrides if present in both.

My original issue shows Sirula correctly rendering the Firefox entries.

The three desktop files I supplied should result in two entries appearing:

  • Firefox should not appear (the entry should mask the one from XDG_DATA_DIRS and be suppressed since it has Hidden, NoDisplay)
  • DetSys should appear (Name overriden)
  • Firefox Nightly Default should appear (Name overridden)

Just to re-demonstrate, here's the only Firefox entries Sirula shows me:
screenshot-1711650263
screenshot-1711650258

@colemickens
Copy link

colemickens commented Apr 1, 2024

Quite annoying the spec refers to XDG_DATA_HOME as being a place to install desktop entries, but then does not list it in the section for where to look for entries. Nor does it talk about precedence in such a case. Very annoying. Though I'm still pretty sure this is how most other launchers work. I would expect my setup to appear correctly with GNOME/Plasma.

@a-kenji
Copy link
Collaborator

a-kenji commented Apr 1, 2024

I also find it quite annoying, but it is in a different spec:

https://specifications.freedesktop.org/menu-spec/latest/ar01s02.html

It would be nice, if they could cross reference.

@friedow
Copy link
Owner Author

friedow commented Apr 1, 2024

I guess since all other launchers just use XDG_DATA_HOME as a sole source for .desktop files if it is defined, we should do the same then?

Also: Can you please post your .desktop files for firefox again? I can't seem to find the line of code where the Name is overridden with "DetSys".

@a-kenji
Copy link
Collaborator

a-kenji commented Apr 1, 2024

From the link for the menu spec it seems that XDG_DATA_DIRS are used, which XDG_DATA_HOME is one part of:

$XDG_DATA_DIRS/applications/
This directory contains a .desktop file for each possible menu item. Each directory in the $XDG_DATA_DIRS search path should be used (i.e. desktop entries are collected from all of them, not just the first one that exists). When two desktop entries have the same name, the one appearing earlier in the path is used.

@colemickens
Copy link

Also: Can you please post your .desktop files for firefox again? I can't seem to find the line of code where the Name is overridden with "DetSys".

Oh, I'm so sorry, there's indeed a 4th desktop file for that profile:

> cat ~/.local/share/applications/detsys.desktop
[Desktop Entry]
Actions=new-private-window;new-window;profile-manager-window
Categories=Network;WebBrowser
Exec=firefox -P detsys --name firefox %U
GenericName=Web Browser
Icon=firefox
MimeType=text/html;text/xml;application/xhtml+xml;application/vnd.mozilla.xul+xml;x-scheme-handler/http;x-scheme-handler/https
Name=DetSys
StartupNotify=true
StartupWMClass=firefox-nightly
Terminal=false
Type=Application
Version=1.4

[Desktop Action new-private-window]
Exec=firefox-nightly --private-window %U
Name=New Private Window

[Desktop Action new-window]
Exec=firefox-nightly --new-window %U
Name=New Window

[Desktop Action profile-manager-window]
Exec=firefox-nightly --ProfileManager
Name=Profile Manager

@friedow
Copy link
Owner Author

friedow commented Apr 2, 2024

After some offline discussion with @a-kenji, I think I got it right this time :D. Would you kindly test again @colemickens?

@colemickens
Copy link

Oh I feel like such a bother, but: nix run github:friedow/centerpiece?ref=49a2c91915d08ce1ca3196b13e1eb24d4df15095 resulted in the same view:

screenshot-1712094780

@friedow
Copy link
Owner Author

friedow commented Apr 2, 2024

No worries at all :)!

I assume we are now running into your $XDG_DATA_DIRS having the wrong order of entries. To confirm this, please run the following:

echo $XDG_DATA_DIRS:$XDG_DATA_HOME: | sed 's/:/\/applications\n/g' | xargs -I{} find {} -name '*firefox*.desktop'

This should give us a list of firefox .desktop entries in any of the directories specified in $XDG_DATA_DIRS and $XDG_DATA_HOME. This list is actually already in the correct order. Meaning, that if there are two .desktop files in this list with the same Name property inside, the first one wins. To debug ths properly it would be nice if you could also show the contents of the found desktop files.

My guess is, that in this list there are firefox .desktop entries with have no Hidden and no NoDisplay property specified and those are ranked higher in priority (due to the order of $XDG_DATA_DIRS) then the ones you would actually like to see (or not to see rather :D).

@colemickens
Copy link

❯ echo $XDG_DATA_DIRS:$XDG_DATA_HOME: | sed 's/:/\/applications\n/g' | xargs -I{} find {} -name '*firefox*.desktop'
find: ‘/nix/store/b45sdskxbr4s71dklhd13skqvyhbzrl6-desktops/share/applications’: No such file or directory
find: ‘/home/cole/.nix-profile/share/applications’: No such file or directory
find: ‘/nix/profile/share/applications’: No such file or directory
find: ‘/home/cole/.local/state/nix/profile/share/applications’: No such file or directory
/etc/profiles/per-user/cole/share/applications/firefox.desktop
/etc/profiles/per-user/cole/share/applications/firefox-nightly.desktop
find: ‘/nix/var/nix/profiles/default/share/applications’: No such file or directory
/home/cole/.local/share/applications/firefox-nightly.desktop
/home/cole/.local/share/applications/firefox-nightly-default.desktop
/home/cole/.local/share/applications/firefox.desktop

Hm, you're right. I didn't check the spec, but XDG_DATA_DIRS normally contains system dirs, and XDG_DATA_HOME more containing user dirs, so I'm a bit surprised that XDG_DATA_HOME ones aren't considered to override or at a higher precedence.

@friedow
Copy link
Owner Author

friedow commented Apr 3, 2024

Nice :), now that we have the root cause of this problem identified, this should be fixable.

This morning I actually found the section that specifies the hirarchy of these environment variables in the XDG specification. It can be found in the XDG Base Directory Specification.

TLDR:

The order of base directories denotes their importance; the first directory listed is the most important. When the same information is defined in multiple places the information defined relative to the more important base directory takes precedent. The base directory defined by $XDG_DATA_HOME is considered more important than any of the base directories defined by $XDG_DATA_DIRS.

This means that the order in which directories are read in the freedesktop-desktop-entry crate (https://github.com/pop-os/freedesktop-desktop-entry/blob/main/src/lib.rs#L329-L333) is wrong since it specifies $XDG_DATA_DIRS before $XDG_DATA_HOME.

I'll go and fix this in the freedesktop-desktop-entry crate and I'll hotfix this by overriding this function in centerpiece for now. Will ping you here once this is ready :).

@friedow
Copy link
Owner Author

friedow commented Apr 3, 2024

Hey @colemickens, I've just pushed the hotfix prioritizing XDG_DATA_HOME over XDG_DATA_DIRS. Wanna test one more time :)?

@friedow
Copy link
Owner Author

friedow commented Apr 3, 2024

I've also created a PR to the freedesktop-desktop-entry crate to upstream this fix: pop-os/freedesktop-desktop-entry#10.

@colemickens
Copy link

I'm quite sad to report that my laptop decided to drink some coffee. I'm limping along from a non-Linux device, and giving it some time to air out. Hopefully, I can test next week, but it could be a month if it's kaput.

@friedow
Copy link
Owner Author

friedow commented Apr 4, 2024

OH NO! I'm rooting for you that it turns on again!

I'll revert my hotfix here and update the https://github.com/pop-os/freedesktop-desktop-entry crate, since the upstream fix was already merged :). After that I'll merge this PR.

Feel free to test this and open a new issue whenever you're back to normal mode @colemickens :).

…when reading desktop entries"

This reverts commit 15dfa5d.
@friedow
Copy link
Owner Author

friedow commented Apr 4, 2024

Actually, I noticed some negative performance impact in the plugin with the new functionality while doing my last tests. I guess I'll have to revise the implementation.

@colemickens
Copy link

The laptop, somehow, lives to fight more Nix and Rust (and Halo!). This latest version seems to behave as I was expecting. Thank you!!

@friedow
Copy link
Owner Author

friedow commented Apr 9, 2024

Performance is improved, the plugin does not read desktop enties repeatedly anymore, time to merge this :>.

Copy link
Collaborator

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@friedow friedow merged commit 94d275b into main Apr 9, 2024
1 check passed
@friedow friedow deleted the fix/implement-full-desktop-spec branch April 9, 2024 20:12
@friedow friedow mentioned this pull request Jun 2, 2024
friedow added a commit that referenced this pull request Jun 2, 2024
## Release Notes

### ✨ New Features
* 💄 display bars to visualize cpu core usage by @friedow in
#121
* ✨ `git_repositories`: add `zoxide` integration by @a-kenji in
#111
* ✨ enable font-fallack for all entries by @friedow in
#131
* 🌈 Allow setting colors via configuration by @pinpox in
#141
* 😜 Gitmoji plugin by @friedow in
#144
* 📑 Firefox bookmarks plugin by @friedow in
#133
* 📜 Firefox History by @friedow in
#152

### 🐛 Fixed Bugs
* 🐛 return false when file_name cannot convert to str by @prmadev in
#123
* 🐛 filter empty plugins for calculating scroll offset by @friedow in
#125
* 🐛 Add sort function to plugin trait by @a-kenji in
#124
* 🐛 implement full desktop spec by @friedow in
#120
* 🐛 fix initial sorting in a bunch of plugins by @friedow in
#129
* 🐛: fix suspend command invocation by @a-kenji in
#134

### Maintenance
* 📝 clarify wayland support by @a-kenji in
#105
* 👷 remove magic-nix-cache action by @a-kenji in
#106
* ✏️ Fix a small typo in the Readme by @a-kenji in
#112
* 🚨 Implement various clippy suggestions by @a-kenji in
#113

### New Contributors
* @prmadev made their first contribution in
#123
* @pinpox made their first contribution in
#141

**Full Changelog**:
v1.0.0...v1.1.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

desktop entries aren't quite rendered correctly
3 participants