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

Desktop entries update #219

Merged
merged 14 commits into from
Jul 12, 2024
Merged

Conversation

wiiznokes
Copy link
Contributor

@wiiznokes wiiznokes commented Jun 8, 2024

  • run cargo clippy and fmt on the all project
  • update edition
  • update fde (depend on Fix applist freedesktop-desktop-entry#20)
  • update the desktop_entries plugin to take advantage of the matching function of fde

I plan to start playing with the score now, for modifying to order of the result. I don't include it in this PR to keep things separate, but since the new matching function will match more things, currently, the search result are not as good as previously. see #217

mmstick
mmstick previously approved these changes Jun 10, 2024
@mmstick mmstick requested a review from a team June 10, 2024 14:20
@wiiznokes wiiznokes mentioned this pull request Jun 11, 2024
2 tasks
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

I noticed a search behavior change that seems odd. Searching for "test", I used to get the Blackmagic RAW Speed Test app I have installed on this system as the first result:

Screenshot from 2024-06-26 11-42-57

On this branch, it doesn't show up as a result at all when searching for that string:

Screenshot from 2024-06-26 11-43-17

Searching for something more specific does yield the result, so it's still being detected as an entry by the launcher:

Screenshot from 2024-06-26 11-43-46

Any idea why this update would cause fuzzy matches to have priority over an exact match? Not matching an exact term anymore seems like a regression.

@wiiznokes
Copy link
Contributor Author

Yes, I think i understand why this is the case. The old impl use

let append = search_interest.starts_with(&*query)
                    || query
                        .split_ascii_whitespace()
                        .any(|query| search_interest.contains(&*query))
                    || strsim::jaro_winkler(&*query, &*search_interest) > 0.6;

for matching while the new only use strsim::jaro_winkler(&*query, &*search_interest) (in fde).

Having both

search_interest.starts_with(&*query)
                    || query
                        .split_ascii_whitespace()
                        .any(|query| search_interest.contains(&*query))

seems redundant. I will add the contains condition and update you

@jacobgkau
Copy link
Member

No rush, but just to make sure this isn't waiting on us again, I assume something will need to be done in this PR to bring pop-os/freedesktop-desktop-entry#23 in? Not sure if the freedesktop-desktop-entry version needs to be bumped, but we at least need a new git commit hash to trigger a rebuild.

@wiiznokes
Copy link
Contributor Author

I assume fde needs a new version

@wiiznokes wiiznokes mentioned this pull request Jul 3, 2024
@jacobgkau
Copy link
Member

@mmstick Can you make a crate release including pop-os/freedesktop-desktop-entry#23 so we can bump the version used in this PR to fix that regression?

@mmstick
Copy link
Member

mmstick commented Jul 3, 2024

Sure

@mmstick
Copy link
Member

mmstick commented Jul 3, 2024

Released as 0.6.2

mmstick
mmstick previously approved these changes Jul 3, 2024
@wiiznokes
Copy link
Contributor Author

@jacobgkau Sorry for the ping, i just making sure you aware that this pr is now ready for review again :)

@jacobgkau
Copy link
Member

@wiiznokes I saw it this morning. There's some other work we're busy with at the moment, but I will review this again this week, hopefully within the next day or so.

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

On 4f281f7, the Blackmagic RAW Speed Test app now shows up when searching test. However, searching for settings does not return the Settings app (a.k.a. GNOME Control Center) as a result. On this machine, it currently shows up as the second option when searching for that term on the released/master branch.

Screenshot from 2024-07-09 17-33-06
Screenshot from 2024-07-09 17-33-25

mmstick
mmstick previously approved these changes Jul 10, 2024
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

The test and settings queries are both working as expected now.

Standard regression testing failed. GNOME Settings panels are not showing up in search anymore with this update. This can be tested with e.g. appearance for the Appearance panel or wifi for the Wi-Fi panel.

@wiiznokes
Copy link
Contributor Author

I suppose this is a popos only thing? Would you be able to provide an example of .desktop file that don't work? I'm on Fedora

@mmstick
Copy link
Member

mmstick commented Jul 10, 2024

gnome-control-center desktop entries in the GNOME session should be searchable.

@jacobgkau
Copy link
Member

jacobgkau commented Jul 10, 2024

Would you be able to provide an example of .desktop file that don't work?

Here's one, for the Appearance page:

[Desktop Entry]
Name=Appearance
Comment=Set light or dark theme
Exec=gnome-control-center desktop-appearance
# FIXME
# Translators: Do NOT translate or transliterate this text (this is an icon file name)!
Icon=preferences-pop-desktop-appearance
Terminal=false
Type=Application
NoDisplay=true
StartupNotify=true
Categories=GNOME;GTK;Settings;DesktopSettings;X-GNOME-Settings-Panel;X-POP-DesktopSettings;
OnlyShowIn=GNOME;Unity;
# Translators: Search terms to find the Privacy panel. Do NOT translate or localize the semicolons! The list MUST also end with a semicolon!
Keywords=
X-Ubuntu-Gettext-Domain=gnome-control-center-2.0

It's installed to /usr/share/applications/gnome-desktop-appearance-panel.desktop in Pop!_OS.

@wiiznokes
Copy link
Contributor Author

I see. Last commits should solve this issue.

