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

Windows & X11: Window::set_resizable #558

Merged
merged 9 commits into from
Jun 11, 2018
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
- On X11, the `Moved` event is no longer sent when the window is resized without changing position.
- `MouseCursor` and `CursorState` now implement `Default`.
- `WindowBuilder::with_resizable` implemented for Windows, X11, and macOS.
- `Window::set_resizable` implemented for MacOS, X11, and Windows.
Copy link
Member

Choose a reason for hiding this comment

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

The correct stylization is "macOS", and it also reads a little easier if you use the same ordering as in the previous line.

- On X11, if the monitor's width or height in millimeters is reported as 0, the DPI is now 1.0 instead of +inf.
- On X11, the environment variable `WINIT_HIDPI_FACTOR` has been added for overriding DPI factor.
- On X11, enabling transparency no longer causes the window contents to flicker when resizing.
Expand Down
22 changes: 0 additions & 22 deletions examples/no_resize.rs

This file was deleted.

36 changes: 36 additions & 0 deletions examples/resizable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
extern crate winit;

fn main() {
let mut events_loop = winit::EventsLoop::new();

let window = winit::WindowBuilder::new()
.with_title("Hit space to toggle resizability.")
.with_dimensions(400, 200)
.with_resizable(false)
.build(&events_loop)
.unwrap();

let mut resizable = false;

events_loop.run_forever(|event| match event {
winit::Event::WindowEvent { event, .. } => match event {
winit::WindowEvent::CloseRequested => winit::ControlFlow::Break,
winit::WindowEvent::KeyboardInput {
input:
winit::KeyboardInput {
virtual_keycode: Some(winit::VirtualKeyCode::Space),
state: winit::ElementState::Released,
..
},
..
} => {
resizable = !resizable;
println!("Resizable: {}", resizable);
window.set_resizable(resizable);
winit::ControlFlow::Continue
}
_ => winit::ControlFlow::Continue,
},
_ => winit::ControlFlow::Continue,
Copy link
Member

Choose a reason for hiding this comment

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

As an official winit example™, it would be good if you refactored this to not repeat winit::ControlFlow::Continue, i.e. https://github.com/tomaka/winit/blob/master/examples/handling_close.rs

});
}
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ pub struct WindowAttributes {
/// The default is `None`.
pub max_dimensions: Option<(u32, u32)>,

/// [Windows & X11 only] Whether the window is resizable or not
/// [Windows, MacOS, and X11 only] Whether the window is resizable or not
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you followed the precedent set by [iOS only], but you should know two things about that:

  1. The [iOS only] doc comment isn't accurate anymore...
  2. That was added a long time ago, and back before winit had someone barking about style, so it's not necessarily something you should replicate. Sticking to the conventions established in other areas of the documentation is probably better.

///
/// The default is `true`.
pub resizable: bool,
Expand Down
9 changes: 9 additions & 0 deletions src/platform/linux/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,15 @@ impl Window {
&Window::Wayland(ref w) => w.set_max_dimensions(dimensions)
}
}

#[inline]
pub fn set_resizable(&self, resizable: bool) {
match self {
&Window::X(ref w) => w.set_resizable(resizable),
// &Window::Wayland(ref w) => w.set_resizable(resizable),
_ => {}
Copy link
Member

Choose a reason for hiding this comment

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

We tend to favor _ => (), but more importantly, it's more appropriate to stub this as unimplemented!(), by decree of tomaka.

The idea is that it makes people more likely to fix it... in my experience, all it does is make people more likely to yell at us, though that does make me more likely to fix it.

Copy link
Contributor

@elinorbgr elinorbgr Jun 8, 2018

Choose a reason for hiding this comment

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

I've released version 0.2.2 of smithay-client-toolkit which adds a set_resizable method to its Window type, that does what we need for wayland support. So, fixing should be pretty trivial, I can do this once this is merged, as soon as I secure the 5 minutes it'll take.

}
}

#[inline]
pub fn set_cursor(&self, cursor: MouseCursor) {
Expand Down
20 changes: 20 additions & 0 deletions src/platform/linux/x11/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,26 @@ impl UnownedWindow {
}.expect("Failed to call XSetWMNormalHints");
}

pub fn set_resizable(&self, resizable: bool) {
unsafe {
self.update_normal_hints(|size_hints| {
if resizable {
(*size_hints).flags &= !ffi::PMinSize;
Copy link
Member

Choose a reason for hiding this comment

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

You can cut the number of flag changes in half in this function.

(*size_hints).flags &= !ffi::PMaxSize;
} else {
(*size_hints).flags |= ffi::PMinSize;
(*size_hints).flags |= ffi::PMaxSize;
if let Some((width, height)) = self.get_inner_size() {
(*size_hints).min_width = width as c_int;
(*size_hints).min_height = height as c_int;
(*size_hints).max_width = width as c_int;
(*size_hints).max_height = height as c_int;
}
}
})
}.expect("Failed to call XSetWMNormalHints");
}

#[inline]
pub fn get_xlib_display(&self) -> *mut c_void {
self.xconn.display as _
Expand Down
27 changes: 27 additions & 0 deletions src/platform/windows/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,33 @@ impl Window {
}
}
}

/// See the docs in the crate root file.
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to add this comment, since #548 completely removes them... they don't seem to be at all helpful, especially since these are doc comments on private methods.

More importantly, what happens when you use this method while the window is fullscreen?

#[inline]
pub fn set_resizable(&self, resizable: bool) {
if let Ok(mut window_state) = self.window_state.lock() {
if window_state.attributes.resizable == resizable {
return;
}
let window = self.window.clone();
let mut style = unsafe {
winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE)
};
if resizable {
style |= winuser::WS_SIZEBOX as i32;
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer you use the winapi LONG type instead of i32.

} else {
style &= !winuser::WS_SIZEBOX as i32;
Copy link
Member

Choose a reason for hiding this comment

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

What's the precedence here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The precedence for removing the WS_SIZEBOX when setting resizable to false? I guess I'm not 100% sure what you are referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh lol. The precedence is LONG.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just wanted to make sure it was zero-extending before flipping the bits. Or rather, I wanted to make sure you thought about that.

}
unsafe {
winuser::SetWindowLongW(
window.0,
winuser::GWL_STYLE,
style as _,
);
};
window_state.attributes.resizable = resizable;
}
}

// TODO: remove
pub fn platform_display(&self) -> *mut ::libc::c_void {
Expand Down
10 changes: 10 additions & 0 deletions src/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,16 @@ impl Window {
pub fn set_max_dimensions(&self, dimensions: Option<(u32, u32)>) {
self.window.set_max_dimensions(dimensions)
}

/// Sets whether the window is resizable or not.
///
/// ## Platform-specific
///
/// This only has an effect on Windows, X11, and MacOS.
#[inline]
pub fn set_resizable(&self, resizable: bool) {
self.window.set_resizable(resizable)
}

/// DEPRECATED. Gets the native platform specific display for this window.
/// This is typically only required when integrating with
Expand Down