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

X11: Flatten window model #536

Merged
merged 1 commit into from
May 29, 2018

Conversation

francesca64
Copy link
Member

@francesca64 francesca64 commented May 27, 2018

X11 HiDPI will be finished soon, I promise! I'm actually very nearly done with it, but some internal design issues in the X11 backend were actively complicating my work. So, this is yet another refactor PR.

Before this PR:

  • Window owns a Window2, which contains all the actual methods and additionally owns an XWindow.
  • Window2 owns SharedState, which is shared with EventsLoop via RefCell<HashMap<WindowId, Weak<Mutex<window::SharedState>>>>.
  • EventsLoop shares Arc<Mutex<HashMap<WindowId, WindowData>>> with Window, but not with Window2. Window has it so it can remove itself from it in drop, and EventsLoop uses it to store various state related to the window. Nested in WindowData is WindowConfig.
  • There's no way to call methods on Window2 from EventsLoop.

So, to recap, there are 3 window types (Window, Window2, and XWindow) and 3 window state types (WindowData, WindowConfig, and SharedState). Chances are, what you want to access isn't going to be where you need it to be. For instance, that's why I didn't work on #242 for the 0.15.0 release, since it would've involved some tedious state coordination and function duplication.

After this PR:

  • XWindow has been flattened into Window2, and Window2 has been renamed to UnownedWindow. This name represents the fact that UnownedWindow is just a bundle of state that's used for calling methods, and not a managed resource.
  • Window owns an UnownedWindow, and cleans up after it.
  • WindowConfig has been flattened into WindowData, and WindowData has been flattened into SharedState!
  • EventsLoop has access to every UnownedWindow via RefCell<HashMap<WindowId, Weak<UnownedWindow>>>. This makes it possible to call UnownedWindow methods.
  • EventsLoop::with_window makes it much more ergonomic to operate on windows.

Before:

self.shared_state
    .borrow()
    .get(&WindowId(xev.window))
    .map(|window_state| {
        if let Some(window_state) = window_state.upgrade() {
            (*window_state.lock()).frame_extents.take();
        }
    });

After:

self.with_window(xev.window, |window| {
    window.invalidate_cached_frame_extents();
});

with_window can also return any Option<T> you want it to, so it generally can do what you need it to without you having to think about it.

There are still some gotchas, i.e.

let window_rect = self.with_window(xev.window, |window| {
    let mut shared_state_lock = window.shared_state.lock();
    // Various methods try to lock `SharedState`,
    // so will deadlock if you already have a lock.
    window.get_rect()
});

Even with that proviso, this design is much easier to work with than the existing one.

@francesca64 francesca64 merged commit 4372f6f into rust-windowing:master May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

1 participant