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

Draft background: Add ability to specify timers to start app #1098

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

Conversation

jsparber
Copy link
Contributor

@jsparber jsparber commented Sep 8, 2023

This is more a proof of concept therefore it's marked as draft.

Since autostart on system start and autostarting an app in ad a specific time are closely related it makes sense for me to not introduce a new portal but add an additional feature to the Background portal.

I implemented the timer backend in xdg-dekstop-portal-gnome using systemd user timers. (https://gitlab.gnome.org/jsparber/xdg-desktop-portal-gnome/-/commits/add_timer_background)

And in gnome clocks https://gitlab.gnome.org/jsparber/gnome-clocks/-/commits/use_background_portal

`xdp_app_info_rewrite_commandline` has to respect `quote_escape` also
when no commandline is given.
@GeorgesStavracas GeorgesStavracas added new api This requires adding API to an existing portal portal: background Background portal and background monitoring labels Oct 3, 2023
@@ -704,6 +710,10 @@ handle_request_background_in_thread_func (GTask *task,
AutostartFlags autostart_flags = AUTOSTART_FLAGS_NONE;
gboolean activatable = FALSE;
g_auto(GStrv) commandline = NULL;
const char * const *run_times = { NULL };
Copy link
Collaborator

@hfiguiere hfiguiere Nov 27, 2023

Choose a reason for hiding this comment

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

Suggested change
const char * const *run_times = { NULL };
g_autofree char * const *run_times = NULL;

^a&s return like g_variant_get_strv() it will leak otherwise.

@razzeee
Copy link
Contributor

razzeee commented Dec 6, 2023

As the context wasn't clear to me and I needed to dig a little bit.

This can be used to wake up the system and start an app with certain commands.

The example julian implemented is for gnome on mobile devices. Specifically the clocks/alarms app saving their alarms via the portal. And the system then waking and starting the app.


commandline = xdp_app_info_rewrite_commandline (request->app_info, autostart_exec,
FALSE /* don't quote escape */);
if (commandline == NULL)
{
g_debug ("Autostart not supported for: %s", app_id);
g_debug ("Autostart and schedualed autostart not supported for: %s", app_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
g_debug ("Autostart and schedualed autostart not supported for: %s", app_id);
g_debug ("Autostart and scheduled autostart not supported for: %s", app_id);

Typo

GVariant *options,
GError **error)
{
// TODO: validate
Copy link
Contributor

Choose a reason for hiding this comment

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

Marking this todo with a comment

<!--
EnableTimer:
@app_id: App id of the application
@timer: List of times the app should be started
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it kind of weird, that this is a list but named with a singular. While flags is named plural. So I guess this should be plural too?

@razzeee
Copy link
Contributor

razzeee commented Dec 6, 2023

One thing I couldn't gather from this (but that might be due to me not knowing much about portals etc) is how the timers have to look, but my assumtion of that being a string might just be wrong.

Anyway, mostly I'm wondering if timezones are something this should care about.

@jsparber
Copy link
Contributor Author

jsparber commented Dec 6, 2023

One thing I couldn't gather from this (but that might be due to me not knowing much about portals etc) is how the timers have to look, but my assumtion of that being a string might just be wrong.

Anyway, mostly I'm wondering if timezones are something this should care about.

This needs a proper format definition for the timers. I used simple systemds format which includes timezones see https://wiki.archlinux.org/title/Systemd/Timers

Thanks for the code review, but I'm looking actually more for feedback if this portal and implementation direction makes sense.

@sonnyp sonnyp marked this pull request as draft December 19, 2023 16:55
@@ -873,11 +907,23 @@ validate_commandline (const char *key,

return TRUE;
}
static gboolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

`nit

Suggested change
static gboolean
static gboolean

g_ptr_array_add (args, g_shell_quote (app_info->id));
} else {
g_ptr_array_add (args, g_strdup (app_info->id));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the indentation style doesn't match

}
else if (!xdp_dbus_impl_background_call_enable_autostart_sync (background_impl,
else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the indentaton style doesn't match

@calebccff
Copy link

imho the design of this portal could have a huge knock-on effect for years to come as apps adopt it, I have a few concerns with the proposal here:

  1. Requiring that apps provide precise times will make it impossible to later optimise them by grouping tasks together as we have no way to know how precise the specified time actually needs to be (e.g. if your email and RSS app both want to update every 30 minutes, but don't care when, we should be able to have the device wake up just once every 30 minutes to handle this rather than twice).
  2. Not having a way to provide hints about what kind of work the app wants to do similarly makes future optimisation work harder to do without breaking apps.

Obviously a lot of these potential future optimisations require a lot of work across the entire userspace stack, but I think if we take some steps to set ourselves up for that it will help a lot as we move more in this direction.

Android has over a decades worth of theory-in-practise on this subject, so I think we should at the very least see what they're doing and make sure that we don't repeat the same mistakes.

The modern approach is WorkManager: https://developer.android.com/develop/background-work/background-tasks/persistent#workmanager-features

It has a lot of dynamic options for scheduling and prioritising work, as well as handling things like retrying work that failed, specifying requirements like network access, or that some tasks should only be run on unrestricted networks, or not run when on low battery, etc, etc...

The Doze overview page is also really useful here: https://developer.android.com/training/monitoring-device-state/doze-standby

If the scope of this draft PR is just to handle things like GNOME clocks to let them wake up the system, then I think we should be explicit in the API: it should allow scheduling one or more alarms that will wake up the device and launch the app. Apps that are not literally alarm apps should not be using this API.

If we want to handle stuff like email/rss sync, we should have an API that allows these apps to properly express their intent so that we can optimise them in the future (and be clear about what capabilities are offered, to-the-minute accuracy probably not being one of them). Otherwise we risk making it nearly impossible to design a system like Doze without breaking apps, as well as potentially ending up in a situation where your phone screen turns on every 15 minutes because the RSS reader wants to check for updates...

The Android "setAlarmClock()" API definitely tells a story here: https://developer.android.com/reference/android/app/AlarmManager#setAlarmClock(android.app.AlarmManager.AlarmClockInfo,%20android.app.PendingIntent)

@jsparber
Copy link
Contributor Author

@calebccff Thanks for your valuable feedback. This isn't intended to be only used by alarm clocks apps, so i think extending ways of how an app can set the time is feasible.

@jsparber
Copy link
Contributor Author

I finally have some time to look into this again and hopefully get it finished.

@calebccff

Requiring that apps provide precise times will make it impossible to later optimise them by grouping tasks together as we have no way to know how precise the specified time actually needs to be (e.g. if your email and RSS app both want to update every 30 minutes, but don't care when, we should be able to have the device wake up just once every 30 minutes to handle this rather than twice).

That's actually not the case, with the current implementation apps can specify times in whatever format systemd understands (i know we need to define the format better and in relation to this API). So it's possible to say weekly or every 30min.

It has a lot of dynamic options for scheduling and prioritising work, as well as handling things like retrying work that failed, specifying requirements like network access, or that some tasks should only be run on unrestricted networks, or not run when on low battery, etc, etc...

I think it's is out of scope for a first version, but i think you are right we shouldn't make future additions harder. In that sense i'm not sure how elegant it can be to have the "wake" portal part of the background portal, especially for future extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new api This requires adding API to an existing portal portal: background Background portal and background monitoring
Projects
Status: Triaged
Development

Successfully merging this pull request may close these issues.

5 participants