Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows #457

edwin0cheng · 2018-04-10T06:52:50Z

No description provided.

edwin0cheng · 2018-04-11T01:17:03Z

Should fixed part of the #72

francesca64 · 2018-04-11T04:33:13Z

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 · 2018-04-11T08:19:02Z

Thanks , and CHANGELOG entry added.

And here is some implementation details about this PR;

Same as Move fullscreen modes to not touch physical resolutions #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)
The whole set_fullscreen function are based on chrome's implemetation.
The ITaskbarList COM interface stuffs i had made a PR in upstream winapi crate Added ITaskbarList and ITaskbarList2 in shobjidl retep998/winapi-rs#592. i would make another PR when new winapi version released.
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 · 2018-04-11T08:44:44Z

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 · 2018-04-11T17:50:56Z

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 · 2018-04-11T18:30:42Z

@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 · 2018-04-11T22:37:11Z

src/platform/windows/window.rs

+    // See : https://stackoverflow.com/questions/10740346/setforegroundwindow-only-working-while-visual-studio-is-open
+    const ALT : i32 = 0xA4;
+    const EXTENDEDKEY : u32 = 0x1;
+    const KEYUP : u32 = 0x2;


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 · 2018-04-11T23:09:05Z

src/platform/windows/window.rs

+    winuser::keybd_event(ALT as _, 0x45, EXTENDEDKEY | 0, 0);
+
+    // Simulate a key release
+    winuser::keybd_event(0xA4, 0x45, EXTENDEDKEY | KEYUP, 0);


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 · 2018-04-11T23:57:36Z

src/platform/windows/window.rs

-        } else {
-            (None, None)
-        };
+        let (x, y) = (None, None);


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 · 2018-04-11T23:59:48Z

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased

+- Implement Window::set_fullscreen, Window::set_maximized and Window::set_decorations for windows.


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 · 2018-04-12T00:07:25Z

src/platform/windows/events_loop.rs

