DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.16k stars 39 forks source link

pupgui2: Send Download Progress and Active Downloads to DBus #409

Closed sonic2kk closed 5 days ago

sonic2kk commented 3 weeks ago

Overview

This PR enables displaying Download Progress and Active Downloads count on Task Managers, Docks, etc that listen for the com.canonical.Unity.LauncherEntry.Update DBus signal. When downloading tools, a progress bar will now appear on the Task Manager / Dock, displaying the same progress as the progress bar on the main window, as well as a count of the total active downloads. This can be useful if you have many windows open across different screens, or conversely on a single small display like the Steam Deck where you may set a tool to download and then open another fullscreen window as screen real estate is at a premium. It provides a visual indication of download progress outside of the ProtonUp-Qt main window and download count, similar to what a browser might show when downloading files.

Despite the signal name, this is supported by KDE Plasma's built-in Task Manager widgets (it may also be supported by Latte Dock, unsure), and should be supported by GNOME as well.

If a system does not have DBus, or in Flatpak if DBus is disabled, nothing will happen. We check if we can use a given DBus bus first with isConnected().

KDE Plasma Icons-Only Task Manager

This Task Manager is also used by default on SteamOS and many KDE Plasma installations with the default panel. pupgui2downloadexample2

KDE Plasma Task Manager

This is a built-in alternative to the Icons-Only Task Manager. I don't think it's used by default though. pupgui2downloadexample1

Background

