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

Expose a native_handle property on WindowBase #8650

Merged
merged 4 commits into from
Mar 23, 2024

Conversation

DataTriny
Copy link
Contributor

@DataTriny DataTriny commented Mar 18, 2024

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

Closes #8597

Retrieving a pointer to the NSWindow will also be required for AccessKit integration, but the native code is not present at the moment.

Copy link

welcome bot commented Mar 18, 2024

Thanks for opening your first pull request here! 💖 Please check out our contributing guidelines.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Hi @DataTriny !

Thank you for taking care of it.

I've left a comment regarding a Cython deprecation, please consider taking care of it so we do not introduce deprecated code.

Regarding missing platforms: I will open a separate issue opened issue (#8651) to target it on a separate PR, but will be quite straightforward as SDL_GetWindowWMInfo already provides the info we need.

What do you think about adding a test that exercises this new feature?

def get_native_handle(self):
window_info = self.get_window_info()

IF USE_WAYLAND:
Copy link
Member

Choose a reason for hiding this comment

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

We're trying to avoid IF/ELSE and DEF on new code (and slowly migrating during changes required for 3.0.0) as is deprecated.

See: http://docs.cython.org/en/latest/src/userguide/migrating_to_cy30.html#deprecation-of-def-if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always try to make my changes blend into what's already there. I now have to import the struct for each platform/graphic system since the imports at the top of the file still use conditional compilation. Is it OK?

if setupconfig.USE_WAYLAND:
self.assertNotEqual(native_handle, 0)

if setupconfig.PLATFORM == 'win32':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is different from the one in the method being tested, but this is what is used in test_window_info.py so I figured maybe your CI expects this exact configuration.

@@ -126,3 +127,19 @@ def test_window_opacity_clamping_negative(self):
if self.check_opacity_support():
self.Window.opacity = -1.5
self.assertEqual(self.Window.opacity, 0.0)


class WindowNativeHandleTest(GraphicUnitTest):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess another property of native handles is that they should not change over time, but I really don't know how to exercise that in a test.

Copy link
Member

Choose a reason for hiding this comment

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

Checking if we're getting a value when we expect one is fine for me ATM.

@misl6 misl6 added this to the 3.0.0 milestone Mar 23, 2024
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Let's wait for the CI jobs to complete the run, but LGTM.
Thank you!

@misl6 misl6 merged commit b6d12cc into kivy:master Mar 23, 2024
29 checks passed
Copy link

welcome bot commented Mar 23, 2024

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose a method on kivy.core.window to get the window handle
2 participants