Allow changing resize increments after window creation #2411

necauqua · 2022-08-07T11:35:39Z

Pretty basic "I hope it works" implementation, only tested on X11, can test on a mac in a few days.

Once this is reviewed->merged->released I hope to finally make Alacritty not suck, heh

~~The branch is based on the #2410 because I kept moving the test window to the other screen, I'll rebase once it's merged,~~ done

~~Currently this is a draft to run your CI and to get reviews.~~ should be ready

Closes #2020

Tested on all platforms changed
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

necauqua · 2022-08-08T14:49:54Z

Still marked as a draft because I did not test this on a mac yet, if you have a mac and are willing to test this you are welcome

kchibisov · 2022-08-08T15:06:12Z

I could test macOs, but not soon. Maybe @madsmtm could check.

examples/window_resize_increments.rs

src/platform_impl/android/mod.rs

src/platform_impl/linux/x11/util/hint.rs

src/platform_impl/web/window.rs

src/platform_impl/windows/window.rs

src/window.rs

madsmtm

Thanks for the PR, looks good and seems to work fine barring a few things

src/platform_impl/macos/window.rs

examples/window_resize_increments.rs

src/platform_impl/macos/window.rs

necauqua · 2022-08-13T23:02:49Z

Ok so I guess @madsmtm tested this on a mac so I'm undrafting this.
A couple of remaining minor reviews waiting for clarification after my comment and contentResizeIncrements/resizeIncrements debate also waits for a quorum agreement I guess

madsmtm

Works flawlessly on macOS now, thanks!

src/window.rs

src/platform_impl/macos/window.rs

necauqua · 2022-08-15T23:04:43Z

Hey @kchibisov have you had a chance to see my reply to your comments about logging on unsupported platforms?

Do you insist on adding those logs or does my reasoning make sense to keep it as I did it initially

kchibisov · 2022-08-23T19:04:04Z

@necauqua the thing is that log was added after some point in time, so the platforms inherited // not supported comments and there wasn't anything to change that. In general we want to log, but for now we can leave it as is for consistency sake.

src/window.rs

kchibisov · 2022-08-23T19:07:54Z

src/platform_impl/linux/x11/window.rs

