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

Input Region and Always On Top support #2328

Merged
merged 38 commits into from
Feb 25, 2023

Conversation

jaredoconnell
Copy link
Contributor

This PR adds useful window features for specialized use cases. I am personally using it to create a better solution to a docking system than to have a bunch of individual windows, since Wayland doesn't allow setting Window positions. This allows specialized situations, and allows a better, less finicky option than the existing ways.

Always on top is implemented as having an input bool, allowing it to be handled by flags or levels on each system.
Input Region is implemented as having an input of a Region, where each platform converts that into a way that can be represented by each system.

All inputs are done directly in the window handler interface. I thought of the potential benefits to integrating this more into the Druid widget system, but that seems wasteful. In specialized use cases that need input region functionality, it can be handled by a custom root widget, which is how I show in the example file.

I added an example Window that demonstrates the changes and allows you to discover the system limitations for the changes on each platform.
Run it with cargo run --example input_region
image

If accepted, I'll also contribute these changes to Glazier, which looks to be a fork of druid-shell.

These changes are implemented on:

  • GTK
  • Wayland (Where possible)
  • Windows (Using API calls that should be compatible back to Windows 2000, so in reality the lowest supported OS by Druid)
  • Mac OS (Where necessary. Only always on top. Because a transparent frameless window will allow clicks through transparent parts)

Stubs are created for compiling on:

  • Web (not possible to support this)
  • X11 (I don't know enough about this to implement on this platform. But I left comments on the things that may be helpful)

I tested it on a lot. Here are the test results:

OS Arch DE API Used Notes
Windows 10 AMD64 N/A Win32 Works perfectly, minus transparent window border when input region is set
Windows 11 ARM64 N/A Win32 Same as W10
Mac OS ARM64 N/A Cocoa Always on top works perfectly. Region not implemented due to transparency doing that task
Ubuntu 20.04 LTS ARM64 Default GTK Works perfectly
Fedora 37 AMD64 Gnome on Wayland GTK Input region works perfectly except when Windowed. Wayland doesn't allow always on top
Fedora 37 AMD64 Gnome on Wayland Wayland Same
Fedora 37 AMD64 Gnome on Wayland X11 It compiles but does nothing
Fedora 37 ARM64 Gnome on X11 GTK Same as on AMD64 on Wayland. Works perfectly with exceptions. No always on top
Fedora 37 ARM64 KDE Plama GTK Input region works. You need to handle always on top and titlebar/frame in the Window menu from KDE.
Fedora 37 ARM64 Cinnamon GTK Works perfectly.
Fedora 37 ARM64 XFCE GTK Works perfectly
Web wasm32 Firefox N/A Compiles but does nothing to the window when run

Let me know if anything needs to be changed. I definitely think these changes will be valuable in the upstream repository.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thank you for your efforts. From the surface the work looks comprehensive. Hopefully we can have a nice review cycle soon.

On a meta level, I'll note that due to the size of the changes this probably won't make it in time for the Druid 0.8 release which is happening early next week hopefully. However we can land it afterwards and yes if it lands we can probably port it to glazier as a follow-up.

For now please do the following:

  • Add a changelog entry to CHANGELOG.md
  • Fix the clippy erros on Windows
error: using `clone` on type `*mut winapi::shared::minwindef::HRGN__` which implements the `Copy` trait
Error:    --> druid-shell\src\backend\windows\window.rs:735:54
    |
735 | ...                   let region_tmp = win32_region.clone();
    |                                        ^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `win32_region`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
    = note: `-D clippy::clone-on-copy` implied by `-D warnings`

error: using `clone` on type `*mut winapi::shared::minwindef::HRGN__` which implements the `Copy` trait
Error:    --> druid-shell\src\backend\windows\window.rs:749:54
    |
749 | ...                   let region_tmp = win32_region.clone();
    |                                        ^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `win32_region`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy

@xStrom xStrom added shell concerns the shell abstraction S-waiting-on-author waits for changes from the submitter labels Jan 12, 2023
@jaredoconnell
Copy link
Contributor Author

Thank you for your efforts. From the surface the work looks comprehensive. Hopefully we can have a nice review cycle soon.

On a meta level, I'll note that due to the size of the changes this probably won't make it in time for the Druid 0.8 release which is happening early next week hopefully. However we can land it afterwards and yes if it lands we can probably port it to glazier as a follow-up.

For now please do the following:

* Add a changelog entry to `CHANGELOG.md`

* Fix the clippy erros on Windows
error: using `clone` on type `*mut winapi::shared::minwindef::HRGN__` which implements the `Copy` trait
Error:    --> druid-shell\src\backend\windows\window.rs:735:54
    |
735 | ...                   let region_tmp = win32_region.clone();
    |                                        ^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `win32_region`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
    = note: `-D clippy::clone-on-copy` implied by `-D warnings`

error: using `clone` on type `*mut winapi::shared::minwindef::HRGN__` which implements the `Copy` trait
Error:    --> druid-shell\src\backend\windows\window.rs:749:54
    |
749 | ...                   let region_tmp = win32_region.clone();
    |                                        ^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `win32_region`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy

Changes addressed. Thank you for the prompt review!

@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jan 12, 2023
@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jan 24, 2023
@jaredoconnell
Copy link
Contributor Author

I'm glad you found the example useful!

Thanks for pointing out the error checks and memory leaks I overlooked! Those are definitely very important for good software.
I think I addressed all of the failure cases and memory leaks.

I tested this on Windows 10 and Windows 11.
On Windows 10, I moved the newest version around for several minutes without failure, with only a slight increase from around 9 to around 16 MB. But that appears to be the OS, not the program, since it does it with the input region feature disabled.
Windows 11 has a huge jump in memory when moving around the window like a maniac. It jumps from around 7 MB to around 135 MB. However, it drops to around 35 MB after a few seconds without resizing. I turned on and off the titlebar and it reset to around 13.5, similar to Windows 10. So this looks to be an OS problem, instead of an issue with this implementation. Though if there is a better way that someone finds in the future, that would be nice.

I also fixed an issue with lingering shadows in Mac OS.

@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jan 30, 2023
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Okay here is another round of comments, mostly doc changes.

Overall the changes continue to look good. Next steps for me are:

  • Go over the Windows code once more just to be sure.
  • Test the example in action on at least Windows and macOS

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

I went over the Windows code again, and besides one comment it looks good.

@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Feb 22, 2023
Makes it so the window isn't force-set to shown when set to always-on-top.

Co-authored-by: Kaur Kuut <[email protected]>
@jaredoconnell
Copy link
Contributor Author

I went over the Windows code again, and besides one comment it looks good.

Thanks for the review! I tested your change and it looks good. No issues.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this! It's now ready for merging. 🎉

@xStrom xStrom added port-to-glazier and removed S-waiting-on-author waits for changes from the submitter labels Feb 25, 2023
@xStrom xStrom merged commit 5fa4ce5 into linebender:master Feb 25, 2023
@jaredoconnell jaredoconnell deleted the advanced-window-features branch February 27, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port-to-glazier shell concerns the shell abstraction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants