Closed jaredoconnell closed 1 year ago
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!
from wayland perspective LGTM. its propagating the same architectural issues as indicated by the number of warnings of unimplemented log messages. but otherwise fine.
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.
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.
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![image](https://user-images.githubusercontent.com/46976761/212110230-769589bc-ac67-4030-887e-10a871f73b1c.png)
cargo run --example input_region
If accepted, I'll also contribute these changes to Glazier, which looks to be a fork of druid-shell.
These changes are implemented on:
Stubs are created for compiling on:
Let me know if anything needs to be changed. I definitely think these changes will be valuable in the upstream repository.