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::set_minimized (#985) #990

Merged
merged 20 commits into from
Dec 22, 2019
Merged

Conversation

SuperiorJT
Copy link
Contributor

@SuperiorJT SuperiorJT commented Jun 27, 2019

Gets started on supporting Window::set_minimized (#985, #983). I'm very new to this repo and a beginner in Rust so it might not be exactly how you guys want it. I've tested it on macOS via keyboard events and was able to make it minimize similar to other macOS applications. I'm willing to work on the Windows side of this as well.

  • Tested on all platforms changed
    • Windows
    • macOS
    • X11
    • Wayland
    • iOS
  • cargo fmt has been run on this branch
  • 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

@SuperiorJT SuperiorJT changed the title Expose set_minimized. Implement for macOS (#985) Window::set_minimized (#985) Jun 27, 2019
@SuperiorJT
Copy link
Contributor Author

On the Windows implementation do we want to create a WindowFlag for MINIMIZED? I'm looking at the way set_maximized works and it looks like we could do something similar, but I don't know if we should track that in our WindowState.

@Osspial
Copy link
Contributor

Osspial commented Jun 28, 2019

@SuperiorJT We should track that in WindowFlags, yeah.

@goddessfreya
Copy link
Contributor

@SuperiorJT What is the status on this PR?

@SuperiorJT
Copy link
Contributor Author

Implementations are made for windows, macOS and wayland. We need someone to develop the x11 and iOS (if applicable) implementation since I am unable to. Once all is implemented, then I can go through and fmt and update required documentation.

@murarth
Copy link
Contributor

murarth commented Sep 25, 2019

I have an implementation for X11 ready: https://github.com/murarth/winit/tree/x11-set-minimize

@SuperiorJT
Copy link
Contributor Author

I added @murarth's X11 implementation, formatted, and added documentation. It's ready for review.

Comment on lines +363 to +365
pub fn set_minimized(&self, _minimized: bool) {
unimplemented!()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this to panic or should it be a no-op? set_maximized below is a no-op.

Copy link
Contributor

@Osspial Osspial Oct 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it particularly matters, since the Android backend doesn't even compile right now.

@Osspial
Copy link
Contributor

Osspial commented Oct 17, 2019

I've tested this out on Windows, and this seems to cause glitches if you do the following:

  1. Maximize the window
  2. Minimize the window via set_minimized
  3. Alt-tab back into the window

If you do that, the minimize button in the window's titlebar has the window restore icon instead of the window minimize icon, and fails to minimize the window when pressed.

Copy link
Contributor

@vbogaevsky vbogaevsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works smoothly on macOS

@SuperiorJT
Copy link
Contributor Author

I've fixed the maximize issue. Windows didn't like the window styles being set after minimization. Let me know if you find any more issues.

Copy link
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It no-op'd as expected with i3, which doesn't support minimizing. I'm going to kick the bucket to @murarth who I'm guessing has a gnome/KDE/etc setup lying around.

@murarth
Copy link
Contributor

murarth commented Nov 22, 2019

@goddessfreya I wrote the X11 implementation myself, so I can confirm it works. If we want to ping our X11 testers to get another set of eyes on it, that's fine by me.

@goddessfreya
Copy link
Contributor

@murarth Haha, I'm ever so slightly blind. I see. Did you test for weird behavior evolving fullscreened/unmapped windows? Should be good to go on the X11 side, I don't see much that could go wrong.

@murarth
Copy link
Contributor

murarth commented Nov 22, 2019

@goddessfreya I've tested for any weird interactions and found none.

Copy link
Contributor

@Osspial Osspial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows changes look good. Could you add stub implementations for WASM? Once that's done, this should be good to merge.

@SuperiorJT
Copy link
Contributor Author

I added the stub for WASM. Let me know if I missed anything.

@Osspial Osspial merged commit 82889e2 into rust-windowing:master Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants