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

Add a right click context menu to the systray applet (to "quit/exit" it and run arch-update) #165

Merged
merged 13 commits into from
May 19, 2024
Merged

Conversation

trigg
Copy link
Contributor

@trigg trigg commented May 17, 2024

trigg added 2 commits May 17, 2024 12:35
- Add right-click-menu with two options
- - Update
- - Exit
- Added blank translation keys
@Antiz96 Antiz96 added this to the 2.0.2 milestone May 17, 2024
@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

Thanks!

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

I know this is unrelated to that PR, but is it fine if we change the name of the function that runs Arch-Update from "update" to "run", or something?

I know this is a minor detail, but I think I would prefer the name of that function to reflect better what it is actually doing (namely running Arch-Update) rather than "update" (which is one of the functions of Arch-Update). What do you think?

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Makes sense, done.

Copy link
Owner

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

Related to my previous comment, I think we can use a more "precise" description for what the systray is actually doing

po/arch-update.pot Outdated Show resolved Hide resolved
po/fr.po Outdated Show resolved Hide resolved
src/script/arch-update-tray.py Outdated Show resolved Hide resolved
@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Good point too, should be easier to understand

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

I'm currently testing this. Will merge right after if everything works as intended :)

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Quick testing can be done with LANG=fr arch-update-tray for anyone not using French by default

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

The right click menu works perfectly and is really great! 😄
However, I can't make the FR translation work for some reason... Running the script and the man pages with LANGUAGE=fr works as intended but it does not for the systray. 🤔

Running LANG=fr arch-update-tray reports Gtk-WARNING **: Locale not supported by C library. Using the fallback 'C' locale., despite the fr_FR.utf-8 locale being enabled in my test env. Am I missing something? Any idea?

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

Well, I even set my test system in french completely and the menu still is in english. So there definitely something missing, either on my side or in this PR ^^ I'm investigating.

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Double check you're installing a newer .mo file. I'll have another look shortly, but it should be automatically accounting for prefix using xdg

@Antiz96 Antiz96 closed this May 17, 2024
@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

Ok found the issue! The python script is looking for the .mo file in the default TEXTDOMAINDIR under /usr/share/locale, while I'm installing Arch-Update via the Makefile with the default /usr/local prefix in my test env.

The main script looks both at /usr and /usr/local for the .mo file/TEXTDOMAINDIR. Can something similar be achieved?

@Antiz96 Antiz96 reopened this May 17, 2024
@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

(Sorry for mistakenly closing the PR... I miss clicked once again 😅)

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Can something similar be achieved?

It absolutely can, give me a while to look into this

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Give this a test

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

I might add, it specifically checks for a French translation, as that is almost guaranteed in this project,

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

Give this a test

Woops, neither the /usr nor the /usr/local prefix have translation working now 😅

Also, I see the code looks explicitly for the "fr" translation. Would it be possible to look for "*" somehow instead (like it's done in the main script)? This is in the eventuality that we ever get more translations than the french one.
Ideally we would just act on the prefix/TEXTDOMAINDIR env var at our level and let gettext fallback to the default english translation if there's no translation for $LANG/$LANGUAGE.

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

I might add, it specifically checks for a French translation, as that is almost guaranteed in this project,

Oh! Yeah okay, smart! I missed the logic here. Ignore my previous comment.

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

fallback is set, and I'd be very worried about systems with neither XDG_DATA_DIRS and XDG_DATA_HOME.

In my case it covers
/home/trigg/.wayfire/share:/home/trigg/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share:/usr/share

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

BTW, I'll also update the main bash script to look for XDG_DATA_DIRS/XDG_DATA_HOME for the .mo file (in addition of /usr | /usr/local). I think this is the last thing where the script assumes /usr or /usr/local without looking into XDG dirs as well so the script should be all clean on that front after that \o/

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

fallback is set, and I'd be very worried about systems with neither XDG_DATA_DIRS and XDG_DATA_HOME.

Well, I have neither defined 👼

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

Impressive! One moment then

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

It will now iterate above XDG dirs, but ensure that the two expected directories are on the end of the list.

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

paths starts empty.

If XDG_DATA_DIRS is set, it's appended

if XDG_DATA_HOME is set, it's appended

regardless of both, /usr and /usr/local are appended.

We then iterate over this list until the first positive match comes up, and use that one.

In your case it'll end on the 2nd element /usr/local, in my case it'll end up on 5th element /usr. Either way we should both be covered by this now

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

Uh, I didn't catch that path was an array with all the potential paths appended to it. My bad!

@trigg
Copy link
Contributor Author

trigg commented May 17, 2024

It was placed after to allow user to override it intentionally

@Antiz96
Copy link
Owner

Antiz96 commented May 17, 2024

Still not working on my side for some reason (both with the /usr/local and /usr prefixes) 🤷

I gotta go right now, I'll make some more testing/investigation either this evening or tomorrow :)
Thanks once again for all the work!

@Antiz96
Copy link
Owner

Antiz96 commented May 18, 2024

I made some more tests and I can't make the translation works with the current code... I tried to play a bit with the script but I can't seem to find the issue.

I can confirm the translation (.mo) file works though, as replacing the whole current "# Find translations" part of the script by the following makes the translation works when using the /usr prefix:

t = gettext.translation('Arch-Update', fallback=True)
_ = t.gettext

My guess is that there's something wrong with the localedir definition, as if the script was not looking into it/using it. But that's just an assumption...

@trigg any idea?

I'll keep investigating on my side as well.

@trigg
Copy link
Contributor Author

trigg commented May 19, 2024

My apologies, something else is taking all my time recently. I'll look into this again ASAP

@Antiz96
Copy link
Owner

Antiz96 commented May 19, 2024

No problem :) Thanks!

@trigg
Copy link
Contributor Author

trigg commented May 19, 2024

It is concerning to me that my setup works before and after this edit.

This post-fixes 'locale' to the path, as that is what the python docs state is expected

@Antiz96
Copy link
Owner

Antiz96 commented May 19, 2024

Works as expected on my side now 🎉

@Antiz96
Copy link
Owner

Antiz96 commented May 19, 2024

@trigg How does that look (syntax wise)? 5f4209b

@Antiz96
Copy link
Owner

Antiz96 commented May 19, 2024

Woops, gotta get rid of the .split(":") part right?

@trigg
Copy link
Contributor Author

trigg commented May 19, 2024

I've merged the parts into one path and appended that

@trigg
Copy link
Contributor Author

trigg commented May 19, 2024

the extend adds each segment of that path as if it was an entire path alone

Copy link
Owner

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

LGTM, thanks! 🎉

@Antiz96 Antiz96 changed the title System tray context menu Add a right click context menu to the systray applet (to "quit/exit" it and run arch-update) May 19, 2024
@Antiz96 Antiz96 merged commit 8a850eb into Antiz96:main May 19, 2024
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.

Add a right click menu to the systray applet to "quit/exit" it
2 participants