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

jsparber · 2023-09-08T13:25:16Z

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.

hfiguiere · 2023-11-27T03:29:21Z

src/background.c

@@ -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 };


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 · 2023-12-06T14:44:12Z

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.

razzeee · 2023-12-06T14:47:23Z

src/background.c


  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);


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

razzeee · 2023-12-06T14:48:34Z

src/background.c

+                      GVariant *options,
+                      GError **error)
+{
+	// TODO: validate


Marking this todo with a comment

razzeee · 2023-12-06T14:50:59Z

data/org.freedesktop.impl.portal.Background.xml

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


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 · 2023-12-06T14:53:21Z

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 · 2023-12-06T15:06:01Z

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.

hfiguiere · 2023-12-20T04:41:56Z

src/background.c

@@ -873,11 +907,23 @@ validate_commandline (const char *key,

  return TRUE;
 }
+static gboolean


`nit

Suggested change

static gboolean

static gboolean

hfiguiere · 2023-12-20T04:42:52Z

src/xdp-utils.c

        g_ptr_array_add (args, g_shell_quote (app_info->id));
+      } else {
+        g_ptr_array_add (args, g_strdup (app_info->id));
+      }


the indentation style doesn't match

hfiguiere · 2023-12-20T04:44:04Z

src/background.c

    }
-  else if (!xdp_dbus_impl_background_call_enable_autostart_sync (background_impl,
+  else {


the indentaton style doesn't match

calebccff · 2024-02-21T19:11:49Z

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:

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).
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 · 2024-03-11T12:30:13Z

@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 · 2024-06-17T12:04:55Z

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.

jsparber added 2 commits September 6, 2023 09:38

xdg-utils: Properly quote flatpak command

b86edec

`xdp_app_info_rewrite_commandline` has to respect `quote_escape` also when no commandline is given.

background: Add ability to specify timers to start app

9fd4bdc

GeorgesStavracas added new api This requires adding API to an existing portal portal: background Background portal and background monitoring labels Oct 3, 2023

GeorgesStavracas self-requested a review October 3, 2023 15:49

hfiguiere reviewed Nov 27, 2023

View reviewed changes

razzeee reviewed Dec 6, 2023

View reviewed changes

src/background.c

GVariant *options,

GError **error)

{

// TODO: validate

Copy link

Contributor

razzeee Dec 6, 2023

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

razzeee reviewed Dec 6, 2023

View reviewed changes

sonnyp marked this pull request as draft December 19, 2023 16:55

hfiguiere reviewed Dec 20, 2023

View reviewed changes

Mikenux mentioned this pull request Mar 8, 2024

Notifications Portal v2 proposal #983

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jsparber commented Sep 8, 2023

hfiguiere Nov 27, 2023 •

edited

Loading

razzeee commented Dec 6, 2023

razzeee Dec 6, 2023

razzeee Dec 6, 2023

razzeee Dec 6, 2023

razzeee commented Dec 6, 2023

jsparber commented Dec 6, 2023

hfiguiere Dec 20, 2023

hfiguiere Dec 20, 2023

hfiguiere Dec 20, 2023

calebccff commented Feb 21, 2024

jsparber commented Mar 11, 2024

jsparber commented Jun 17, 2024

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

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

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

Are you sure you want to change the base?

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

Conversation

jsparber commented Sep 8, 2023

hfiguiere Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

razzeee commented Dec 6, 2023

razzeee Dec 6, 2023

Choose a reason for hiding this comment

razzeee Dec 6, 2023

Choose a reason for hiding this comment

razzeee Dec 6, 2023

Choose a reason for hiding this comment

razzeee commented Dec 6, 2023

jsparber commented Dec 6, 2023

hfiguiere Dec 20, 2023

Choose a reason for hiding this comment

hfiguiere Dec 20, 2023

Choose a reason for hiding this comment

hfiguiere Dec 20, 2023

Choose a reason for hiding this comment

calebccff commented Feb 21, 2024

jsparber commented Mar 11, 2024

jsparber commented Jun 17, 2024

hfiguiere Nov 27, 2023 •

edited

Loading