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

Implemented resizing for non-decorated winit windows #5026

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

LucFauvel
Copy link

Fixes #5023

Heavily inspired by these changes in Iced: iced-rs/iced#1542

Completely open to any changes, tested this on windows and works great but don't have any other environments at my disposal for testing yet.

@CLAassistant
Copy link

CLAassistant commented Apr 7, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thank you very much for this patch.
Sorry for answering so late, but i was busy traveling during the week.

I'm just worried that this interact badly with eh contents of the window if there are some buttons or touch area near the edges.

Y::Default
};

Some(match xdir {
Copy link
Member

Choose a reason for hiding this comment

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

for this pattern I'd have done a match (xdir, ydir) {
But this is fine too.

Copy link
Author

Choose a reason for hiding this comment

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

Funny you mention this because I wasn't sure which pattern was better for readability after trying both. I'll commit this change and you can let me know which one you like better once you read it.

current_direction: Option<ResizeDirection>,
) -> Option<ResizeDirection> {
if !window.is_decorated() {
let location = get_resize_direction(window.inner_size(), position, 8_f64);
Copy link
Member

Choose a reason for hiding this comment

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

8 seems like a huge border.
I think it should be 1 or 2 logical pixel maximum. Don't you think?

Copy link
Author

Choose a reason for hiding this comment

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

So I tried a few numbers and 3_f64 is the smallest I could go before I felt it started to get hard to hit it properly. Is that alright with you?

internal/backends/winit/event_loop.rs Outdated Show resolved Hide resolved
@LucFauvel
Copy link
Author

LucFauvel commented Apr 13, 2024

@ogoffart

I'm just worried that this interact badly with eh contents of the window if there are some buttons or touch area near the edges.

Yeah I experimented with this and if, lets say, a toucharea is within the border, the cursor flickers as its being switched between the resize one and the toucharea one, but the resizing actually still works. Not sure how to fix it without breaking anything.

@LucFauvel
Copy link
Author

I've added && window._is_resiable() to the handle_cursor_move_for_resize condition to prevent resizing if the user specifically disabled it.

@tronical
Copy link
Member

I think this is a good feature to have, in general. More specifically though, the size of the resize border is something that should be configurable.

Imagine a custom UI of a media player (like Winamp back in the day), where the UI design conveys to the user where the borders are that can be used for resizing. This could be implemented visually through a nine patch image in Slint, and behaviourally this should be a property then on the Window. That's also the way of making it opt-in. Imagine a resize-border property that defaults to 0px. If it's non-zero, your code should become active.

What do you think?

@LucFauvel
Copy link
Author

@tronical I agree that it should be configurable and thought about it in when first making this PR, I just wasn't familiar enough with the codebase to start adding options yet. Now that I am a bit more familiar I should be able to add it later today.

@LucFauvel
Copy link
Author

LucFauvel commented Apr 17, 2024

@tronical @ogoffart As discussed above, I've added a property called "resize-border" to Window which configures the border. I've only done it with Winit since I don't have a working Qt setup atm.

Also I had some concerns while writing this, specifically this line that I've added: runtime_window.window_item().map_or(0_f64, |w| w.as_pin_ref().resize_border().into())

This creates a pinned refrence on every cursor moved event, is this a heavy operation that should be stored in the event loop state instead? Or is it light enough to not add too much additional memory pressure?

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks for the change.
Sorry for answering so late.

Since the property is a size in pixel, it should be a length value, so that its actual physical value depends on the scale factor.

internal/compiler/builtins.slint Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-frame: true disables window resizing on Windows
4 participants