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

Implement WindowBuilder::resizable and WindowHandle::resizable for windows #712

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

teddemunnik
Copy link
Contributor

Implementation of #633 for windows.

When WindowBuilder::resizable or WindowHandle::resizable is set to false the window can no longer be resized by dragging on the borders, and the window can no longer be maximized.
not resizable

druid-shell/src/platform/windows/window.rs Show resolved Hide resolved
Comment on lines 1088 to 1090
style |= (WS_THICKFRAME | WS_MAXIMIZEBOX) as LONG_PTR;
} else {
style &= !(WS_THICKFRAME | WS_MAXIMIZEBOX) as LONG_PTR;
Copy link
Member

Choose a reason for hiding this comment

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

Need to use WindowLongPtr instead of LONG_PTR for 32-bit support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is WindowLongPtr a type? I can't seem to find it in the winapi docs. From what I understand GetWindowLongPtrW and SetWindowLongPtrW returns and uses LONG_PTR which is 32 bit on 32 bit windows, and 64 bit on 64 bit windows.

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit confusing for sure.

  1. WindowLongPtr is a druid-shell type defined in window.rs aka this very same file. You can search for it to see its definition.
  2. GetWindowLongPtrW only works on 64-bit but the Rust winapi crate we're using correctly aliases that function to GetWindowLongW in 32-bit mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks! I didn't think to look in druid for WindowLongPtr type.. ;) I'm a bit confused though as to why it is neccesary:

  • According to MSDN LONG_PTR is defined to be the size of a pointer, that is 4 bytes on 32 bit windows, and 8 bytes on 64 bit windows. LONG_PTR docs
  • In the winapi crate LONG_PTR is defined to be isize, which also is 4 bytes on 32 bit windows, and 8 bytes on 64 bit windows: isize docs, winapi LONG_PTR docs

Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I found issue #437, I had expected this to be the same between using the windows API in C++ and rust.. Will change it :)

Copy link
Member

Choose a reason for hiding this comment

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

Well the docs for GetWindowLongPtrW state that for 32-bit support you should use GetWindowLongPtr instead which is aliased to GetWindowLong on 32-bit.

The rust winapi crate aliases GetWindowLongPtrW to GetWindowLongW on 32-bit. The GetWindowLongW expects i32 while GetWindowLongPtrW expects isize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, makes sense. Seems like it's something that might be getting changed in winapi 0.4. Thanks for explaining!

druid-shell/src/platform/windows/window.rs Outdated Show resolved Hide resolved
@teddemunnik
Copy link
Contributor Author

Thanks for the review @xStrom! I completely neglected error handling, giving it another pass soon :)

@teddemunnik
Copy link
Contributor Author

Updated with error handling and WindowLongPtr. I re-used Error::Hr, because I am hoping that in a future change we can then use FormatMessage in the Display implementation to get a nice error description as well as just the error code. Could you give it another look @xStrom ?

@xStrom
Copy link
Member

xStrom commented Mar 20, 2020

This seems good, but we're having issues with the testing system here (#724 fixed it for the future, but doesn't apply here). Could you re-push so that it triggers the testing on a new commit? For example combine your two commits into one and force push to your branch.

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, this looks good.

@xStrom xStrom merged commit a4f6038 into linebender:master Mar 20, 2020
@teddemunnik teddemunnik deleted the windows_resizable branch April 9, 2020 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants