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.

Sign up for GitHub

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

Jump to bottom

glib: Add helper functions to spawn a non-Send future on the curren… #902

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

glib: Add helper functions to spawn a non-Send future on the curren… #902

wants to merge 1 commit into from

Conversation

sdroege
Copy link
Member

@sdroege sdroege commented Jan 18, 2023

…t thread-default MainContext

CC @elmarco @pbor @A6GibKm

sdroege
sdroege commented Jan 18, 2023
View reviewed changes
glib/src/lib.rs
@@ -199,7 +199,7 @@ pub use self::bridged_logging::{rust_log_handler, GlibLogger, GlibLoggerDomain,
pub mod subclass;

mod main_context_futures;
pub use main_context_futures::{JoinError, JoinHandle};
pub use main_context_futures::{spawn, spawn_with_priority, JoinError, JoinHandle};
Copy link
Member Author

Choose a reason for hiding this comment

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

The naming of the functions is a bit suboptimal as there's also this set of functions: https://gtk-rs.org/gtk-rs-core/git/docs/glib/fn.spawn_async.html

Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

spawn_main_context?

Copy link
Member

Choose a reason for hiding this comment

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

Or directly make these two functions methods of MainContext and add doc aliases on them.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are already methods with that name on MainContext. The goal here is to have a function that gets the correct main context instance and then calls the method on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

spawn_on_thread_default_main_context() could work of course, but that's almost as long as the implementation of the function :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe could move this into a glib::futures module, and also potentially move some other things in there. Not sure.

@sdroege
Copy link
Member Author

sdroege commented Jan 18, 2023

This is apparently not too useful because g_application_run() and g_main_loop_run() are not pushing their main context as the thread default one.

@sdroege sdroege added this to the 0.17.0 milestone Jan 18, 2023
@sdroege sdroege mentioned this pull request Jan 19, 2023
Open
@sdroege
Copy link
Member Author

sdroege commented Jan 19, 2023

Maybe something like this. It would take the thread-default main context if any, otherwise the global default. And then make sure that the current thread actually owns it.

@sdroege
Copy link
Member Author

sdroege commented Jan 27, 2023

I guess there is not really any interest in having such a function? Let's close this then?

@BrainBlasted
Copy link
Contributor

BrainBlasted commented Jan 27, 2023 •
edited
Loading

Huh? This would definitely be helpful. Apps currently use a macro for this.

@sdroege
Copy link
Member Author

sdroege commented Jan 27, 2023

I got no feedback at all so I assumed nobody actually needed it :) As it's apparently useful to you, do you have suggestions about the name and where to put it? Where/how would you expect it?

@A6GibKm
Copy link
Contributor

A6GibKm commented Jan 27, 2023 •
edited
Loading

Huh? This would definitely be helpful. Apps currently use a macro for this.

Note that they do MainContext::default() rather than MainContext::thread_default(), that has me a bit confused, if they are equivalent then sure this would be rather helpful.

@sdroege
Copy link
Member Author

sdroege commented Jan 27, 2023

Note that they do MainContext::default() rather than MainContext::thread_default(), that has me a bit confused, if they are equivalent then sure this would be rather helpful.

For an application that didn't configure a custom thread default main context this would end up the same.

@sdroege
Copy link
Member Author

sdroege commented Jan 28, 2023

Conclusion seems to be to make a new glib::futures module and add this as glib::futures::spawn(), and I guess move various other function in there too.

@sdroege sdroege mentioned this pull request Jan 31, 2023
Open
@sdroege sdroege modified the milestones: 0.17.0, 0.18.0 Feb 8, 2023
@BrainBlasted
Copy link
Contributor

Has anyone started working in that direction?

@sdroege
Copy link
Member Author

sdroege commented May 31, 2023

I'm not aware of anybody having started such work

@bilelmoussaoui bilelmoussaoui modified the milestones: 0.18.0, 0.20 May 31, 2024
@sdroege sdroege modified the milestones: 0.20, 0.21 Jul 10, 2024
bilelmoussaoui
bilelmoussaoui reviewed Nov 19, 2024
View reviewed changes
glib/src/main_context_futures.rs
///
/// Panics if there is no thread-default main context on this thread and if this thread
/// does not own the global default main context.
pub fn spawn_with_priority<R: 'static, F: Future<Output = R> + 'static>(
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be named spawn_local_with_priority?

Copy link
Member

Choose a reason for hiding this comment

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

same for above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bilelmoussaoui bilelmoussaoui bilelmoussaoui left review comments

@GuillaumeGomez GuillaumeGomez Awaiting requested review from GuillaumeGomez

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.21
Development

Successfully merging this pull request may close these issues.
5 participants
@sdroege @BrainBlasted @A6GibKm @GuillaumeGomez @bilelmoussaoui