Add WindowBuilder::with_outer_position #1866

xixixao · 2021-02-23T07:50:10Z

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
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

I think this finishes the important platforms and we could actually merge the branch to master?

xixixao · 2021-03-01T02:05:39Z

I combined all the changes into a single commit on top of master. Let me know if I should first update this branch, or if I should open PR against master.

LoganDark · 2021-03-01T03:05:37Z

Why does this simple PR have 153 files changed? It's impossible for anyone to sort through this.

maroider · 2021-03-01T05:19:14Z

Could you make this PR against master instead, since this seems to finish the with_outer_position feature?
This PR would then supplant #1240/#1239.
You may or may not also want to add @Osspial and @murarth as co-authors, if you're feeling polite.

xixixao · 2021-03-01T16:53:22Z

Changed base to master, added contributors, fixed X11 (I can't check it so I have to wait for CI to tell me whether it's broken - let me know if there is a better way).

maroider · 2021-03-10T16:59:45Z

It seems like GitHub won't accept your co-author assignments. Try this instead:

Co-authored-by: Osspial <osspial@gmail.com>
Co-authored-by: Murarth <murarth@gmail.com>

PS: Osspial is no longer a mainainer for Windows. @msiglreith is the one you want for now.

src/window.rs

ArturKovacs · 2021-03-10T20:37:35Z

src/platform_impl/macos/window.rs

+                let (left, bottom) = match attrs.outer_position {
+                    Some(position) => {
+                        let logical = window_position(position.to_logical(scale_factor));
+                        // macOS wants the position of the bottom left corner,
+                        // but caller is setting the position of top left corner
+                        (logical.x, logical.y - height)
+                    }
+                    // This value is ignored by calling win.center() below
+                    None => (0.0, 0.0),
+                };


This doesn't work for me. When I create two windows and set the height of both to 50 and set the y coordinate of one of them to 50, using with_outer_position and set the y coordinate of the other to 50 using set_outer_position, both windows should be at the same y coordinate. But this is what I get:

I believe you forgot to include the menubar height in your calculations here. You might need to create the window first and then set the position for the existing window. This might help: https://stackoverflow.com/questions/28955483/how-do-i-get-default-title-bar-height-of-a-nswindow

This is unlikely to be solvable. Creating window and then setting its position breaks the whole point of this feature, which is to avoid the flickering when creating a window and then repositioning it.

I'm also not sure which position is wrong one, whether this one or the one in set_outer_position.

I could see if I could hardcode the title bar height perhaps to fix the wrong one.

Guessing or hardcoding the title bar height is unlikely to work, due to macOS UI changes like this:

https://www.reddit.com/r/MacOSBeta/comments/hq00ll/insane_titlebar_height_and_question_on_logical/

I think we'll have to settle on documenting that this method sets "inner" position on MacOS, despite its name, due to platform limitations.

Yep we should definitely not hardcode or guess the menubar height when the function name promises to set the outer position. For the same reason I don't think we should just set the inner position when the function is called with_outer_position. But I agree that it would be nice to have at least some way to set the startup position. So at this point the only thing I have problem with is the function name. I think calling it with_approx_position or something similar would be completely fine. And the documentation should of course describe what this means for each platform.

What do you think @msiglreith @murarth?

src/platform_impl/macos/window.rs

Co-authored-by: Osspial <osspial@gmail.com> Co-authored-by: Murarth <murarth@gmail.com>

… with_outer_position

xixixao · 2021-03-13T02:34:53Z

Ready for re-review fyi @ArturKovacs @msiglreith

msiglreith

Thanks, lgtm (Windows).

The default Win10 theme has some invisible borders which result in some minor offsets (7/8pts) of the visibile position. But not too important imo.

…not be exactly outer or inner position

xixixao · 2021-03-22T01:18:35Z

This should be ready for merging btw. cc @ArturKovacs

ArturKovacs · 2021-03-22T07:14:55Z

src/window.rs

+    /// **Windows**: Borders might be shown outside the set position, so the position is
+    /// approximate.


This should specify what kind of position it is, and to me, the wording is a bit hard to understand. I suggest

"The outer position. In other words, the top left corner of the title-bar. There may be a small gap between this position and the window due to specifics of the Window Manager"

This is not yet addressed

ArturKovacs · 2021-03-22T07:29:52Z

src/window.rs

+    /// The desired position of the window. If this is `None`, some platform-specific position
+    /// will be chosen.
+    ///
+    /// The default is `None`.
+    ///
+    /// ## Platform-specific
+    ///


The Platform-specific list should be formatted as a list, that is each item beggining with a - like this:

- **macOS**: ... - **Window**: ...

Also under Platform-specific, you should describe the kind of position this means on all of the platforms. For example

- **X11**: The outer position. - **Others**: This is ignored.

ArturKovacs · 2021-03-22T07:35:15Z

I know I'm being unusually picky, but it's mostly because this one functionality does different things on different platforms, so I just want to make sure that whoever comes across this will have no confusion about what it does.

Furthermore considering that the name of a public function and a public field has changed, I believe @msiglreith and @murarth need to approve this before it can be merged.

