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

Records (all) Applications that Send Notifications #162

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chrisdotcode
Copy link

@chrisdotcode chrisdotcode commented Feb 2, 2022

In tandem with elementary/switchboard-plug-notifications#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!

@chrisdotcode chrisdotcode marked this pull request as draft February 2, 2022 10:06
@chrisdotcode chrisdotcode changed the title Records Applications that Send Notifications. Records (all) Applications that Send Notifications Feb 2, 2022
@jeremypw
Copy link

jeremypw commented Mar 6, 2022

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
Copy link
Author

chrisdotcode commented Mar 6, 2022

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
Copy link
Author

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
Copy link

jeremypw commented Mar 6, 2022

Yes, sorry - for some reason I did not notice that you were, in fact, checking for duplicates 😞 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 jeremypw requested a review from danirabbit March 6, 2022 15:05
@jeremypw
Copy link

jeremypw commented Mar 6, 2022

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

@@ -1,6 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<schemalist>
<schema path="/io/elementary/notifications/" id="io.elementary.notifications">
<key type="as" name="applications">
Copy link

Choose a reason for hiding this comment

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

This is the same name as an existing subfolder which might be confusing. Not sure what a better name would be. Maybe "notifying-applications"?

@jeremypw
Copy link

jeremypw commented Mar 6, 2022

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

@jeremypw
Copy link

jeremypw commented Mar 6, 2022

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?

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.

2 participants