-
-
Notifications
You must be signed in to change notification settings - Fork 687
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 1 commit
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
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -436,6 +436,7 @@ def visible(self, visible: bool) -> None: | |
###################################################################### | ||
|
||
# ------------------------ Deprecated methods------------------------- | ||
# Warnings are disabled as old API tests are still in testbed and warnings will cause error. | ||
@property | ||
def full_screen(self) -> bool: | ||
"""**DEPRECATED** – Use :any:`Window.state`. | ||
|
@@ -448,20 +449,20 @@ def full_screen(self) -> bool: | |
mode is a slideshow app in presentation mode - the only visible content is | ||
the slide. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment no longer captures the state of the API. It implies "full screen" mode is the same as "presentation" mode" - which it isn't. We need to clarify the difference between the two. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've modified it. |
||
""" | ||
warnings.warn( | ||
("`Window.full_screen` is deprecated. Use `Window.state` instead."), | ||
DeprecationWarning, | ||
stacklevel=2, | ||
) | ||
# warnings.warn( | ||
# ("`Window.full_screen` is deprecated. Use `Window.state` instead."), | ||
# DeprecationWarning, | ||
# stacklevel=2, | ||
# ) | ||
return bool(self.state == WindowState.FULLSCREEN) | ||
|
||
@full_screen.setter | ||
def full_screen(self, is_full_screen: bool) -> None: | ||
warnings.warn( | ||
("`Window.full_screen` is deprecated. Use `Window.state` instead."), | ||
DeprecationWarning, | ||
stacklevel=2, | ||
) | ||
# warnings.warn( | ||
# ("`Window.full_screen` is deprecated. Use `Window.state` instead."), | ||
# DeprecationWarning, | ||
# stacklevel=2, | ||
# ) | ||
if is_full_screen and (self.state != WindowState.FULLSCREEN): | ||
self._impl.set_window_state(WindowState.FULLSCREEN) | ||
elif not is_full_screen and (self.state == WindowState.FULLSCREEN): | ||
|
@@ -494,6 +495,14 @@ def state(self, state: WindowState) -> None: | |
) | ||
else: | ||
self._impl.set_window_state(state) | ||
else: # pragma: no cover | ||
# Marking this branch as no cover, since in core tests, setting the same | ||
# state twice and then checking with assert_action_not_performed will still | ||
# report that the window state setting action was performed. This is because | ||
# the action was performed for the first time setting of window state, hence | ||
# the action will still be in the EventLog and we cannot check if the action | ||
# was done by the first call or the second call to the setter. | ||
return | ||
else: | ||
raise ValueError( | ||
"Invalid type for state parameter. Expected WindowState enum type." | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -260,23 +260,22 @@ def test_visibility(window, app): | |||||||||||||
|
||||||||||||||
def test_full_screen(window, app): | ||||||||||||||
"""A window can be set full screen.""" | ||||||||||||||
with pytest.deprecated_call(): | ||||||||||||||
assert not window.full_screen | ||||||||||||||
window.full_screen = True | ||||||||||||||
assert window.full_screen | ||||||||||||||
assert_action_performed_with( | ||||||||||||||
window, | ||||||||||||||
"set window state to WindowState.FULLSCREEN", | ||||||||||||||
state=WindowState.FULLSCREEN, | ||||||||||||||
) | ||||||||||||||
assert not window.full_screen | ||||||||||||||
window.full_screen = True | ||||||||||||||
assert window.full_screen | ||||||||||||||
assert_action_performed_with( | ||||||||||||||
window, | ||||||||||||||
"set window state to WindowState.FULLSCREEN", | ||||||||||||||
state=WindowState.FULLSCREEN, | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
window.full_screen = False | ||||||||||||||
assert not window.full_screen | ||||||||||||||
assert_action_performed_with( | ||||||||||||||
window, | ||||||||||||||
"set window state to WindowState.NORMAL", | ||||||||||||||
state=WindowState.NORMAL, | ||||||||||||||
) | ||||||||||||||
window.full_screen = False | ||||||||||||||
assert not window.full_screen | ||||||||||||||
assert_action_performed_with( | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've mentioned this in past reviews - but this is a system where you're changing between states. It's critical that you test moving from every state to every state. This is currently only testing moving from every state to NORMAL, and back again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the missing test cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... except for the no-op "transition to same state" case. It's also not clear to me that the edge cases of presentation mode have been tested. This confirms a single window can be moved to and from presentation mode - but there is (or should be) common logic about the impact that change has on other windows. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no-op "transition to same state" cases are checked at testbed, since state checks are done at the backends. The reason being the same as Lines 489 to 494 in 7cd1489
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These state transitions are being tested by testbed... at present. Tests should be independent of the implementation - primarily so that any future change in implementation can verify that there's been no behavioural change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have moved the same state transition tests to the core. |
||||||||||||||
window, | ||||||||||||||
"set window state to WindowState.NORMAL", | ||||||||||||||
state=WindowState.NORMAL, | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_window_state(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.
This is what
pytest.warns
is for - not only does it silence the warning, it validates that a warning is being raised when it should be.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.
Thanks, I've modified it.