The tl;dr is that we have to get down-and-dirty with DBus as there is no higher-level wrapper for this functionality that I could find with Qt, at least not in Qt6. There have been requests to replace the signal with a Freedesktop Portal (https://github.com/flatpak/xdg-desktop-portal/discussions/1276) but nothing has materialized as of yet.

This started because I noticed on SteamOS that Discover displays its download progress (I don't use a Task Manager or a Dock on my main PC), and I wondered how this was possible. After much research, I discovered based on https://github.com/ValveSoftware/steam-for-linux/issues/4777 that this is done via DBus.

From this, I figured that since Qt is fairly modern, there should be a good built-in way to hook into this DBus signal, as it is standard across at least two major Desktop Environments, KDE and GNOME. There is not, and someone actually created a custom Qt C++ class called QTaskbarControl to manage exactly this in a cross-platform way, using DBus on Linux (labelled as X11, but the underlying DBus logic works on Wayland as well).

At the time, I knew absolutely nothing about DBus, so I didn't really understand how to copy this over effectively. I didn't understand when and where to use certain identifiers. Instead of working backwards from how QtDBus does it I first started with understanding how the gdbus command works and what each part of that meant. It took a long time and a lot of playing around with things in ProtonUp-Qt, but I finally got somewhere, and so here we are!

Some other things I referenced were:

Implementation

You might know a lot more about DBus than me, and my explanations may be wrong, but I will try to explain this the way I would've liked it broken down and explained to me yesterday when I was coming at this fresh.

Emitting to com.canonical.Unity.LauncherEntry.Update via The Command Line

In order to send the DBus progress information, here's how a gdbus command would look:

# Send a 60% progress message to com.canonical.Unity.LauncherEntry.Update signal
gdbus emit --session \
    --object-path /net/davidotek/pupgui2/Update \
    --signal com.canonical.Unity.LauncherEntry.Update \
        "application://net.davidotek.pupgui2.desktop" \
        "{'progress': <0.6>, 'progress-visible': <true> }"

I'm not an expert with DBus, but here's a breakdown of my understanding:

Putting all this together, this will set the a Download Progress indicator for ProtonUp-Qt on a Task Manager, if it is running, to 60% completion.

wide task manager progress

icons only task manager progress

Emitting DBus Messages with PySide6 and QtDbus

Qt has some built-in classes for working with DBus. Since this is all still very low-level they are mostly wrappers around various DBus actions, and still very close to their C++ counterparts.

I didn't have the benefit of all of the above knowledge or how to implement in PySide6 at first, so a lot of this I figured out by hacking away in Python. The main way we can emit this signal in PySide6 and QtDBus is as follows, using QtDBus.QDBusConnection and QtDBus.QDBusMessage.

Implementing in ProtonUp-Qt

So the first part covers how we would generally send this message along dbus from the command-line. Then we get more specific in how to do this in PySide6 with QtDBus and its quirks compared to the command.

So for ProtonUp-Qt, I implemented the download progress functionality by by doing this:

Concerns

I made a few decisions when implementing this PR that I wanted to address.

Changes to MainWindow#set_download_progress_percent

I made some changes to set_download_progress_percent to use a full if/elif/else, and I removed the returns. This was because I want the method to always fall through and call dbus_progress_message. If a download is cancelled or fails it means we will properly send the negative progress and updated active downloads to DBus via dbus_progress_message, as otherwise the progress would hang. The alternative would've been putting calls to dbus_progress_message in each block that had a return.

It doesn't seem to have negatively impacted the progress method, it seems to work as it did before, since value doesn't cahnge inside of set_download_progress_percent so it should be safe. But let me know if these were not an elif for a reason, and we can revert that change and put the calls to dbus_progress_message in each of those blocks explicitly.

DBUS_INTERFACES_AND_SIGNALS Constant

I created a new constant called DBUS_INTERFACES_AND_SIGNALS. This is a dictionary of dictionaries. Each nested dictionary contains an interface and a signal key/value pair, and each of these nested dictionaries is identified with a name representing the signal that the interface and signal belong to.

My rationale for this was, in future if we ever decide to send other information over DBus to other signals, instead of hardcoding the names, we should keep track of them as a constant. And when working with them in Qt, the names are broken down into the interface and name, for example com.canonical.LauncherEntry as the interface, and then Update as the signal.

This means in wrapper functions that wrap around sending a DBus message, such as dbus_progress_message, only that wrapper function has to be aware of what the interface and name are.

However, I am not super happy about this approach. I think we should have some constant to track these pairs, but I feel like having to remember them by their higher dictionary key, such as LauncherEntryUpdate, is a bit flimsy.

Maybe this should be a dataclass/enum or something that keeps track of these names, and then we can make the higher key something like DBusSignals.LauncherEntryUpdate? I am not sure, thoughts on the current approach and any improvements are welcome!

Better Naming for dbus_progress_message

Perhaps we should make this a bit more general, like send_task_manager_progress, since it's on the MainWindow? That way as well, if this ever does get implemented at a higher level in Qt, we don't have a reference to DBus where it is unnecessary.

Efficiency

I believe the current approach to sending information to DBus is efficient. I think you have to keep sending messages to a signal that listens for it, despite the name being createSignal which did have me a little concerned that we should only make one message this way and keep re-using it. But that doesn't seem possible, and messages seem to be one-time objects that you send and dispose of.

But, I knew nothing about DBus until yesterday afternoon. I still don't really know very much, just a very basic understanding on how to send this specific kind of message over DBus. So I want to caveat this PR with the fact that, it is very possible that my approach is not correct. I based it off of my own tinkering around and examples I seen online, primarily in C++, a language I am very unfamiliar with.

So if you spot anything in the code that doesn't look right please let me know!

Systems Without DBus / Restricted Permissions

I believe handling the busses with dbus.isConnected() should be enough, but I am concerned in case the calls to QtDBus altogether fail if DBus is missing. I am not sure if this is the case and don't know how I would test for that, so it may be worth double-checking this.

In particular we may want to be careful with the Flatpak permissions, in case we need to explicitly add permissions.

Future Work

Not much to note here other than, if this ever does get added as a Portal and/or if Qt ever adds a higher level wrapping around this for us, we may want to replace this approach with that.


That was quite a lot for what should've been a very straightforward feature to implement. I hope my explanation and rationale made some sense.

All feedback on this implementation is appreciated!

Thanks!

sonic2kk commented 3 weeks ago

Should also mention that I made dbusutil a separate file just because it makes the code a little cleaner, and I think before we mentioned breaking up the util files a little bit, so it takes us the work later since this is a brand new set of utils unrelated to previous ones.

Maybe after we start introducing unit tests, we can look at breaking up the files a little more :-)

sonic2kk commented 2 weeks ago

From looking at Flatseal, there is a D-Bus session bus permission that we would probably have to add here as --socket=session-bus based on the Flatseal description: https://github.com/flathub/net.davidotek.pupgui2/blob/2042c0940a5ec0488e0e3c4f2361ba3d1bf9d408/net.davidotek.pupgui2.json#L11

sonic2kk commented 1 week ago

I should've mentioned: If you'd like me to test on other DEs let me know, but I think it should work for the major ones that have functionality which listens for this DBus signal.

It's not my intent to merge something that only benefits two specific Task Managers on KDE. It's my preferred DE but not the only one and my hope was to make something to benefit any Task Manager that can display progress like this. It just happens that I use KDE, and that this DBus signal (while originally made for Unity7) seems to be implemented by popular Task Managers, not just KDE.

DavidoTek commented 1 week ago

Thanks, that is a nice addition!

The tl;dr is that we have to get down-and-dirty with DBus as there is no higher-level wrapper for this functionality that I could find with Qt, at least not in Qt6

I feel like for some of the desktop stuff there are good libraries missing. I think it is similar with tray icons. Last time I checked, there actually were libraries available, but I think the implementation was a bit different for KDE and GNOME for example.

This tripped me up and I spent a while trying to figure out why I couldn't get the code to work, I thought the name argument could be a name that we came up with and that the interface was the same as --signal in the gdbus command

I guess that's just the paradigm with the RPC stuff... I remember I had similar issues when I wanted to implement media control using the MPRIS D-Bus interface for something.

Note how in Python we don't have to use any of the weird syntax around the arguments, we can just pass them as-is and Qt takes care of the rest.

Good. I don't like it when you can't just pass a dict but you need to create a string with a "json-like" object first.

new util file called dbusutil.py that abstracts this away To avoid having to do a lot of duplicated calls to a general create_and_send_dbus_message Should also mention that I made dbusutil a separate file just because it makes the code a little cleaner, and I think before we mentioned breaking up the util files a little bit, so it takes us the work later since this is a brand new set of utils unrelated to previous ones.

I guess we can use that if there is anything else in the future. For example, notifications also use D-Bus (org.freedesktop.Notifications). But I guess there are some wrappers for that already.

This means if ProtonUp-Qt crashes or otherwise doesn't exit cleanly (Steam Deck dies mid-download, power outage, system crash, etc), then the next time we open ProtonUp-Qt, we will clear the previous download progress data we sent to DBus and so there won't be a progress bar or download count displayed.

Oh, I didn't knew that it was persistent. That's interesting.


I made some changes to set_download_progress_percent to use a full if/elif/else

I think it is fine to handle all cases of value using if/elif/else. Not sure why I did it like this in the first place. I don't think we should include if len(self.pending_downloads): in that though, i.e., leave if value == -2 as is.

Maybe this should be a dataclass/enum or something that keeps track of these names, and then we can make the higher key something like DBusSignals.LauncherEntryUpdate? I am not sure, thoughts on the current approach and any improvements are welcome!

I think the current approach is okay, but something like an Enum. I think there are some overrides required for an Enum to store anything but integers or strings. What we can do is just have multiple variables like DBUS_LAUNCHER_ENTRY_UPDATE = {...} (or maybe a better name idk).

Perhaps we should make this a bit more general, like send_task_manager_progress, since it's on the MainWindow?

That would make sense I guess. Not sure if we are getting something high-level from Qt in the near future though :)

