gtk-rs-core

gtk-rs-core copied to clipboard

Followup to #1201

Allow to set custom priority for the spawned futures.

IMHO this is not an idiomatic way to handle the problem. We are polluting the library with a lot of combinations do_x, do_x_with_priority do_x_local, do_x_with_priority_local...

The same problems exists for timeouts, and other glib sources.

In #1033 I was trying to build a futures module where you can use a builder pattern to reduce the number of spawn combinations. For example, that PR introduced a SpawnOption builder...

    SpawnOptions::new()
         .context(ctx)
         .priority(Priority::HIGH)  
         .spawn_local(async move {});

Note: the context is optional, the priority is optional. That's 1 API to handle all the possible spawn combinations. That also resembles the API of OpenOptions in the rust std library.

I like that approach @ranfdev

The Builder is nice yes, but every app will still end up with convenient wrappers around it. It won't cut down on the boilerplate required compared to glib::spawn* write now.

Now making wrappers for these on the application side is harmless for the most part. But if that's the case I'd say we'd remove #1201 as well or get the wrappers on the bindings side.

Yes I think these should be replaced by the builder too.

I would keep the functions #1201 under the futures scope, eg: glib::futures::spawn_local.

Only because spawn_local is used 10x more than spawn_local_with_priority, so I think it's nice having a shorthand for the common case.

Also, you can use the functions in a module to shorten the call even more, but you cannot do that with a Builder method.

// Valid
use glib::futures::spawn_local;
spawn_local(async {});

// No way to shorten the builder as easily
SpawnOptions::new().local().spawn(async {});

Only because spawn_local is used 10x more than spawn_local_with_priority, so I think it's nice having a shorthand for the common case.

Most applications that use ::spawn actually wanted/should have been using a spawn with default_idle, as default has higher priority than GTK's rendering machinery and can lead to missing frames.

I expect 9/10 times this is the case.

Ah, I didn't expect that. Can we make every spawn_local use the priority default_idle by default then?

For sure I've spawned many futures with spawn_local in the past, without thinking they had a higher priority than GTK's rendering. I'm confident a lot of people are making the same mistake.

Ah, I didn't expect that. Can we make every spawn_local use the priority default_idle by default then?

Yea, so I just opened an MR for loupe which you can use as reference.

https://gitlab.gnome.org/GNOME/loupe/-/merge_requests/304

That's also a more general problem then. g_timeout_add() for example also uses G_PRIORITY_DEFAULT, as does g_main_context_invoke(), and basically every GSource in GLib except for the idle one.

If that's not suitable for GTK applications then something will have to change in GTK.

Ah, I didn't expect that. Can we make every spawn_local use the priority default_idle by default then?

Like @sdroege said ideally that would be changed on the GTK side. At the very least, we should double-check with a GTK person that default_idle is even a good default.

It doesn't even seem like a good default for GTK. It depends on how you want your future to be ordered in relation to the drawing operations. It probably makes sense to export the GDK priority as a constant though.