The current behavior seems odd to me. The launcher will search in desktop entries that set NoDisplay=true, which is technically fine, but will discard any entry that set NoDisplay=true and don't have an OnlyShowIn field. This is the case for example for applets entries.

I'm not sure if applets should use the desktop enty spec here. It could potentially impact other DEs if i understand correctly. cc @mmstick

@mmstick
Copy link
Member

mmstick commented Jul 10, 2024

The previous logic was based on the session. If you are in the GNOME session, then desktop entries which use OnlyShowIn=GNOME will appear regardless of NoDisplay=true.

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Jul 10, 2024

Yes, i guess this is fine, even if the launcher could miss some useful entry of normal app (i don't have an example but an app not specific to a DE could use NoDisplay=true.

This means another launcher could choose to display this kind of entries, and then, all cosmic applets shows up.

@mmstick
Copy link
Member

mmstick commented Jul 10, 2024

Yeah, we similarly want cosmic settings pages to appear in the launcher, but not in the app library.

@wiiznokes
Copy link
Contributor Author

Yes, but my point was related to cosmic applets. AFAIU, cosmic applets are not meant to be presented in launchers or app library

@wiiznokes
Copy link
Contributor Author

And the only things that prevent them to being displayed are NoDisplay=true and no OnlyShowIn field, which is not in the spec, if i'm not mistaken

@mmstick
Copy link
Member

mmstick commented Jul 10, 2024

It's what GNOME is doing, so we're working with behaviors that are already being relied upon.

@wiiznokes
Copy link
Contributor Author

I see. The last 2 commits implements this behavior

@wiiznokes wiiznokes requested a review from jacobgkau July 10, 2024 23:40
mmstick
mmstick previously approved these changes Jul 10, 2024
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

Found another issue on 6ec06ef during standard regression testing. Searching for extensions should only return one entry. Before:

Screenshot from 2024-07-11 15-35-04

With this branch, there are two results for that app:

Screenshot from 2024-07-11 15-35-32

This .desktop file comes from /usr/share/applications/org.gnome.Extensions.desktop:

[Desktop Entry]
Type=Application
Name=Extensions
# Translators: Do NOT translate or transliterate this text (this is an icon file name)!
Icon=org.gnome.Extensions
Comment=Configure GNOME Shell Extensions
Exec=/usr/bin/gnome-extensions-app --gapplication-service
DBusActivatable=true
Categories=GNOME;GTK;Utility;
OnlyShowIn=GNOME;
X-Ubuntu-Gettext-Domain=gnome-shell

@wiiznokes
Copy link
Contributor Author

Damn, this will never end 😭

I don't really know why there is 2 extensions now. I'm using the same logic to deduplicate entries, e.i, only with the appid. I made a test on my machine and it seems to deduplicate as intented. I think it would help if you can provide the other org.gnome.Extensions.desktop

Thanks

@jacobgkau
Copy link
Member

Aha. I found this issue from just before this item was added to our QA checklist: pop-os/shell#293

In addition to the org.gnome.Extensions.desktop that I posted before, there is also an org.gnome.Shell.Extensions.desktop installed to the same location:

[Desktop Entry]
Type=Application
# Keep in sync with subprojects/extensions-app
Name=Extensions
# Translators: Do NOT translate or transliterate this text (this is an icon file name)!
Icon=org.gnome.Shell.Extensions
# Never launch this, just provide name+icon to portal dialog
Exec=false
OnlyShowIn=GNOME;
NoDisplay=true
X-Ubuntu-Gettext-Domain=gnome-shell

I do want to point out that if existing behaviors are breaking the spec, we could probably consider following the spec if we fix those behaviors in their respective other components (i.e. patch the .desktop files in gnome-shell/gnome-control-center/etc). We just can't ship the update when it would break user-facing behaviors on the checklist.

As for why this duplicate wasn't showing up before, I think it's because the exec line is set to false. If I change the exec line for e.g. LibreOffice Writer's .desktop file to false, it shows with this branch and doesn't show on master. So that's the regression we're looking at now.

@wiiznokes
Copy link
Contributor Author

As for why this duplicate wasn't showing up before, I think it's because the exec line is set to false.

Yep, that must be it. I don't think this is in the spec but that's not surprising coming from Gnome

@wiiznokes
Copy link
Contributor Author

Should be hopefully good now

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

The test and settings queries are still working as expected.

Standard regression testing passed:
  • All windows on all workspaces appear on launch
  • Choosing an app on another workspace moves workspaces and focus to that app
  • Launching an application works
  • Typing text and then removing it will re-show those windows
  • Search works for applications and windows
  • Search works for GNOME settings panels
  • Search for "Extensions". There should be only one entry.
  • The overlay hint correctly highlights the selected window
  • Open windows are sorted above applications (e.g. "firefox")
  • t: executes a command in a terminal
  • : executes a command in sh
  • = calculates an equation
  • Search results are as expected:
    • cal returns Calendar and Calculator before Color
    • pops returns Popsicle first
    • shop returns the Pop!_Shop first

@mmstick mmstick merged commit 1725320 into pop-os:master Jul 12, 2024
8 checks passed
@wiiznokes wiiznokes deleted the desktop_entries_update branch December 11, 2024 13:56
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.

3 participants