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

Semantics of getters/setters and events? #3690

Open
madsmtm opened this issue May 8, 2024 · 5 comments
Open

Semantics of getters/setters and events? #3690

madsmtm opened this issue May 8, 2024 · 5 comments
Labels
C - needs discussion Direction must be ironed out F - question There's no such thing as a stupid one

Comments

@madsmtm
Copy link
Member

madsmtm commented May 8, 2024

I'm working on improving the internals of the event handling and queuing on macOS (and iOS), and couldn't seem to find any docs on what the expected behaviour actually is? Take the following example of modifying and querying the outer position of a window:

match event {
    WindowEvent::Moved(new_position) => {
        println!("moved to: ({}, {})", new_position.x, new_position.y);
    },
    WindowEvent::KeyboardInput { event: KeyEvent { state: ElementState::Pressed, .. }, .. } => {
        println!("setting position");
        window.set_outer_position(PhysicalPosition::new(100, 100));
        let new_position = window.outer_position().unwrap();
        println!("new position: ({}, {})", new_position.x, new_position.y);
    },
    _ => (),
}

There are a few possible ways this could behave:

  1. The value is immediately updated, and querying it yields the new value:
    setting position
    new position: (100, 100)
    moved to: (100, 100)
    
  2. The value is deferred to only be updated once the event executes:
    setting position
    new position: (712, 234) // Old value
    moved to: (100, 100)
    
  3. The event is executed immediately, and the new value is available afterwards:
    setting position
    moved to: (100, 100) // Emitted before window.set_outer_position returns
    new position: (100, 100)
    

Of these, I would argue that option 3 is the "better" option, as it allows the user to have one place where updates to the position is always executed, and every part of the system instantly remain up to date about the current state. It is, however, also not possible in Rust with the current event handler, since it'd be re-entrant (and is probably also pretty difficult to implement efficiently).

That leaves us with option 1 and 2, of which I'm honestly not sure which is the best one? I can see the arguments for both sides, with option 1 ensuring consistency within the section, while option 2 ensures consistency within the entire application (forcing you to do stuff that depends on a specific update in that event handler).

Currently though, I'm leaning towards option 1 as that's how the macOS backend currently works (mostly), and I think that's what users expect when they call a method called set_x.

Is that how it works on the other platforms too? And for all the other methods? I see that we have request_inner_size, I think because it works like option 2 on some platforms (X11? Windows?), can and should we change that to be more consistent?

@madsmtm madsmtm added F - question There's no such thing as a stupid one C - needs discussion Direction must be ironed out labels May 8, 2024
@daxpedda
Copy link
Member

Is that how it works on the other platforms too? And for all the other methods?

Not for Web, where any getter method will always return the current true value regardless of what the last thing is the user did. Though more often then not this turns out to look like 1. anyway because on Web most things are updated immediately.

I see that we have request_inner_size, I think because it works like option 2 on some platforms (X11? Windows?), ...

Yes, additionally because there is no guarantee that this is set to the value the user requested because e.g. its not allowed to be that big/small, or in the case of Web there is no way to accurately set the size, it might be a pixel off.

can and should we change that to be more consistent?

I think it would be best to basically make a list of every single method and let backend maintainers answer this question for every method. Though this is probably a lot of work.

But yes, I also believe we should be consistent here and document it when we are not, which we did in some places.


I also agree that we should stick with option 1., as long as viable, e.g. consistent on all platforms. Though it might not make sense for functions that can't predict what the outcome might be, e.g. with request_inner_size().

@kchibisov
Copy link
Member

Well, reporting old value could be just incorrect because the windowing system could deny what you did, so you use something wrong and e.g. fail in validation, etc.

In general, only platforms where things are entirely client side can do that correctly and do 1 or 3. So I guess wayland/macOS can do so.

You also can not really do e.g. 3 unless you allow re-entrance.

About the option 1, it works the way you'd expect only on Wayland, the rest likely can reject it on server and you'll desync unless you try to correct the user. And the user could also easily enter the update cycle because they want to prevent the server change. Though, you can end up with that in a lot of cases right now anyway and we shouldn't prevent from it.

@madsmtm
Copy link
Member Author

madsmtm commented May 10, 2024

Rough conclusion from today's meeting: Option 1 has the desired semantics.

So taking the simpler example of set_title, the following assert should succeed:

window.set_title("foo");
assert_eq!(window.title(), "foo");

There are, naturally, cases where it might not, where the title might be updated in between those two calls, such as:

  • On web, when running from a service worker, the main thread (or another service worker, or some JS somewhere) might race setting a new title.
  • On macOS, when running from a thread that is not the main thread, and other user code sets the title.
  • On X11, the server is asynchronous, and might receive another event (from the user). (Or might just misbehave, and not set the title?)
  • On all platforms, if setting the title concurrently from two threads (I think?)

The point is not to rule out the above cases, the point is that the title is immediately available in the getter.

In short, the desired behaviour is that you will never get an old value, though you might get a newer value than the one you just set.

Does that match your understanding?


I think instead of making a list of which methods play by this rule, and which ones doesn't, I'm gonna make a test case or some feature-flagged assertions, or something, to ensure that this is the behaviour, and to make it easy to test if it is (in the future too).

@kchibisov
Copy link
Member

On X11, the server is asynchronous, and might receive another event (from the user). (Or might just misbehave, and not set the title?)

It could be not applied instantly. In general, relying that title was actually changed is not a great idea because it's not guaranteed, etc, etc.

I'd just suggest that we model methods that are crucial, like Window::request_inner_size as close to their real semantics as possible, and for the rest we do best effort when it comes to consistency.

@daxpedda
Copy link
Member

This has come up in #3861, where assert_eq!(Window::set_fullscreen(Some(Fullscreen::Borderless(None))), Window::fullscreen()) would fail.

This is again caused by the Web call actually being async, so we can't actually know if the request succeeded or not right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out F - question There's no such thing as a stupid one
Development

No branches or pull requests

3 participants