I believe the current approach to sending information to DBus is efficient. I think you have to keep sending messages to a signal that listens for it, despite the name being createSignal which did have me a little concerned that we should only make one message this way and keep re-using it

I can't answer that for sure, but when looking at other examples, some of them are doing it the same way. https://doc.qt.io/qtforpython-6/examples/example_dbus_pingpong.html does it differently, by first creating a QDBusInterface and then calling ping on it. I wonder if ping and Update work the same.

I believe handling the busses with dbus.isConnected() should be enough, but I am concerned in case the calls to QtDBus altogether fail if DBus is missing. I am not sure if this is the case and don't know how I would test for that, so it may be worth double-checking this.

I tested the code inside the Flatpak by running flatpak run --command=sh net.davidotek.pupgui2 without giving access to the session bus. bus.isConnected() returned True and nothing crashed :) I'm not sure what happens if there is no D-Bus at all. I can't imagine there is any desktop environment that works without it. Not sure though.


I should've mentioned: If you'd like me to test on other DEs let me know, but I think it should work for the major ones that have functionality which listens for this DBus signal.

I'm planning to test it on GNOME. I think it is a standard interface and should work on most DEs. I'm not sure what happens when sending the message to an interface that doesn't exist. Apparently to the Qt docs, send just returns False then? Returns true if the message was queued successfully, false otherwise.