+    {
+        unsafe {
+            let boxed = Box::new(function) as Box<FnMut(_)>;
+            let boxed2 = Box::new(boxed);


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.

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 ?

Refactoring would be lovely.

francesca64 · 2018-04-12T02:08:32Z

src/platform/windows/events_loop.rs

+            let raw = Box::into_raw(boxed2);
+
+            let res = winuser::PostThreadMessageA(self.thread_id, *EXEC_MSG_ID,
+                                                 raw as *mut () as usize as WPARAM, 0);


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 · 2018-04-12T02:18:38Z

src/platform/windows/window.rs

+    let win = Window {
+        window: real_window,
+        window_state: window_state,
+        events_loop_proxy


Missing trailing comma

francesca64 · 2018-04-12T02:22:24Z

src/platform/windows/window.rs

+            // And because ShowWindow will resize the window
+            // We call it in the main thread
+            self.events_loop_proxy.execute_in_thread(move |_| {
+                winuser::ShowWindow(window.0, if maximized { winuser::SW_MAXIMIZE } else {winuser::SW_RESTORE} );


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 · 2018-04-12T02:47:11Z

src/platform/windows/window.rs

 }

 unsafe impl Send for Window {}
 unsafe impl Sync for Window {}

+// https://blogs.msdn.microsoft.com/oldnewthing/20131017-00/?p=2903


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

francesca64 · 2018-04-12T02:57:08Z

src/platform/windows/window.rs

@@ -1,4 +1,7 @@
 #![cfg(target_os = "windows")]
+#![allow(non_upper_case_globals)]
+#![allow(non_snake_case)]
+#![allow(dead_code)]


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

edwin0cheng · 2018-04-12T05:32:59Z

Done ^^

francesca64 · 2018-04-12T06:24:13Z

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:

Run fullscreen example program
Turn off decorations
Exit fullscreen
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 · 2018-04-12T06:30:32Z

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

edwin0cheng · 2018-04-12T13:51:21Z

@francesca64 It should be fixed :)

francesca64 · 2018-04-12T15:38:04Z

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

Here's another bug:

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

This fortunately doesn't happen for decoration toggling.

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

edwin0cheng · 2018-04-12T15:53:25Z

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 · 2018-04-12T15:58:07Z

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 · 2018-04-12T16:10:41Z

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.

Run fullscreen example program
Exit fullscreen
Press the window's maximization button
Enter fullscreen
Exit fullscreen
The window is no longer maximized

and:

Run fullscreen example program
Press key to maximize
Exit fullscreen
Move window to un-maximize
Enter fullscreen
Exit fullscreen
The window is maximized

edwin0cheng · 2018-04-12T16:42:01Z

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

francesca64 · 2018-04-12T17:01:30Z

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

edwin0cheng · 2018-04-12T17:05:58Z

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

francesca64 · 2018-04-12T17:10:28Z

No problem! This PR is very much appreciated.

…ples/canvas_webgl_minimal/www/y18n-4.0.1, r=jdm Bump y18n from 4.0.0 to 4.0.1 in /examples/canvas_webgl_minimal/www Bumps [y18n](https://github.com/yargs/y18n) from 4.0.0 to 4.0.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/yargs/y18n/blob/master/CHANGELOG.md">y18n's changelog</a>.</em></p> <blockquote> <h1>Change Log</h1> <p>All notable changes to this project will be documented in this file. See <a href="https://github.com/conventional-changelog/standard-version">standard-version</a> for commit guidelines.</p> <h3><a href="https://www.github.com/yargs/y18n/compare/v5.0.4...v5.0.5">5.0.5</a> (2020-10-25)</h3> <h3>Bug Fixes</h3> <ul> <li>address prototype pollution issue (<a href="https://github-redirect.dependabot.com/yargs/y18n/issues/108">#108</a>) (<a href="https://www.github.com/yargs/y18n/commit/a9ac604abf756dec9687be3843e2c93bfe581f25">a9ac604</a>)</li> </ul> <h3><a href="https://www.github.com/yargs/y18n/compare/v5.0.3...v5.0.4">5.0.4</a> (2020-10-16)</h3> <h3>Bug Fixes</h3> <ul> <li><strong>exports:</strong> node 13.0 and 13.1 require the dotted object form <em>with</em> a string fallback (<a href="https://github-redirect.dependabot.com/yargs/y18n/issues/105">#105</a>) (<a href="https://www.github.com/yargs/y18n/commit/4f85d80dbaae6d2c7899ae394f7ad97805df4886">4f85d80</a>)</li> </ul> <h3><a href="https://www.github.com/yargs/y18n/compare/v5.0.2...v5.0.3">5.0.3</a> (2020-10-16)</h3> <h3>Bug Fixes</h3> <ul> <li><strong>exports:</strong> node 13.0-13.6 require a string fallback (<a href="https://github-redirect.dependabot.com/yargs/y18n/issues/103">#103</a>) (<a href="https://www.github.com/yargs/y18n/commit/e39921e1017f88f5d8ea97ddea854ffe92d68e74">e39921e</a>)</li> </ul> <h3><a href="https://www.github.com/yargs/y18n/compare/v5.0.1...v5.0.2">5.0.2</a> (2020-10-01)</h3> <h3>Bug Fixes</h3> <ul> <li><strong>deno:</strong> update types for deno ^1.4.0 (<a href="https://github-redirect.dependabot.com/yargs/y18n/issues/100">#100</a>) (<a href="https://www.github.com/yargs/y18n/commit/3834d9ab1332f2937c935ada5e76623290efae81">3834d9a</a>)</li> </ul> <h3><a href="https://www.github.com/yargs/y18n/compare/v5.0.0...v5.0.1">5.0.1</a> (2020-09-05)</h3> <h3>Bug Fixes</h3> <ul> <li>main had old index path (<a href="https://github-redirect.dependabot.com/yargs/y18n/issues/98">#98</a>) (<a href="https://www.github.com/yargs/y18n/commit/124f7b047ba9596bdbdf64459988304e77f3de1b">124f7b0</a>)</li> </ul> <h2><a href="https://www.github.com/yargs/y18n/compare/v4.0.0...v5.0.0">5.0.0</a> (2020-09-05)</h2> <h3>⚠ BREAKING CHANGES</h3> <ul> <li>exports maps are now used, which modifies import behavior.</li> <li>drops Node 6 and 4. begin following Node.js LTS schedule (<a href="https://github-redirect.dependabot.com/yargs/y18n/issues/89">#89</a>)</li> </ul> <h3>Features</h3> <ul> <li>add support for ESM and Deno <a href="https://github-redirect.dependabot.com/yargs/y18n/issues/95">#95</a>) (<a href="https://www.github.com/yargs/y18n/commit/4d7ae94bcb42e84164e2180366474b1cd321ed94">4d7ae94</a>)</li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li>See full diff in <a href="https://github.com/yargs/y18n/commits">compare view</a></li> </ul> </details> <details> <summary>Maintainer changes</summary> <p>This version was pushed to npm by <a href="https://www.npmjs.com/~oss-bot">oss-bot</a>, a new releaser for y18n since your current version.</p> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=y18n&package-manager=npm_and_yarn&previous-version=4.0.0&new-version=4.0.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/servo/pathfinder/network/alerts). </details>

edwin0cheng added 4 commits April 10, 2018 14:49

Implement set_fullscreen for windows

bbc62fd

Implement get_current_monitor for windows

f6f998d

Implement set_maximized

348d5e8

Implement set_decorations for windows

ebf8472

edwin0cheng changed the title ~~Implement set_maximized, set_fullscreen and set_decorations for windows~~ Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows Apr 10, 2018

edwin0cheng mentioned this pull request Apr 11, 2018

Roadmap to 0.1.2 (WIP) unrust/unrust#43

Open

9 tasks

Update CHANGELOG.md

61ff5b4

edwin0minv added 2 commits April 11, 2018 16:38

Fixed minor syntax bug for stable rust version

49a49bf

Added support for WindowBuilder::with_maximized

182e8af

Move all window sized related functions to main thread

bd33cef

francesca64 mentioned this pull request Apr 11, 2018

Doc typo fix for struct winit::Window #452

Merged

francesca64 suggested changes Apr 12, 2018

View reviewed changes

edwin0cheng added 6 commits April 12, 2018 12:36

Refactor and formatting force_window_active

3320e02

Remove unused code

dac03ef

Update CHANGELOG.md

3998320

Refactor and change keyboard handling code

8a5c673

Reformatting and refactoring

87e98b3

Added back missing link for comment

f61de91

francesca64 approved these changes Apr 12, 2018

View reviewed changes

Fixed set_maximized and set_fullscreen wrong order bug

d972f57

Call ShowWindow(SW_RESTORE) when restore_saved_window

9f52ed1

Sync system maximized status when set_fullscreen

adfaf77

Fixed wrong function name

ab345b4

francesca64 merged commit bdc01fe into rust-windowing:master Apr 12, 2018

edwin0cheng deleted the windows-fullscreen branch April 12, 2018 17:12

edwin0cheng mentioned this pull request Apr 12, 2018

[META] Feature parity between platforms #252

Closed

This was referenced Apr 12, 2018

Error when switching to fullscreen on Windows #37

Closed

Would it make sense to add a way to maximize the window? #129

Closed

Is it possible to make an existing window fullscreen? #72

Closed

It would be nice if everything was pure rust. ggez/ggez#29

Closed

edwin0cheng mentioned this pull request Apr 13, 2018

Expected behavior of fullscreen and maximized in MacOS #460

Closed

This was referenced Apr 18, 2018

with_maximized(true) produces non-maximized window #471

Closed

4k HiDPI monitor detected as 2560x1440 on Windows 10 #469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows #457

Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows #457

edwin0cheng commented Apr 10, 2018

edwin0cheng commented Apr 11, 2018

francesca64 commented Apr 11, 2018

edwin0cheng commented Apr 11, 2018 •

edited

Loading

edwin0cheng commented Apr 11, 2018

francesca64 commented Apr 11, 2018

edwin0cheng commented Apr 11, 2018 •

edited

Loading

francesca64 left a comment

francesca64 Apr 11, 2018

francesca64 Apr 11, 2018

francesca64 Apr 11, 2018

francesca64 Apr 11, 2018

francesca64 Apr 12, 2018

edwin0cheng Apr 12, 2018

francesca64 Apr 12, 2018

francesca64 Apr 12, 2018

francesca64 Apr 12, 2018

francesca64 Apr 12, 2018

francesca64 Apr 12, 2018

francesca64 Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018 •

edited

Loading

edwin0cheng commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018 •

edited

Loading

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

		@@ -1,5 +1,6 @@
		# Unreleased

		- Implement Window::set_fullscreen, Window::set_maximized and Window::set_decorations for windows.

Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows #457

Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows #457

Conversation

edwin0cheng commented Apr 10, 2018

edwin0cheng commented Apr 11, 2018

francesca64 commented Apr 11, 2018

edwin0cheng commented Apr 11, 2018 • edited Loading

edwin0cheng commented Apr 11, 2018

francesca64 commented Apr 11, 2018

edwin0cheng commented Apr 11, 2018 • edited Loading

francesca64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018 • edited Loading

edwin0cheng commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018 • edited Loading

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 12, 2018

francesca64 commented Apr 12, 2018

edwin0cheng commented Apr 11, 2018 •

edited

Loading

edwin0cheng commented Apr 11, 2018 •

edited

Loading

francesca64 commented Apr 12, 2018 •

edited

Loading

edwin0cheng commented Apr 12, 2018 •

edited

Loading