Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows

[edwin0cheng]

No description provided.

[edwin0cheng]

Should fixed part of the #72

[francesca64]

Thanks so much for this! This will be useful for a lot of people.

I won't have time to give this a full review until later today, though what I've looked at so far looks good. However, this needs a CHANGELOG entry.

[edwin0cheng]

Thanks , and CHANGELOG entry added.

And here is some implementation details about this PR;

1. Same as #270, we remove supports for changing screen native resolution when WindowBuilder::with_fullscreen was set. It is more consistent with others platforms implementation and more stable. (however it is a breaking change)
2. The whole set_fullscreen function are based on chrome's implemetation.
3. The ITaskbarList COM interface stuffs i had made a PR in upstream winapi crate retep998/winapi-rs#592. i would make another PR when new winapi version released.
4. Based on how current threading model works, all window sized events have to be called from main thread, so you would see all SetWindowPos is wrapped by execute_in_thread.

[edwin0cheng]

Just a minor bug first for rust-stable (I use nightly) and added support for with_maximized. (So that with_decorations, with_fullscreen all works now).

It should be completed. Thanks :)

[francesca64]

After testing this on Windows 10, everything works as expected. However, I've succeeded in breaking things: while toggling either decorations or fullscreen rapidly repeatedly, the window in question eventually freezes. My other windows don't freeze, continuing to play their animations, but as soon as one of those windows is focused (in other words, as soon as it interacts with the backend), it freezes as well. This can happen using your updated fullscreen example program, though it's far easier to reproduce if you change the key handlers to respond to Pressed rather than Released, since then you can simply hold down the key and encounter the problem very readily. This fortunately isn't an issue with maximization.

[edwin0cheng]

@francesca64 I changed the fullscreen example use PRESSED and I just tested this and it should works now.

By the way, the cause of freezes are related to this block, which i didn't fully understand why it would write in this way. A quick search lead me to PR #250.

[francesca64]

Excellent work! Not only does everything work fine now, but after reviewing every single line, I haven't found any real mistakes.

That said, I do have some changes to request. They're all pretty minor, and most of them are related to formatting/maintainability, so they won't take very long.

[francesca64]

Excellent work! Not only does everything work fine now, but after reviewing every single line, I haven't found any real mistakes.

That said, I do have some changes to request. They're all pretty minor, and most of them are related to formatting/maintainability, so they won't take very long.

[francesca64]

These constants are already defined in winapi:
https://github.com/retep998/winapi-rs/blob/d8290fc406c8e390101dd225f7ec506848fc86ce/src/um/winuser.rs#L360
https://github.com/retep998/winapi-rs/blob/d8290fc406c8e390101dd225f7ec506848fc86ce/src/um/winuser.rs#L2968-L2969

[francesca64]

keybd_event has been superseded by SendInput, which can also be used to send both events at once.

I saw the 0x45 scancode used in the example you linked, but judging by the official example, it's actually the scancode for numlock. For getting the correct scancode, MapVirtualKey looks appropriate.

[francesca64]

This doesn't appear to have any use anymore, since it will always just get unwrap_or'd to CW_USEDEFAULT in the call to CreateWindowExW.

[francesca64]

• Don't forget about WindowBuilder::with_maximized
• WindowBuilder::with_fullscreen no longer changing display resolution is an important behavior change
• Windows should be capitalized, to prevent ambiguity
• Use backticks to format method names/etc. as code

[francesca64]

It would be good to have a comment that makes it more obvious that the double-boxing is intentional, as it looks very surprising to anyone who's not already well-acquainted with the backend.

[edwin0cheng]

This execute_in_thread function i just copy from EventsLoop::execute_in_thread. Maybe i should copy the comment as well? Or i should refactor it out as a common implementation function ?

[francesca64]

Refactoring would be lovely.

[francesca64]

For calls that can't fit on one line, it's standard to have each argument on its own line with a trailing comma.

[francesca64]

Missing trailing comma

[francesca64]

Formatting:

winuser::ShowWindow(window.0, if maximized { winuser::SW_MAXIMIZE } else { winuser::SW_RESTORE });

There's a similar formatting issue at the end of mark_fullscreen, so please address it there as well.

[francesca64]

While I doubt this will 404 any time soon, it's always good to explain things in comments.

[francesca64]

I'd prefer if you used #[allow(dead_code)] per-definition rather than for the whole module.

[edwin0cheng]

Done ^^

[francesca64]

Magnificent! I can tell you went above and beyond to make things cleaner.

I was ready to merge this, but I've found one more bug. Here are the steps to reproduce:

1. Run fullscreen example program
2. Turn off decorations
3. Exit fullscreen
4. Attempt to turn decorations back on (it doesn't work until the 3rd press)

The same thing happens with toggling maximization while in fullscreen, though that only takes 2 presses.

[edwin0cheng]

Good catch, but im away from keyboard now, would check it tonight(asia time), thanks

[edwin0cheng]

@francesca64 It should be fixed :)

[francesca64]

Great, I've confirmed that it's fixed.

Here's another bug:

1. Run fullscreen example program
2. Exit fullscreen
3. Press key to maximize
4. Enter fullscreen
5. Press key to un-maximize
6. Exit fullscreen
7. (Window will still be maximized) press key to un-maximize; nothing happens until the second press

This fortunately doesn't happen for decoration toggling.

Using the built-in maximization button causes things like this to happen:

1. Run fullscreen example program
2. Exit fullscreen
3. Press the window's maximization button
4. Press key to un-maximize; nothing happens until the second press

It's also important to account for the user un-maximizing the window by moving it:

1. Run fullscreen example program
2. Exit fullscreen
3. Press key to maximize
4. Move window to un-maximize
5. Press key to maximize; nothing happens until the second press

[edwin0cheng]

Using the built-in maximization button causes things like this to happen:

Run fullscreen example program
Exit fullscreen
Press the window's maximization button
Press key to un-maximize; nothing happens until the second press

It's also important to account for the user un-maximizing the window by moving it:

Run fullscreen example program
Exit fullscreen
Press key to maximize
Move window to un-maximize
Press key to maximize; nothing happens until the second press

It is because the fullscreen example just use a bool to indicate Maximize status, which is not sync to the system status. And we do not have an api to query this status. : )

[edwin0cheng]

Run fullscreen example program
Exit fullscreen
Press key to maximize
Enter fullscreen
Press key to un-maximize
Exit fullscreen
(Window will still be maximized) press key to un-maximize; nothing happens until the second press

9f52ed1 should fixed this

[francesca64]

Right, good point about it being an issue in the example and not your implementation, sorry about that.

While it does fix my first issue, there are still issues with the wrong state being restored after using the built-in maximization button or moving to un-maximize.

1. Run fullscreen example program
2. Exit fullscreen
3. Press the window's maximization button
4. Enter fullscreen
5. Exit fullscreen
6. The window is no longer maximized

and:

1. Run fullscreen example program
2. Press key to maximize
3. Exit fullscreen
4. Move window to un-maximize
5. Enter fullscreen
6. Exit fullscreen
7. The window is maximized

[edwin0cheng]

Sorry about that, the maximized state is a little bit tricky to implement correctly. Anyway, it should be fixed all the issues listed.

[francesca64]

Awesome! This is the last thing, I promise: in the CHANGELOG, you accidentally put WindowBuilder::set_maximized instead of WindowBuilder::with_maximized.

[edwin0cheng]

Sorry about that, thank you so much to help me to catch out these bugs. It will be my second closed PR :)

[francesca64]

No problem! This PR is very much appreciated.