sonic2kk commented 1 week ago

doc.qt.io/qtforpython-6/examples/example_dbus_pingpong.html does it differentl

I wonder if it does it differently because it's set up to send and receive signals, whereas we're just sending the message along to (presumably a Task Manager) listening for it. I don't know that for sure though.

I think the current approach is okay, but something like an Enum. I think there are some overrides required for an Enum to store anything but integers or strings. What we can do is just have multiple variables like DBUS_LAUNCHER_ENTRY_UPDATE = {...} (or maybe a better name idk).

I did have a quick look about creating an enum, and we can just create enums that store dictionaries in Python 3.10.14 (I set up a virtual environment with Python 3.10 at long last). This is perfectly valid:

from enum import Enum

class DBusSignals:
    LAUNCHER_ENTRY_UPDATE = { 'interface': 'com.canonical.Unity.LauncherEntry', 'signal': 'update' }

# Gives us the dict
print(DBusSignals.LAUNCHER_ENTRY_UPDATE.value)

If you'd like I can refactor the code to use an Enum here. We could put it in dbusutil or datastructures, or somewhere else. I'm not sure if another type of class makes more sense here over an enum.

I tested the code inside the Flatpak by running flatpak run --command=sh net.davidotek.pupgui2 without giving access to the session bus. bus.isConnected() returned True and nothing crashed :)

Oh wow that is surprising. I wonder why that permission exists for the Flatpak, maybe it is used differnetly than I expected. But that's good to know there are no alarming fires on the Flatpak side to resolve :smile: Thanks for checking that.


I think it is a standard interface and should work on most DEs. I'm not sure what happens when sending the message to an interface that doesn't exist.

It should return False yes, and at least will do nothing. I think it is very unlikely that this signal will exist and not do what we expect it to (i.e. a DE has implemented it to blow up the user's computer :laughing:) so the only scenario we need to be concerned with is if the signal doesn't exist, but even then it will simply do nothing. I got the signal wrong many times in testing and nothing happened.

I'm not sure what happens if there is no D-Bus at all. I can't imagine there is any desktop environment that works without it. Not sure though.

I am not sure either, but I'm sure if there are any desktops without dbus we will hear soon enough :smile:

DavidoTek commented 5 days ago

I tested different scenarios and everything seems to be working fine. I also ran it from inside the Flatpak, the progress bar worked there too. It's probably a standard interface. Thanks again, I will merge this now.

If you'd like I can refactor the code to use an Enum here. We could put it in dbusutil or datastructures, or somewhere else. I'm not sure if another type of class makes more sense here over an enum.

We can leave it as it is now. The Dict-Enum feels Python-y, but it's a bit too much in my opinion :smile:

I feel like having to remember them by their higher dictionary key, such as LauncherEntryUpdate, is a bit flimsy.

I just tested it, today's smart IDEs take care of it :tada:

grafik

sonic2kk commented 3 days ago

Been using my branch and now main since this merge with this feature and I think it adds a nice little bit of UI sparkle :smile: I hope others like it too.

If ever you want me to revisit the way this is implemented (or anything else) please feel free to ping me.

I just tested it, today's smart IDEs take care of it 🎉

Wow, I had no idea this feature was available in VSCode, I was about to ask what it was but I was able to find it. That is pretty cool!