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

Window starting position #806

Closed
chrisduerr opened this issue Mar 8, 2019 · 15 comments
Closed

Window starting position #806

chrisduerr opened this issue Mar 8, 2019 · 15 comments
Labels
D - easy Likely easier than most tasks here H - good first issue Ideal for new contributors S - api Design and usability S - enhancement Wouldn't this be the coolest?

Comments

@chrisduerr
Copy link
Contributor

I'm not sure about support of other platforms, but I believe at least XCB should support setting the starting position of a Window, so in theory it should be possible to have a with_position method on at least the X11 backend.

Most of the other functions relevant at startup have a matching with_* function call for their set_* calls, so I'm not sure if this is due to a lack of support for some platforms, however it seems there are some clear advantages for the platforms that support it since it allows spawning the window without things jumping around.

@repi
Copy link
Contributor

repi commented Mar 9, 2019

A with_position would indeed be nice.

Currently we work around it by directly setting the position of the window with window.set_position, which works but is less elegant and a bit more work.

@chrisduerr
Copy link
Contributor Author

The big issue with window.set_position is that it will resize the window, no matter how early you do it. So unfortunately that creates this flickering effect where the window is first spawned at one size and then later changed to another.

@Osspial
Copy link
Contributor

Osspial commented Mar 17, 2019

This should work on all non-Wayland desktop platforms, so I'd be fine with this getting added.

@goddessfreya goddessfreya added D - easy Likely easier than most tasks here H - good first issue Ideal for new contributors S - api Design and usability S - enhancement Wouldn't this be the coolest? labels May 20, 2019
@Abendstolz
Copy link
Contributor

Abendstolz commented Jun 22, 2019

As the API was changed lately (commit) and there is only a set_outer_position I assume there should only be a with_outer_position.

Is that assumption correct?

Another question: As by @Osspial's comment this doesn't work on a Wayland desktop - does or should set_outer_position? Because the "platform-specific" part of the docs there only mention iOS.

@elinorbgr
Copy link
Contributor

Another question: As by @Osspial's comment this doesn't work on a Wayland desktop - does or should set_outer_position? Because the "platform-specific" part of the docs there only mention iOS.

Wayland apps cannot control their location on the desktop, at all.

@Abendstolz
Copy link
Contributor

Another question: As by @Osspial's comment this doesn't work on a Wayland desktop - does or should set_outer_position? Because the "platform-specific" part of the docs there only mention iOS.

Wayland apps cannot control their location on the desktop, at all.

So should that be documented in set_outer_position as well or is it considered common knowledge for anyone targeting Wayland? (Which you might do without yourself knowing)

@felixrabe
Copy link
Contributor

felixrabe commented Jun 22, 2019

It should be documented, because using winit should not require reading up on surprising implementation differences on different platforms. Everything that could cause someone to later open an issue along the lines of "why does xyz not work as expected?" should be explicitly documented.

Assume someone primarily developed the app on one platform, then later decides to try running it on another.

@chrisduerr
Copy link
Contributor Author

Usually better than documenting things is encoding it in the API. So if this doesn't work on Wayland, it just shouldn't be implemented for that platform.

@elinorbgr
Copy link
Contributor

@chrisduerr The choice between x11 and wayland is done at runtime, so we cannot really do anything more than the method doing nothing on wayland, apart maybe returning an error?

Though I tend to think it's the kind of functionality that would not really be a big issue if it just does not work on some platforms.

@chrisduerr
Copy link
Contributor Author

Oh yeah, I always forget about the Wayland/X11 troubles where everything has to be done at runtime.

I personally find it very difficult to have things only documented if there's an alternative, because often when using code completion or you're just skimming docs for specific methods you'll never find these limitations. When stuff then suddenly hard crashes on certain platforms that gets a bit problematic I think.

If the method just does nothing, that's not a hard crash at least, but that might cause even more confusion when the issue is reported and it might take a lot longer until it is reported.

Returning an error would at least directly encode the potential of runtime failure into the API, but obviously make the API inconvenient to use on other platforms.

Ultimately I can't say what the best choice is in general and I think this has to be decided on a case by case basis, but it generally seems like something that goes against the grain of Rust, where most things are encoded into the compile time API checks.

@Osspial
Copy link
Contributor

Osspial commented Jun 22, 2019

The best solution I can think of right now is to have WindowBuilder::with_outer_position return Result<WindowBuilder, NotSupportedError>, which forces people to deal with the possibility of an error when they call that function. That doesn't deal with people setting the field in WindowAttributes, though.

EDIT: Really set_outer_position should also return a NotSupportedError on Wayland. Looks like I forgot to add that in the original error message commit.

@felixrabe
Copy link
Contributor

felixrabe commented Jun 23, 2019

Speaking of Result – I think there is too much unwrap in winit. Has there been some discussion of this previously? Could we make winit 0.20 also a "don't panic" (or as little as possible) release? (Edit: Opened #971 for further discussion of this.)

@chrisduerr
Copy link
Contributor Author

Could we make winit 0.20 also a "don't panic" (or as little as possible) release?

I'd just like to say that extending the 0.20 scope further and further likely isn't a great idea. Though limiting the panics directly in winit feels like a good thing.

@murarth
Copy link
Contributor

murarth commented Sep 25, 2019

How can we get the ball rolling on this? Would it be okay if I submitted a PR creating the public API, with an implementation on X11, and created a tracking issue for implementation on other platforms?

The best solution I can think of right now is to have WindowBuilder::with_outer_position return Result<WindowBuilder, NotSupportedError>, which forces people to deal with the possibility of an error when they call that function. That doesn't deal with people setting the field in WindowAttributes, though.

I think that, while returning Result may be suitable for Window::set_outer_position, it would significantly hamper the method-chaining usage of WindowBuilder. It would be better, I think, just to add a note in the docs for WindowBuilder::with_outer_position about Wayland-specific behavior. There are a number of Window methods with "no effect" notes like this.

@VincentJousse
Copy link

Isn't this issue already fixed ?

Window and WindowBuilder structs already have the corresponding function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D - easy Likely easier than most tasks here H - good first issue Ideal for new contributors S - api Design and usability S - enhancement Wouldn't this be the coolest?
Development

No branches or pull requests

10 participants