search

elementary / notifications

Gtk Notifications Server
https://elementary.io
GNU General Public License v3.0
38 stars 6 forks source link

Records (all) Applications that Send Notifications #162

Open chrisdotcode opened 2 years ago

chrisdotcode commented 2 years ago

In tandem with https://github.com/elementary/switchboard-plug-notifications/pull/88 (so please read that one first), this PR lays the groundwork for dynamic applications to be recorded.

The method used is that all applications that send a notification are recorded here, and will be later filtered and processed once the switchboard notifications plug is opened. My reasoning for putting very little processing in this code itself is because I care about my battery life: notifications are sent many more times than the switchboard is open, and so it's much faster to just modify a single gsetting here, and do necessary processing when the switchboard gets opened. This also explains the implementation: initially I had a "not known" applications list, and a "known applications list", but it 1. added more complexity to the code than was needed and 2. using a single list is completely sufficient here, since in the switchboard, we fetch all of the applications anyway, so we can do filtering there.

Additionally, the has-app setting is used to note whether or not the desktop entry in a given notification hint actually corresponds to a specific desktop entry on disk (gnome-power-panel is one such counterexample: it sends notifications as gnome-power-panel, but no such gnome-power-panel exists on disk that I could find).

Once again, please point me in the right direction eOS team, and I'll be happy to keep working on this!

jeremypw commented 2 years ago

I am concerned that the array of applications can grow without limit. It would be better to check for duplicates before updating it - when idle or after a timeout if necessary to avoid blocking. The array could be loaded at startup and only rewritten if a new application appears.

There is also the question about what to do when an application in the list gets uninstalled that has not been addressed.

chrisdotcode commented 2 years ago

Thanks @jeremypw for some love.

You're right in that the array application list can grow boundlessly, but can't the list of installed applications (retrieved from AppInfo.get_all() in the switchboard code) grow boundlessly as well? ~That being said, a duplicate check, especially if we load the array during startup and only write for new applications makes a lot of sense~. The code is already checking to see if the entry exists before adding it, but we can certainly keep the array in memory from the start, and therefore not have to look it up every time (saving us even more battery!).

Uninstalled apps are also tricky, thanks for mentioning. As of now, in my switchboard code, not-found apps are treated as if they are "dynamic applications" (once again, gnome-power-panel as an example: it has code that sends notifications, but no desktop entry with that corresponding name). In actuality, a not-found application name can be either a dynamic app, or an uninstalled app. I'll have to think about a solution a bit more, and will gladly take some more feedback.

chrisdotcode commented 2 years ago

With respect to uninstalled apps, do we have any vala/flatpak triggers we can use at the time an app is uninstalled? Because if so, could that help us solve the problem?

jeremypw commented 2 years ago

Yes, sorry - for some reason I did not notice that you were, in fact, checking for duplicates :disappointed: This should be OK, then. I thought there was going to be an entry for each notification! The number of installed apps should remain a manageable number I guess.

I suggest looking at the AppCenter code for info on getting a list of installed apps. At least, those installed by apt and flatpak. Apps and scripts installed from source unlikely to be covered though.

jeremypw commented 2 years ago

Requesting a review from "higher up" to check that this is something we want to implement.

jeremypw commented 2 years ago

Note that it is better to make your changes to a new branch on your fork, not to master.

jeremypw commented 2 years ago

At the moment the Notification.app_id defaults to "gala-other" unless "desktop-entry" hint is found. Do you need to deal with non-standard notifications so they get a specific app id if possible?