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

Respect precedence of found desktop files and Hidden/NoDisplay #41

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

otaj
Copy link
Contributor

@otaj otaj commented Sep 23, 2023

Hi,

this PR implements precedence of desktop files (only first one with the same file name is used). It also respects Hidden/NoDisplay values of desktop entries.

The motivation for this is that I have my own modified versions of desktop files located in /usr/local/share/wayland-session which are using my own wrapper script, which are different than the ones which are installed by package manager.

It also respects Hidden=true and NoDisplay=true.

I've tested this in my system and it seems to work, however, this my first code in Rust, so it definitely might use some improvements.

Cheers

Copy link
Owner

@rharish101 rharish101 left a comment

Choose a reason for hiding this comment

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

Looks good. I'd prefer if you could split the commit into two, one for the precedence and the other for Hidden/NoDisplay, but it's not a hard requirement.

Also, could you document the file stem behaviour in the README?

src/sysutil.rs Outdated Show resolved Hide resolved
src/sysutil.rs Outdated Show resolved Hide resolved
src/sysutil.rs Outdated Show resolved Hide resolved
@rharish101
Copy link
Owner

I also have another concern: would this break existing desktops that have both X11 and Wayland sessions, but use the same file stem?

@otaj
Copy link
Contributor Author

otaj commented Sep 30, 2023

Looks good. I'd prefer if you could split the commit into two, one for the precedence and the other for Hidden/NoDisplay, but it's not a hard requirement.

I can do that, no problem.

Also, could you document the file stem behaviour in the README?

Will do as well.

I also have another concern: would this break existing desktops that have both X11 and Wayland sessions, but use the same file stem?

I don't think this happens though. Gnome, Plasma and Qtile have all different stems for Wayland and X11 sessions and I can't think of any more, that have both sessions. To be on a safe side, we can store something like parent directory + stem?

I will get to coding in couple days

@rharish101
Copy link
Owner

I don't think this happens though. Gnome, Plasma and Qtile have all different stems for Wayland and X11 sessions and I can't think of any more, that have both sessions.

In that case, it's fine.

To be on a safe side, we can store something like parent directory + stem?

Well, that would be less convenient for your use case, right? Because you would need the parent directory for your custom files to match too. I think an easier and safer option would be a config option to enable/disable this behaviour. What do you think?

@otaj
Copy link
Contributor Author

otaj commented Oct 3, 2023

I don't think this happens though. Gnome, Plasma and Qtile have all different stems for Wayland and X11 sessions and I can't think of any more, that have both sessions.

In that case, it's fine.

To be on a safe side, we can store something like parent directory + stem?

Well, that would be less convenient for your use case, right? Because you would need the parent directory for your custom files to match too. I think an easier and safer option would be a config option to enable/disable this behaviour. What do you think?

I wrangled with the code for a bit. Now I'm storing OsStr, which contains the immediate prefix (xsessions or wayland-sessions). I was not able to easily split the commits as I already merged the updates to your upstream and my git-fu is not strong enough.

src/sysutil.rs Outdated Show resolved Hide resolved
Copy link
Owner

@rharish101 rharish101 left a comment

Choose a reason for hiding this comment

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

Apart from one tiny change, all that's missing is documenting this behaviour in the README.

src/sysutil.rs Outdated Show resolved Hide resolved
@otaj
Copy link
Contributor Author

otaj commented Oct 8, 2023

Apart from one tiny change, all that's missing is documenting this behaviour in the README.

Done

@rharish101
Copy link
Owner

LGTM!

@rharish101 rharish101 merged commit e787317 into rharish101:main Oct 10, 2023
1 check passed
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.

None yet

2 participants