@@ -386,7 +386,7 @@ impl UnownedWindow {
                normal_hints.set_min_size(min_inner_size.map(Into::into));
                normal_hints.set_max_size(max_inner_size.map(Into::into));
                normal_hints.set_resize_increments(
-                    pl_attribs
+                    window_attrs
                        .resize_increments
                        .map(|size| size.to_physical::<u32>(scale_factor).into()),


There's no need for map given that function handles that or the scaling is wrong when calling the set_resize_increments here? Could you add a comment about that at least? I do understand that the code was like that before, but it would be nice to ensure that and remove extra convertion, since it looks confusing.

You might be confused by the two lines above which look like normal_hints.set_max_size(max_inner_size.map(Into::into)); - but in fact I modelled resize_incremets exactly like min/max_inner_size.

In those lines above e.g. the min_inner_size is is defined as let mut min_inner_size = window_attrs.min_inner_size.map(|size| size.to_physical::<u32>(scale_factor)), it just needs to be a mut local to update it for the ability to make windows unresizable.

I'm not saying that the logic is wrong or anything. I'm trying to say that set_resize_increments already does conversion to physical pixels with the scale_factor so the logic seems redundant and you can pass resize_increments directly?

There's a chance that I don't understand this code or scale factor isn't there.

Here we call normal_hints.set_resize_increments which just accepts a pair of i32s, physical, not window.set_resize_increments which accepts the Size enum and does the conversion - so that on window creation there is only one xconn.set_normal_hints call to set all the initial hints at once.

Same with other normal_hints related things like position/size/min_size/max_size as I said

necauqua · 2022-08-26T00:36:34Z

Ok then

Maybe later some separate effort should be put in to update all these comments with log warnings (I'd personally also add a feature to disable that? or better a special log target(s) for those 'not implemented/unsupported' warnings)

necauqua · 2022-08-26T00:48:21Z

oh wow I dont think I've anything to do with those fails

CHANGELOG.md

src/window.rs

kchibisov · 2022-08-28T21:35:27Z

src/platform_impl/linux/x11/window.rs

@@ -386,7 +386,7 @@ impl UnownedWindow {
                normal_hints.set_min_size(min_inner_size.map(Into::into));
                normal_hints.set_max_size(max_inner_size.map(Into::into));
                normal_hints.set_resize_increments(
-                    pl_attribs
+                    window_attrs
                        .resize_increments
                        .map(|size| size.to_physical::<u32>(scale_factor).into()),


I'm not saying that the logic is wrong or anything. I'm trying to say that set_resize_increments already does conversion to physical pixels with the scale_factor so the logic seems redundant and you can pass resize_increments directly?

There's a chance that I don't understand this code or scale factor isn't there.

kchibisov · 2022-09-03T17:16:26Z

Could you rebase. Should be good after that.

Introduced in #2411

necauqua changed the title ~~Implement dynamically changing resize increments~~ Allow changing resize increments after window creation Aug 7, 2022

necauqua mentioned this pull request Aug 8, 2022

Add an option to ask X11/MacOS to use window resize increments (cell-precision resize) alacritty/alacritty#5417

Closed

kchibisov requested changes Aug 9, 2022

View reviewed changes

madsmtm self-requested a review August 10, 2022 14:30

madsmtm requested changes Aug 10, 2022

View reviewed changes

src/platform_impl/macos/window.rs Outdated Show resolved Hide resolved

examples/window_resize_increments.rs Show resolved Hide resolved

src/platform_impl/macos/window.rs Show resolved Hide resolved

necauqua marked this pull request as ready for review August 13, 2022 23:02

necauqua requested review from msiglreith and francesca64 as code owners August 13, 2022 23:02

madsmtm approved these changes Aug 14, 2022

View reviewed changes

src/window.rs Outdated Show resolved Hide resolved

src/window.rs Show resolved Hide resolved

src/platform_impl/macos/window.rs Show resolved Hide resolved

msiglreith removed their request for review August 14, 2022 11:11

kchibisov requested changes Aug 23, 2022

View reviewed changes

kchibisov requested changes Aug 28, 2022

View reviewed changes

kchibisov approved these changes Sep 3, 2022

View reviewed changes

Allow changing resize increments after window creation

5100b85

kchibisov merged commit ab56e9f into rust-windowing:master Sep 3, 2022

necauqua deleted the dynamic-resize-increments branch September 3, 2022 18:54

madsmtm added a commit that referenced this pull request Sep 8, 2022

macOS: Fix WindowBuilder::with_resize_increments

d7d6495

Introduced in #2411

madsmtm mentioned this pull request Sep 8, 2022

macOS: Fix WindowBuilder::with_resize_increments #2477

Merged

1 task

madsmtm added a commit that referenced this pull request Sep 8, 2022

macOS: Fix WindowBuilder::with_resize_increments (#2477)

da7bf8e

Introduced in #2411

madsmtm mentioned this pull request Sep 8, 2022

Tracking issue: Sizing(&mut PhysicalSize) event #850

Closed

MultisampledNight mentioned this pull request Aug 3, 2023

Auto fit content size when resizing the window neovide/neovide#1956

Closed

madsmtm mentioned this pull request Aug 25, 2024

Rename "inner size" to "surface size" #3889

Merged

3 tasks

madsmtm mentioned this pull request Sep 4, 2024

Which dimensions should resize increments be relative to? #3900

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow changing resize increments after window creation #2411

Allow changing resize increments after window creation #2411

necauqua commented Aug 7, 2022 •

edited

Loading

necauqua commented Aug 8, 2022

kchibisov commented Aug 8, 2022

madsmtm left a comment

necauqua commented Aug 13, 2022

madsmtm left a comment

necauqua commented Aug 15, 2022

kchibisov commented Aug 23, 2022

kchibisov Aug 23, 2022

necauqua Aug 26, 2022

kchibisov Aug 28, 2022

necauqua Aug 28, 2022 •

edited

Loading

necauqua commented Aug 26, 2022

necauqua commented Aug 26, 2022

kchibisov Aug 28, 2022

kchibisov commented Sep 3, 2022

Allow changing resize increments after window creation #2411

Allow changing resize increments after window creation #2411

Conversation

necauqua commented Aug 7, 2022 • edited Loading

necauqua commented Aug 8, 2022

kchibisov commented Aug 8, 2022

madsmtm left a comment

Choose a reason for hiding this comment

necauqua commented Aug 13, 2022

madsmtm left a comment

Choose a reason for hiding this comment

necauqua commented Aug 15, 2022

kchibisov commented Aug 23, 2022

kchibisov Aug 23, 2022

Choose a reason for hiding this comment

necauqua Aug 26, 2022

Choose a reason for hiding this comment

kchibisov Aug 28, 2022

Choose a reason for hiding this comment

necauqua Aug 28, 2022 • edited Loading

Choose a reason for hiding this comment

necauqua commented Aug 26, 2022

necauqua commented Aug 26, 2022

kchibisov Aug 28, 2022

Choose a reason for hiding this comment

kchibisov commented Sep 3, 2022

necauqua commented Aug 7, 2022 •

edited

Loading

necauqua Aug 28, 2022 •

edited

Loading