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

Make systray applet also searching in XDG_DATA_HOME & XDG_DATA_DIRS when looking for the .desktop file #156

Merged
merged 9 commits into from
May 13, 2024

Conversation

Antiz96
Copy link
Owner

@Antiz96 Antiz96 commented May 13, 2024

This PR makes the systray applet searching for the .desktop file in the following path (in that specific order):

  • $XDG_DATA_HOME/applications/arch-update.desktop
  • $HOME/.local/share/applications/arch-update.desktop
  • $XDG_DATA_DIRS/applications/arch-update.desktop
  • /usr/local/share/applications/arch-update.desktop
  • /usr/share/applications/arch-update.desktop

That would allow users to be able to provide a customized .desktop file for the systray applet to execute if needed (e.g. force Arch-Update to launch in a specific terminal emulator that is not supported by gio/glib2).

Fixes #151

@Antiz96 Antiz96 added this to the v2.0.1 milestone May 13, 2024
@Antiz96
Copy link
Owner Author

Antiz96 commented May 13, 2024

@trigg Can you give this a look?

I used the same logic currently used to determine where the state file is, except I added some if not os.path.isfile statement between each of the possible paths. While the state file creation/location is fully automated, we could imagine that a user could put a custom .desktop file under $HOME/.local/share/applications/arch-update.desktop despite having $XDG_DATA_HOME set to somewhere else for instance.

Hopefully the syntax and indentation is okay 😅
I'll run some tests later today.

By the way, I know you asked me to assign you to the linked issue, but I wanted to give this a try myself to get familiar with the systray applet codebase. I hope you don't mind 😶

@trigg
Copy link
Contributor

trigg commented May 13, 2024

I hope you don't mind

Not at all! I apologise, my work has kept me busy lately.

My concern with this code is that it makes several references to DESKTOP_FILE before it is defined.

I think we need to start off setting it to None and check

if not DESKTOP_FILE or not os.path.isfile(DESKTOP_FILE):

which will more cleanly catch both option (file wasn't already set or file is not valid for our use)

@trigg
Copy link
Contributor

trigg commented May 13, 2024

Simulating a system where XDG_DATA_HOME is unset...
env -u XDG_DATA_HOME ./src/script/arch-update-tray.py

Traceback (most recent call last):
  File "/home/triggerhapp/dev/arch-update5/./src/script/arch-update-tray.py", line 68, in update
    arch_update()
  File "/home/triggerhapp/dev/arch-update5/./src/script/arch-update-tray.py", line 31, in arch_update
    if not os.path.isfile(DESKTOP_FILE):
                          ^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'DESKTOP_FILE' where it is not associated with a value

@trigg
Copy link
Contributor

trigg commented May 13, 2024

I've made a PR to your PR 😁

@Antiz96
Copy link
Owner Author

Antiz96 commented May 13, 2024

Not at all! I apologise, my work has kept me busy lately.

Oh don't worry, no problem! Take all the time you need ;)

My concern with this code is that it makes several references to DESKTOP_FILE before it is defined.
I think we need to start off setting it to None and check

if not DESKTOP_FILE or not os.path.isfile(DESKTOP_FILE):

which will more cleanly catch both option (file wasn't already set or file is not valid for our use)

Oh yeah you're right, thanks!

I've made a PR to your PR 😁

Can you make "suggestions" I can commit to this one directly instead (so we keep everything in the same PR)?

@trigg
Copy link
Contributor

trigg commented May 13, 2024

The PR I made is pointing at this branch, not master. But by all means just recreate what I did in the changes here

https://github.com/Antiz96/arch-update/pull/157/files

@Antiz96
Copy link
Owner Author

Antiz96 commented May 13, 2024

The PR I made is pointing at this branch, not master.

Oh sorry, I missed that! Thanks :)

@Antiz96
Copy link
Owner Author

Antiz96 commented May 13, 2024

I'll adapt the documentation in that PR so the fact that the systray is now looking for those different paths is documented and then we should be good to merge it :)

I'll probably cut a v2.0.1 release right after because I already had one feedback from someone that would like this improvement to be shipped for their own need :)

Copy link
Owner Author

@Antiz96 Antiz96 left a comment

Choose a reason for hiding this comment

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

✔️

@Antiz96 Antiz96 merged commit 5fcd69c into main May 13, 2024
1 check passed
@Antiz96 Antiz96 deleted the systray_desktop_file branch May 13, 2024 17:52
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.

Tray should check all of XDG_DATA_DIRS and XDG_DATA_HOME for arch-update
2 participants