Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added
on_gain_focus
,on_lose_focus
,on_show
&on_hide
handlers ontoga.Window
#2096base: main
Are you sure you want to change the base?
Added
on_gain_focus
,on_lose_focus
,on_show
&on_hide
handlers ontoga.Window
#2096Changes from all commits
6e2eda0
4bb2a42
3a202e4
9cecb4a
60a3cac
87ea0a4
6f38ae2
6dce43e
3133dfb
3b03ec4
d746ba8
096e6bf
943612d
7b864fe
88c56b9
156bc9e
aa4714e
6895d8d
27bf2a7
9fabc3e
7b23209
0427a8f
b9e0412
7f7468d
429568b
3440221
fc8a29c
6fd5e3c
44bade9
8604faa
84234e8
eaeaf5d
ea28017
894a89c
cb2e541
f7a014b
0a1d093
c441e77
059ea14
81dd316
d958394
d94a69a
8c364a1
c0405c3
927b3d4
869da6a
2423548
9fd0e0e
77b8822
89f095d
69f90d5
d5e8e3f
52e75b8
dc8b52d
004a98a
2fa2bfa
246eeb0
673e2cd
5cf9bbe
bfe62fa
e65c528
5273e54
69f3f25
30cb114
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This line of repeated logic doesn't seem entirely right. You're adding a stateful variable
is_previously_shown
; but it's not clear to me that the value of this variable can't be derived from other properties. You're also checking the value of is_visible in a number of cases where it's not clear that you'd be able to be invisible (how do you deminaturize a window that isn't visible?)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.
Not visible to the user refers to window states like minimized, hidden, etc. So, this checks for cases where the window was in a minimized state(Not visible to the user) to a maximized state(Visible to the user).
Same logic as: #2096 (comment)
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.
These two strike me as either unnecessary, or incomplete. I didn't think you could make a hidden window full screen (in which case, they're unnecessary); if you can, then what happens when you exit full screen? Does it return to the hidden state? If it returns to a visible state, then how is the window not
is_visible
?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.
Same logic as #2096 (comment)
Not visible to the user refers to window states like minimized, hidden, etc. So, these check for cases where the window goes to fullscreen(Visible to the user) from a minimized state(Not Visible to the user).
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'm not sure I following why these 2 are necessary. A window has to be visible in order for a user to click on the zoom/maximize button - so it must already be shown.
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 had added
self.impl._is_previously_shown
because minimizing a window is same as losing focus to the native OS.But from the API viewpoint, minimizing a window should mean that it is not visible to the user and also it has lost focus.
isVisible
would beTrue
even when the window is minimized. But as per the use case of the API, the window contents are practically not visible to the user and the app can pause updating its contents.These two cases are necessary as the window can be maximized(making the window visible to the user) from a minimized state(not visible to the user).
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.
Right - but that just reinforces that the issue is the absence of a window state API. You've added an internal state variable to work around the fact that you can't ask a window if it's minimized. The right approach here isn't the add a workaround - you fix the underlying problem.
However, more generally, my comment still stands. This particular endpoint is invoked when a macOS window goes "full screen" as a result of hitting the "maximise" button. That requires that the window is on the screen, at some size, with its title bar visible. You can't click a maximise button that isn't there. So the window must already be in a shown state. Similar, if it's in a maximized state, it must be on the screen, with a title bar visible.
So why are these two handlers required? What's the edge case where a window goes from a "not shown" state to a "shown" state as a result of being maximized.
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.
The one edge case I had in mind was if the window was made full screen(Visible to user) from a minimized state(Not Visible to user) through API. In this case, the window doesn't needs to be in a visible to user state for it become full screen.
I agree a window state API would be better. I will start working on it as soon as the other dependent PRs are completed.
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.
This doesn't strike me as a very useful test. Firstly, the initial on_show event occurs when you call
window.show()
; subsequent events aren't the result of a notional "show" event, but the result of being minimized, made full screen etc. That is the behaviour that needs to be tested.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.
Yes, this would be better tested with a window state API in place.
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.
Take a second to think this one through. Under what conditions will you be able to see this label? :-)
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.
:-) That's why the visibility state is also printed to stdout. The only place where it's visible is when you preview the minimized window without unminimizing it(by placing the mouse on the minimized app icon in taskbar on windows).
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.
Why multiple labels? We just need to display some text to say an event occurred; we don't need multiple lines of text for each independent signal.
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.
But some window focus & window visibility events would overlap at the same time, and we won't be able to tell if the event occurred, as the singular label updated by 1st event, would be instantly updated by the overlapping 2nd event.