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 systray applet #149

Merged
merged 41 commits into from
May 10, 2024
Merged

Add a systray applet #149

merged 41 commits into from
May 10, 2024

Conversation

trigg
Copy link
Contributor

@trigg trigg commented May 7, 2024

  • One-file python script added as src/script/arch-update-tray.py
  • Installs to /usr/bin/arch-update-tray by default
  • Clicking icon will launch the updater
  • Launching the updater is done via the .desktop file, so glib dependency is required to launch it
  • Statefile is monitored and if the first line of the file starts with 'arch-update' the entire first line, stripped of whitespace, is used as the icon.

Dependencies

  • python3
  • python-pyqt6
  • qt6-svg
  • glib2
  • qt6-wayland (for people running Wayland)

Closes #146

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

A note: If we want to pick sides (most likely for the sake of simplicity) between the toolkits, this could be cut down noticably.

I simply wanted to try my hand at making it as available as I could.

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

Just some quick comments after a first read:

[...]
* * Due to a fun quirk in GTK AppIndicator, clicking the icon opens a menu with the update option in it.

Uh... Too bad :(

[...]
* Launching the updater is done via the .desktop file, so another dependency is required to launch it (more about this later)

Is there a specific reason to launch it via the .desktop file rather than calling the arch-update bin directly?

[...]
* environment variable ARCH_UPDATE_TK is checked. If set to gtk3 or qt6 then prefer that toolkit, if multiple toolkits are installed. Defaults to qt6 for no reason at all.

"Defaults to qt6 for no reason at all." <-- Haha. Well, if the QT6 toolkit allows a direct launch of the updater (where the gtk one opens a menu first), that's a good enough reason I guess :D

[...]

Dependencies

Naturally this is a Lot of extra dependencies above what the project requires previously. I've tried to lessen the impact by allowing alternatives.

* `python3`

Fair enough 🤣

[...]
Note that I'm not certain the list here is comprehensive, testing will be necessary

Alright, no problem. Should be able to test that in a minimal environment to make sure nothing's missing :)

[...]
A note: If we want to pick sides (most likely for the sake of simplicity) between the toolkits, this could be cut down noticably.
I simply wanted to try my hand at making it as available as I could.

Well, I'm in a favor of making this as simple as possible (at least to start things of), ideally both in terms of dependencies and about the python script itself (also for maintenance reasons. While the script looks fairly simple and intelligible, I'm definitely not as comfortable with Python than I am with bash 😅). So I'm all in favor of picking a side on that front.

Usually I would personally choose GTK over QT for some reasons, but if the "menu quirk" cannot be fixed or worked around, I'm afraid that's a non-starter :/
In such case, I guess going with the QT toolkit and, hopefully, not the kde-cli-tools launcher would do 😜

Once again, thank you so much for your incredible work on this! 🙏
I'll take some time (hopefully this evening) to create the missing PR, merge the other ones, and start testing this Python script! 🐍

@Antiz96 Antiz96 added this to the v2.0.0 milestone May 7, 2024
@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

I also saw you made pylint exclusions/references in the script. Can I had pylint to the test CI?

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

Is there a specific reason to launch it via the .desktop file rather than calling the arch-update bin directly?

It nets your the users preferred terminal. Running it directly will be unhelpful as it'll get no terminal, and we could take guesses at which terminal installed the user would prefer.

The launchers used will do it in a more sensible way than I could manage in a handful of lines of code

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

I also saw you made pylint exclusions/references in the script. Can I had pylint to the test CI?

You're welcome to!

I always use them locally because it's a huge burden off me to be able to auto-format python.

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

Is there a specific reason to launch it via the .desktop file rather than calling the arch-update bin directly?

It nets your the users preferred terminal. Running it directly will be unhelpful as it'll get no terminal, and we could take guesses at which terminal installed the user would prefer.

The launchers used will do it in a more sensible way than I could manage in a handful of lines of code

Oh yeah right, good catch!

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

"menu quirk" cannot be fixed or worked around, I'm afraid that's a non-starter :/

Yeah, this is a huge problem for me. From everything I'm reading AppIndicator is dropped entirely in Gtk4, and they put as much effort into breaking it as possible before that happened too.

I keep thinking there should be a toolkit-agnostic way to do this, but I've not found it yet

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

Yeah, this is a huge problem for me. From everything I'm reading AppIndicator is dropped entirely in Gtk4, and they put as much effort into breaking it as possible before that happened too.

Well, I guess that coincides with the fact that Gnome wants to get rid of systrays altogether :P

I guess this is also an additional legit reason to go with QT?

I keep thinking there should be a toolkit-agnostic way to do this, but I've not found it yet

Hopefully there is, maybe we'll find one at some point ^^

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

