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
Add window states API #2473
Add window states API #2473
Changes from 64 commits
ab9b778
0a90a12
73721b3
c809698
06b7f2a
11fea69
938504c
be87113
9aefd30
13d2ff3
176e879
1ba73b8
d4bfdaa
d993b80
38bf5c3
b710eaf
b461295
0c7900b
97a6c66
ba28194
c7320ce
aed5251
a76bf9b
f0d5d80
e416f20
1fa8cde
6350b5f
d091955
5c2a53b
564c5ed
e36e596
174d66b
17564a8
51cea93
feb78a8
4632738
8286b86
16d5758
5b59b14
85dd202
552c0fc
56edec1
971b8da
934b6b1
1e6bd76
426e8b9
ccef113
4d83d19
067e988
3c84a80
a276f22
78cdd3b
b9e87c5
06d5a9e
b0c37fe
09ee428
1bd1f39
2b6d347
8bba100
6b3e883
74685d1
25c420f
83dcab4
ab0ed45
4fa4af6
71f4314
a308ea7
42642ee
6b513c7
ccc8f11
56f32a7
5da8231
4c6070d
cfa255a
87383f3
c3f32c7
4b2fdbb
b592ee2
a5891e8
e8ad898
8684038
1b53c16
3cebc76
d2510e0
4485cda
d0dcb35
f429307
6aedeea
d7228ba
600e4c9
3689314
1a6b4ad
2885d1c
cd9e00b
57fd1e3
7835fe8
1c0a969
cf23ce9
e6f4dcc
30e5968
c623254
f2bd750
11444e3
0771349
c9b7b90
3441cb3
62dcd7d
4f36f28
ea8058a
11c3ec3
0bd68c4
edef651
3c8b0f1
e915d72
fb6e657
b810d65
1e59da1
9dba56b
56cc124
2d29072
c2e88e5
279dfda
73bf7aa
af6a292
5a0352f
ef68572
ca48ba2
cd8eba2
e5eff2b
26eb01c
3a6aaf7
78b5ddf
e0f5bf0
54cfaab
8b8685e
d599730
91b70da
b38ff03
2cf1839
aa120e9
3661110
c928ac1
f9c18f5
ace13ce
ef7b3dc
bc19c7f
c2235f7
b0ead86
7bbf330
0f7fe4f
7308e11
8f858be
9f95554
295cd59
5bd410e
00a742e
4ef9c50
a1613be
138d0e3
450efad
51d3590
3de8eef
77e2b7e
7db80d2
aaee1f9
903860b
a6df392
f9f1375
ad427ab
56de33d
59012cd
5a66725
bb22a9f
f6a851d
35bbb8e
8bddf0e
03dd538
2af1509
8b56d66
7cd1489
598021f
596acc4
d3f90ba
945caf5
116eec3
71aec38
2559fa0
d822ff0
14b64b7
d8c8422
bb513d3
eb2d9a5
abfa707
8f7c954
379e002
e3f31ff
383e027
c986e3d
5f365c6
2961507
9ca14ed
b6edc2a
0f74ad9
589e3c7
621c723
655a146
4f2f2a5
c29ab39
0d67322
07e82f9
8be4d8d
1692a0d
f8ee69d
b82b6d0
247847e
1b624ad
5560e3d
a8fed19
5f0c1a2
136cdcf
8fcdbd6
ef584b1
7f28348
6e0c091
f535cd6
c175b6a
bc1f902
e8864d5
0f0103c
4162e4b
eb51730
b25b902
8df6c4b
1097207
7e7cc1e
e01237a
3236559
ef8725b
9520809
4ff1fd0
0931280
2024626
91a8f72
68b0a44
0d807f6
9add077
35a49a1
c7ce202
23319cf
5b5e44a
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.
!= 0
should be redundant here. Integers cleanly evaluate as booleans.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.
Removed it.
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.
Need to be careful about this - if the main window is a
toga.Window
, the support action bar will exist, but it shouldn't ever 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 have changed it.
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.
If something doesn't work, it doesn't work. We shouldn't be committing large chunks of commented-out code.
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.
Removed it.
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 is the API
is_window_state
, and notget_window_state
?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.
Changed it.
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 above - we try not to use the "all on one lines but indented" when there's more than one argument. Add a comma to the end of the argument list and black-format to 1 argument per line.
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.
Done, Thanks.
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 feel like I've asked this before, but this ticket has enough history that it's not easy to find a relevant comment...
Why is this logic in App, and not part of the
set_window_state()
handling for Window? It's literally per-window logic for performing per-window state changes - I can't see why there's a need to split up the logic, if only because it means there's a gap inWindow.set_window_state()
that would be picked up by a formal type analysis. If there is a reason, then the reason should be documented in a code 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.
During the design discussion, we had discussed to keep presentation mode on the app in cocoa. But looking at it now, moving the code to window makes more sense. So, I'll move it.
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 can see how this mechanically exit presentation mode, but I don't see how this causes a change in the value of window.state. The
windowDidExitFullScreen:
selector will apply the pending state... but how is the pending state set? This either indicates (a) a gap in testing, or (b) the need for a comment explaining the workflow.Also - as above, it's not clear why this isn't part of the
set_window_state()
implementation.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 have moved it to
set_window_state()
.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 it doesn't do anything. Why is it required?
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.
What happens if I take a window without content, and put it into full screen mode? It seems to me that this implies a window should always have content, even if it's a dummy box.
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.
Entering presentation mode on cocoa re-homes the content(i.e.,
NSBox
) in a separateNSFullScreenWindow
. Here the window isn't going in presentation mode, rather the content is. So, without content, the API to enter presentation mode won't work on cocoa.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 I can still call the API, so it needs to return an appropriate value. As currently written, this will return a value of NORMAL. The fact that the implementation works on the content doesn't alter the public API contract.
Alternatively, it needs to be either documented as a platform quirk, or there's a higher level "guarantee that the window has content, even if it's just a toga.Box" requirement.
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 have added to the docs, that it is mandatory to have a content on a window, for it to go on PRESENTATION mode. I have also added a new check on the core and a new test.
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.
Or, we can avoid the documentation issue entirely, and fix the underlying problem. It seems relatively straightforward to define a toga.Box() as window content if the window doesn't explicitly provide content. Why not do that and avoid the edge case entirely?
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 is now solved by #2800.
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.
In terms of code organisation, it might make more sense for "NORMAL" to be the "else" case, since it's the "least special" case, and a more reasonable default.
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.
Changed it.
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.
What happens if you maximise a window that is currently in presentation mode? Or maximise a window that isn't being displayed while you're in presentation mode?
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 modified it.
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.
In which case - you pick a "default" value and make this the else. In this case, you can do this with a single level of
if
, making theNORMAL
case the fallthrough else case.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.
Changed it.
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.
Sure - but you've left the
#pragma:no cover
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.
But, since that else branch would never be touched, so removing
#pragma:no cover
would give missing coverage.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.
If the branch isn't being touched, then that's a gap in your coverage. That's something that needs to be fixed.
# pragma: no cover
is a last resort - something you only use if there is literally no way to reach a particular line of code (e.g., a scenario that can't be replicated in testbed, likeshow_actionbar
on Android's base Window class). In all other cases, it's an indication of a missing test. This case is "move to presentation mode from normal mode" - I can't see any reason why that wouldn't be testable.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,
exit_presentation_mode()
is called early inwindow.state
, so if the window was in presentation mode then it would have exited presentation mode by the timeset_presentation_mode()
would be called. So,current_state
will never be PRESENTATION when checked inside_apply_state()
. Hence, this branch cannot be reached.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.
If we're renaming an API, we need to rename it everywhere.
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.
What do we gain from making this change? Either keep the implementation here, or move it to Window. There's no need to preserve both, and make one probe dependent on the other.
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 with the underlying API - why
is_
and notget_
?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.
Changed it.