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

Terminate app run loop on Windows when all windows have closed. #763

Merged
merged 3 commits into from
Apr 27, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions druid-shell/examples/invalidate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ impl WinHandler for InvalidateTest {
}

fn main() {
let mut app = Application::new(None);
let mut builder = WindowBuilder::new();
let app = Application::new();
let mut builder = WindowBuilder::new(app.clone());
let inv_test = InvalidateTest {
size: Default::default(),
handle: Default::default(),
Expand All @@ -98,5 +98,5 @@ fn main() {

let window = builder.build().unwrap();
window.show();
app.run();
app.run(None);
}
8 changes: 4 additions & 4 deletions druid-shell/examples/perftest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl WinHandler for PerfTest {
}

fn destroy(&mut self) {
Application::quit()
Application::global().quit()
}

fn as_any(&mut self) -> &mut dyn Any {
Expand All @@ -116,8 +116,8 @@ impl WinHandler for PerfTest {
}

fn main() {
let mut app = Application::new(None);
let mut builder = WindowBuilder::new();
let app = Application::new();
let mut builder = WindowBuilder::new(app.clone());
let perf_test = PerfTest {
size: Default::default(),
handle: Default::default(),
Expand All @@ -130,5 +130,5 @@ fn main() {
let window = builder.build().unwrap();
window.show();

app.run();
app.run(None);
}
10 changes: 5 additions & 5 deletions druid-shell/examples/shello.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl WinHandler for HelloState {
match id {
0x100 => {
self.handle.close();
Application::quit();
Application::global().quit()
}
0x101 => {
let options = FileDialogOptions::new().show_hidden().allowed_types(vec![
Expand Down Expand Up @@ -101,7 +101,7 @@ impl WinHandler for HelloState {
}

fn destroy(&mut self) {
Application::quit()
Application::global().quit()
}

fn as_any(&mut self) -> &mut dyn Any {
Expand Down Expand Up @@ -129,14 +129,14 @@ fn main() {
menubar.add_dropdown(Menu::new(), "Application", true);
menubar.add_dropdown(file_menu, "&File", true);

let mut app = Application::new(None);
let mut builder = WindowBuilder::new();
let app = Application::new();
let mut builder = WindowBuilder::new(app.clone());
builder.set_handler(Box::new(HelloState::default()));
builder.set_title("Hello example");
builder.set_menu(menubar);

let window = builder.build().unwrap();
window.show();

app.run();
app.run(None);
}
145 changes: 127 additions & 18 deletions druid-shell/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,13 @@

//! The top-level application type.

use std::cell::RefCell;
use std::rc::Rc;
use std::sync::atomic::{AtomicBool, Ordering};

use crate::clipboard::Clipboard;
use crate::platform::application as platform;
use crate::util;

/// A top-level handler that is not associated with any window.
///
Expand All @@ -36,44 +41,148 @@ pub trait AppHandler {
fn command(&mut self, id: u32) {}
}

//TODO: we may want to make the user create an instance of this (Application::global()?)
//but for now I'd like to keep changes minimal.
/// The top level application object.
pub struct Application(platform::Application);
///
/// This can be thought of as a reference and it can be safely cloned.
#[derive(Clone)]
pub struct Application {
state: Rc<RefCell<State>>,
pub(crate) platform_app: platform::Application,
}

/// Platform-independent `Application` state.
struct State {
running: bool,
}

/// Used to ensure only one Application instance is ever created.
static APPLICATION_CREATED: AtomicBool = AtomicBool::new(false);

thread_local! {
/// A reference object to the current `Application`, if any.
static GLOBAL_APP: RefCell<Option<Application>> = RefCell::new(None);
}

impl Application {
pub fn new(handler: Option<Box<dyn AppHandler>>) -> Application {
Application(platform::Application::new(handler))
/// Create a new `Application`.
///
/// # Panics
///
/// Panics if an `Application` has already been created.
///
/// This may change in the future. See [druid#771] for discussion.
///
/// [druid#771]: https://github.com/xi-editor/druid/issues/771
pub fn new() -> Application {
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to think a bit about what the API should look like in this new world.

Some basic questions:

  • should we require explicit initialization, or should we just lazily initialize in global?
  • if we do require explicit initialization, should that be a method like new that returns the application, or should it be a method like init that returns Result<(), SomeError>?, and then the actual application is always retrieved via global?
  • Should Application be passed by value, or by reference? Having global return &'static Application' (and having Application: !Clone` perhaps better expresses the semantics of the program, where there is a single shared application).

Copy link
Collaborator

Choose a reason for hiding this comment

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

About your second bullet (In particular with gtk and perhaps macOS)

I think returning a Result might be good for cases where we are dealing with a remote application that was previously launched. E.g. In the MacOS case there is NSRunningApplication, and in GTK the application is less functional, cannot display windows, but can send signals such as to open a new window to the remote application.

Being able to differentiate these via Result<ApplicationInitializationStatus, SomeError> or such might work for exposing this behavior on both platforms?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll play around a bit with the &'static Application idea and see if and how well I can get it working.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I ran into a dead end with &'static Application but opened a thread on zulip for it.

Regarding initialization, I favor explicit because it gives clear control over the timing of when this significant work takes place, and if we plan on returning errors here in the future instead of panicking - then also a single place to handle those errors. The global function would always be fast and there wouldn't be a need to reason about whether one should check for initialization errors at every call location.

Naming wise, new vs init, I think new is better if we don't want to start painting ourselves into a strategy corner in terms of #771. That's because I think it makes more sense to have new if this function is going to be called more than once.

Return value wise I think changing it to fn new() -> Result<Application, Error> would be a good upgrade. This would allow the calling app to choose whether to panic or to do something else. Right now druid-shell is a bit too panic friendly in its initialization code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a new commit containing the Result<Application, Error> change.

if APPLICATION_CREATED.compare_and_swap(false, true, Ordering::AcqRel) {
panic!("The Application instance has already been created.");
}
util::claim_main_thread();
let state = Rc::new(RefCell::new(State { running: false }));
let app = Application {
state,
platform_app: platform::Application::new(),
};
GLOBAL_APP.with(|global_app| {
*global_app.borrow_mut() = Some(app.clone());
});
app
}

/// Get the current globally active `Application`.
///
/// A globally active `Application` exists
/// after [`new`] is called and until [`run`] returns.
///
/// # Panics
///
/// Panics if there is no globally active `Application`.
/// For a non-panicking function use [`try_global`].
///
/// This function will also panic if called from a non-main thread.
///
/// [`new`]: #method.new
/// [`run`]: #method.run
/// [`try_global`]: #method.try_global
#[inline]
pub fn global() -> Application {
// Main thread assertion takes place in try_global()
Application::try_global().expect("There is no globally active Application")
}

/// Get the current globally active `Application`.
///
/// A globally active `Application` exists
/// after [`new`] is called and until [`run`] returns.
///
/// # Panics
///
/// Panics if called from a non-main thread.
///
/// [`new`]: #method.new
/// [`run`]: #method.run
pub fn try_global() -> Option<Application> {
util::assert_main_thread();
GLOBAL_APP.with(|global_app| global_app.borrow().clone())
}

/// Start the runloop.
/// Start the `Application` runloop.
///
/// The provided `handler` will be used to inform of events.
///
/// This will consume the `Application` and block the current thread
/// until the `Application` has finished executing.
///
/// # Panics
///
/// This will block the current thread until the program has finished executing.
pub fn run(&mut self) {
self.0.run()
/// Panics if the `Application` is already running.
pub fn run(self, handler: Option<Box<dyn AppHandler>>) {
// Make sure this application hasn't run() yet.
if let Ok(mut state) = self.state.try_borrow_mut() {
if state.running {
panic!("Application is already running");
}
state.running = true;
} else {
panic!("Application state already borrowed");
}

// Run the platform application
self.platform_app.run(handler);

// This application is no longer active, so clear the global reference
GLOBAL_APP.with(|global_app| {
*global_app.borrow_mut() = None;
});
// .. and release the main thread
util::release_main_thread();
}

/// Terminate the application.
pub fn quit() {
platform::Application::quit()
/// Quit the `Application`.
///
/// This will cause [`run`] to return control back to the calling function.
///
/// [`run`]: #method.run
pub fn quit(&self) {
self.platform_app.quit()
}

// TODO: do these two go in some kind of PlatformExt trait?
/// Hide the application this window belongs to. (cmd+H)
pub fn hide() {
pub fn hide(&self) {
#[cfg(target_os = "macos")]
platform::Application::hide()
self.platform_app.hide()
}

/// Hide all other applications. (cmd+opt+H)
pub fn hide_others() {
pub fn hide_others(&self) {
#[cfg(target_os = "macos")]
platform::Application::hide_others()
self.platform_app.hide_others()
}

/// Returns a handle to the system clipboard.
pub fn clipboard() -> Clipboard {
platform::Application::clipboard().into()
pub fn clipboard(&self) -> Clipboard {
self.platform_app.clipboard().into()
}

/// Returns the current locale string.
Expand Down
6 changes: 3 additions & 3 deletions druid-shell/src/clipboard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub use crate::platform::clipboard as platform;
/// ```no_run
/// use druid_shell::{Application, Clipboard};
///
/// let mut clipboard = Application::clipboard();
/// let mut clipboard = Application::global().clipboard();
/// clipboard.put_string("watch it there pal");
/// if let Some(contents) = clipboard.get_string() {
/// assert_eq!("what it there pal", contents.as_str());
Expand All @@ -83,7 +83,7 @@ pub use crate::platform::clipboard as platform;
/// ```no_run
/// use druid_shell::{Application, Clipboard, ClipboardFormat};
///
/// let mut clipboard = Application::clipboard();
/// let mut clipboard = Application::global().clipboard();
///
/// let custom_type_id = "io.xieditor.path-clipboard-type";
///
Expand All @@ -104,7 +104,7 @@ pub use crate::platform::clipboard as platform;
/// ```no_run
/// use druid_shell::{Application, Clipboard, ClipboardFormat};
///
/// let clipboard = Application::clipboard();
/// let clipboard = Application::global().clipboard();
///
/// let custom_type_id = "io.xieditor.path-clipboard-type";
/// let supported_types = &[custom_type_id, ClipboardFormat::SVG, ClipboardFormat::PDF];
Expand Down
1 change: 1 addition & 0 deletions druid-shell/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ mod keycodes;
mod menu;
mod mouse;
mod platform;
mod util;
mod window;

pub use application::{AppHandler, Application};
Expand Down
11 changes: 6 additions & 5 deletions druid-shell/src/platform/gtk/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@ thread_local!(
static GTK_APPLICATION: RefCell<Option<GtkApplication>> = RefCell::new(None);
);

pub struct Application;
#[derive(Clone)]
pub(crate) struct Application;

impl Application {
pub fn new(_handler: Option<Box<dyn AppHandler>>) -> Application {
pub fn new() -> Application {
// TODO: we should give control over the application ID to the user
let application = GtkApplication::new(
Some("com.github.xi-editor.druid"),
Expand All @@ -59,7 +60,7 @@ impl Application {
Application
}

pub fn run(&mut self) {
pub fn run(self, _handler: Option<Box<dyn AppHandler>>) {
util::assert_main_thread();

// TODO: should we pass the command line arguments?
Expand All @@ -71,7 +72,7 @@ impl Application {
});
}

pub fn quit() {
pub fn quit(&self) {
util::assert_main_thread();
with_application(|app| {
match app.get_active_window() {
Expand All @@ -86,7 +87,7 @@ impl Application {
});
}

pub fn clipboard() -> Clipboard {
pub fn clipboard(&self) -> Clipboard {
Clipboard
}

Expand Down
6 changes: 3 additions & 3 deletions druid-shell/src/platform/gtk/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use gtk::{AccelGroup, ApplicationWindow};
use crate::kurbo::{Point, Rect, Size, Vec2};
use crate::piet::{Piet, RenderContext};

use super::application::with_application;
use super::application::{with_application, Application};
use super::dialog;
use super::menu::Menu;
use super::util::assert_main_thread;
Expand Down Expand Up @@ -81,7 +81,7 @@ pub struct WindowHandle {
}

/// Builder abstraction for creating new windows
pub struct WindowBuilder {
pub(crate) struct WindowBuilder {
handler: Option<Box<dyn WinHandler>>,
title: String,
menu: Option<Menu>,
Expand Down Expand Up @@ -111,7 +111,7 @@ pub(crate) struct WindowState {
}

impl WindowBuilder {
pub fn new() -> WindowBuilder {
pub fn new(_app: Application) -> WindowBuilder {
WindowBuilder {
handler: None,
title: String::new(),
Expand Down
Loading