I added pylint to CI. Do you mind checking the output to see what can be addressed (or eventually ignored if needed)?

I assume that picking a side regarding the toolkit will allow to remove some parts of the code. Regarding the apparent GTK position regarding AppIndicator, I guess the safe choice is to go with QT6.
Glib2 feels like a good choice to me regarding the launcher.

trigg added 4 commits May 7, 2024 21:29
- - Doing so successfully would require we use python pip for our packaging, which is a problem for another day
- Some minor alterations for pylint
@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

C0103: Module name "arch-update-tray" doesn't conform to snake_case naming style (invalid-name)

My opinion is this is one for "we don't care"... the import ones we couldn't get around without switching the project to using pip or some other python-first build system, but this one is "we want your file to use underscores and not minus" ... which I don' think matters here as this isn't a module to begin with

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

C0103: Module name "arch-update-tray" doesn't conform to snake_case naming style (invalid-name)

My opinion is this is one for "we don't care"... the import ones we couldn't get around without switching the project to using pip or some other python-first build system, but this one is "we want your file to use underscores and not minus" ... which I don' think matters here as this isn't a module to begin with

Yeah fair enough, sounds good to me. 👍
Seems like CI is now green ✔️ 🎉

Thanks!

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

Do we want it stripped of GTK before going ahead?

It brings the least with it and has the higher bar for requirements

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

Well, I was about to ask your opinion about going with python-pyqt6 + glib2 but it seems your mind is set 🙂
Let's strip it then!

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

Oh out of AppIndicator3 and QSystemTrayIcon there's a clear winner.

A bit confusing choosing glib2 and pyqt6 but... kioclient automatically assumes konsole in my case.

It'll make requirements page much simpler than trying everything-and-kitchen-sink.

@trigg
Copy link
Contributor Author

trigg commented May 7, 2024

Something I didn't mention earlier...

Python pulls in the gettext translations set by the rest of this package, but never ends up using it in any meaningful way. For now I'll remove it again, but it's always an option to have menus etc with translated text

- Remove exo open
- Remove kioclient
- Remove gettext
@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

A bit confusing choosing glib2 and pyqt6 but...

Why is that? To give you my train of thoughts: pyqt6 is the only option listed to go with the QT6 toolkit. As for glib2, it has the advantage of being a dependency for a lot of packages, meaning it is probably already installed on basically every people's system. But am I missing something?
Is this because it's intended to get along with kde-cli-tools? (See below)

...kioclient automatically assumes konsole in my case.

I don't mind QT but I'd like to avoid KDE stuff if possible. I'm genuinely triggered by such "assumptions" as you described above as well as the insanely huge amount of dependencies they require. 😵‍💫

It'll make requirements page much simpler than trying everything-and-kitchen-sink.

That's the aim here. Keep it simple as they say :)

@Antiz96
Copy link
Owner

Antiz96 commented May 7, 2024

Something I didn't mention earlier...

Python pulls in the gettext translations set by the rest of this package, but never ends up using it in any meaningful way. For now I'll remove it again, but it's always an option to have menus etc with translated text

Alright, thanks for info. I don't intend to add any kind of menu for now so it's fine dropping it.
But good to know still.

@Antiz96
Copy link
Owner

Antiz96 commented May 8, 2024

Alright, I think we're good for the systray applet itself, everything seems to work as intended. 🥳
Once again, thanks a lot for your incredible work and your precious help on that front @trigg! 🙏

I'll proceed with the remaining parts listed here as soon as possible. Once done, and after an additional round of extended tests (just for good measures), we can merge this PR! 😄

I should have the new icons set merged by then so we will hopefully be able to cut the v2.0.0 release soon after 🎉

@trigg
Copy link
Contributor Author

trigg commented May 9, 2024

Looks good! Glad we found the rest of the dependencies, I had a feeling something else was needed

@Antiz96
Copy link
Owner

Antiz96 commented May 10, 2024

@trigg Hey! Can you sync/update your fork so I can cherry-pick the new icons set into that PR please? :)

I'm trying to get this merged today \o/

@trigg
Copy link
Contributor Author

trigg commented May 10, 2024

Done that now

@Antiz96
Copy link
Owner

Antiz96 commented May 10, 2024

Thx!

@Antiz96
Copy link
Owner

Antiz96 commented May 10, 2024

@trigg Some updated icons have been merged in the main branch. Are you available to re-sync your fork? 🙏

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.

It took quite some time but I finally adapted documentation, added the few remaining specs and ran tests. I can finally say it: LGTM!

One last time, thanks for your incredible work and help on this.

I'll tag a new release shortly. Let's ship this applet to people! 🚀

@Antiz96 Antiz96 merged commit fdbe905 into Antiz96:main May 10, 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.

Make a proper systray applet for Arch-Update
2 participants