maroider · 2021-03-22T07:56:49Z

Furthermore considering that the name of a public function and a public field has changed

Isn't this PR purely additive when compared against master?

ArturKovacs · 2021-03-22T08:06:48Z

What I meant was that there were changes in the API since those maintainers approved therefore their approval doesn't apply to the current state.

msiglreith · 2021-03-24T17:17:58Z

Fine for me once the latest comments by @ArturKovacs have been addressed.

xixixao · 2021-03-25T15:46:14Z

Addressed @ArturKovacs's comments.

xixixao force-pushed the with_outer_position branch from 16984d0 to 3ba3be9 Compare March 1, 2021 02:04

xixixao changed the base branch from with_outer_position to master March 1, 2021 16:31

xixixao force-pushed the with_outer_position branch 2 times, most recently from e00cba5 to 465d653 Compare March 1, 2021 16:51

xixixao force-pushed the with_outer_position branch 3 times, most recently from 5a979aa to 58e6f94 Compare March 1, 2021 19:23

ArturKovacs self-requested a review March 10, 2021 16:59

ArturKovacs suggested changes Mar 10, 2021

View reviewed changes

msiglreith changed the title ~~Add macOS support for with_outer_position~~ Add WindowBuilder::with_outer_position Mar 10, 2021

msiglreith self-requested a review March 10, 2021 22:22

Support setting position of new window on x11, macos and windows

b1122ae

Co-authored-by: Osspial <osspial@gmail.com> Co-authored-by: Murarth <murarth@gmail.com>

xixixao force-pushed the with_outer_position branch from 58e6f94 to b1122ae Compare March 12, 2021 01:18

Merge branch 'master' of https://github.com/rust-windowing/winit into…

cf54c29

… with_outer_position

msiglreith approved these changes Mar 15, 2021

View reviewed changes

Renamed outer_position to position in attributes to reflect it might …

a034020

…not be exactly outer or inner position

ArturKovacs reviewed Mar 22, 2021

View reviewed changes

Improve docs

cedcc50

murarth approved these changes Mar 25, 2021

View reviewed changes

ArturKovacs approved these changes Mar 25, 2021

View reviewed changes

msiglreith merged commit 0d634a0 into rust-windowing:master Mar 25, 2021

QuantumBadger mentioned this pull request Mar 29, 2021

Flapping windows on start QuantumBadger/Speedy2D#16

Open

TimUntersberger mentioned this pull request Apr 18, 2021

Change initial window position iced-rs/iced#515

Closed

msiglreith mentioned this pull request Jun 4, 2021

Tracking issue for WindowBuilder::with_outer_position #1190

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WindowBuilder::with_outer_position #1866

Add WindowBuilder::with_outer_position #1866

xixixao commented Feb 23, 2021 •

edited

Loading

xixixao commented Mar 1, 2021

LoganDark commented Mar 1, 2021

maroider commented Mar 1, 2021

xixixao commented Mar 1, 2021

maroider commented Mar 10, 2021

ArturKovacs Mar 10, 2021

xixixao Mar 11, 2021 •

edited

Loading

xixixao Mar 11, 2021

ArturKovacs Mar 15, 2021 •

edited

Loading

ArturKovacs Mar 15, 2021

xixixao commented Mar 13, 2021

msiglreith left a comment

xixixao commented Mar 22, 2021

ArturKovacs Mar 22, 2021

msiglreith Mar 24, 2021

ArturKovacs Mar 22, 2021

ArturKovacs commented Mar 22, 2021

maroider commented Mar 22, 2021

ArturKovacs commented Mar 22, 2021

msiglreith commented Mar 24, 2021

xixixao commented Mar 25, 2021

		/// Windows: Borders might be shown outside the set position, so the position is
		/// approximate.

Add WindowBuilder::with_outer_position #1866

Add WindowBuilder::with_outer_position #1866

Conversation

xixixao commented Feb 23, 2021 • edited Loading

xixixao commented Mar 1, 2021

LoganDark commented Mar 1, 2021

maroider commented Mar 1, 2021

xixixao commented Mar 1, 2021

maroider commented Mar 10, 2021

ArturKovacs Mar 10, 2021

Choose a reason for hiding this comment

xixixao Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

xixixao Mar 11, 2021

Choose a reason for hiding this comment

ArturKovacs Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

ArturKovacs Mar 15, 2021

Choose a reason for hiding this comment

xixixao commented Mar 13, 2021

msiglreith left a comment

Choose a reason for hiding this comment

xixixao commented Mar 22, 2021

ArturKovacs Mar 22, 2021

Choose a reason for hiding this comment

msiglreith Mar 24, 2021

Choose a reason for hiding this comment

ArturKovacs Mar 22, 2021

Choose a reason for hiding this comment

ArturKovacs commented Mar 22, 2021

maroider commented Mar 22, 2021

ArturKovacs commented Mar 22, 2021

msiglreith commented Mar 24, 2021

xixixao commented Mar 25, 2021

xixixao commented Feb 23, 2021 •

edited

Loading

xixixao Mar 11, 2021 •

edited

Loading

ArturKovacs Mar 15, 2021 •

edited

Loading