-
-
Notifications
You must be signed in to change notification settings - Fork 684
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
Fixed cocoa window content visibilty in app full screen #2800
Conversation
cocoa/src/toga_cocoa/window.py
Outdated
# In app full-screen mode, window._impl.native.contentView is moved to a | ||
# _NSFullScreenWindow, making NSWindow.contentView as None. Hence, retain a | ||
# reference to the view before going full-screen. | ||
self.content_view = self.native.contentView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full explaination comment that I had previously written, but felt too big to include in the source file:
Once we go fullscreen on window._impl.native.contentView
, the view referenced by window._impl.native.contentView
is detached from the current native NSWindow
, and attached to a _NSFullScreenWindow
. And the view can remain attached to 1 native window at a time i.e., either to NSWindow
or NSFullScreenWindow
, but not to both.
Hence, we can no longer accesss the view by window._impl.native.contentView
, since window._impl.native.contentView
will be None. So, it is useful to retain a reference to the view attached to window._impl.native.contentView
, before going fullscreen or doing any other operation that would displace the contentView
from the current window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may not be able to reference _impl.native.contentView
, but you can reference _impl.container.native
. That keeps the container as a source of truth, and removes any bugs that could leak in if the container content is changed but content_view
isn't updated.
cocoa/src/toga_cocoa/app.py
Outdated
# guaranteed to be present and assigned on a NSWindow. | ||
# | ||
# Hence, we need to go fullscreen on window._impl.native.contentView instead. | ||
window._impl.content_view.enterFullScreenMode(screen, withOptions=opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a bonus effect of using window._impl.native.contentView
, we can lift the requirement of needing a content to be assigned to a window in cocoa, for entering into PRESENTATION mode in #2473
The linux-wayland testbed failure is unrelated and I haven't made any changes to other backends or to the core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good analysis of the underlying problem. The fix broadly looks good, although as I've noted inline, it can be made cleaner by removing the copy of the content view and using the source of truth - the container's content.
There's also a need for a testbed test to validate the change works as expected. This is resolving a bug, so we should have a test to validate that the bug no longer exists. We have existing tests of going full screen; I suspect if we added a validation that the size of the rendered content is >0, it would fail until this fix is applied.
cocoa/src/toga_cocoa/window.py
Outdated
# In app full-screen mode, window._impl.native.contentView is moved to a | ||
# _NSFullScreenWindow, making NSWindow.contentView as None. Hence, retain a | ||
# reference to the view before going full-screen. | ||
self.content_view = self.native.contentView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may not be able to reference _impl.native.contentView
, but you can reference _impl.container.native
. That keeps the container as a source of truth, and removes any bugs that could leak in if the container content is changed but content_view
isn't updated.
Oh - the Wayland failure is an intermittent issue we've seen that seems to be related to the specific timing of actions on CI under Wayland. I've been unable to replicate it locally, but re-running usually fixes it. Feel free to ignore that specific error if you see it in your own tests. |
readthedocs failure is a temporary timeout of a fostodon link and is unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a small tweak to the release note to make it clear this was a macOS problem, but otherwise - nice work!
Thank you! |
Fixed cocoa window content visibilty in app full screen.
The bug was caused since in cocoa, widgets are not added to
window.content._impl.native
, but rather towindow._impl.container.native
. However, this PR usesNSWindow.contentView
to remain internal-implementation independent and less likely to break with any future changes.#2796
With
window.content
astoga.Box()
:With
window.content
astoga.ScrollContainer()
:PR Checklist: