Fix gtk window.state getter when window is hidden

[proneon267]

Currently, on all platforms except on gtk, when the window is hidden using hide(), the window.state getter continues to report the same state as it did when the window was last visible. But on gtk, when the window is hidden, window.state getter always reports it as WindowState.NORMAL. This PR fixes the bug on gtk.

This bug was identified in #2096. Since, it would take some time to accommodate the other inconsistencies identified on gtk for implementation of visibility events(i.e., on_show and on_hide), I have created this fix as a separate PR, independent of the other PRs.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

The test and the general approach of the implementation makes sense; one suggestion inline to avoid having a state flag that is potentially in an indeterminate state.

[freakboy3742]

This makes me slightly nervous - you're adding a persistent state variable to the widget, but only setting it under very specific circumstances. That means the _previous_state attribute might not exist, and might have a misleading value that isn't the "previous" state under a lot of circumstances.

Since this is entirely compensating for the fact that "get_window_state()" doesn't return a reliable value, I'd suggest merging this flag into _window_state_flags - making that value a (last known state, last event state) tuple.

That way, there's a single "state" variable, and its value will always exist and be reliable.

[proneon267]

I included the previous state in _window_state_flags as a member:

diff --git a/gtk/src/toga_gtk/window.py b/gtk/src/toga_gtk/window.py
index 1fe08d0cd..2bb59176c 100644
--- a/gtk/src/toga_gtk/window.py
+++ b/gtk/src/toga_gtk/window.py
@@ -32,7 +32,7 @@ class Window:
         )
         self.native.connect("window-state-event", self.gtk_window_state_event)
 
-        self._window_state_flags = None
+        self._previous_state_current_window_flags = None
         self._in_presentation = False
         # Pending Window state transition variable:
         self._pending_state_transition = None
@@ -69,12 +69,16 @@ class Window:
     ######################################################################
 
     def gtk_window_state_event(self, widget, event):
+        previous_state = self.get_window_state()
         # Get the window state flags
-        self._window_state_flags = event.new_window_state
+        self._previous_state_current_window_flags = [
+            previous_state,
+            event.new_window_state,  # Current window flags
+        ]
         current_state = self.get_window_state()
 
         if self.get_visible():
-            self._previous_state = current_state
+            self._previous_state_current_window_flags[0] = current_state
 
         if self._pending_state_transition:
             if current_state != WindowState.NORMAL:
@@ -192,10 +196,17 @@ class Window:
     def get_window_state(self, in_progress_state=False):
         if in_progress_state and self._pending_state_transition:
             return self._pending_state_transition
-        window_state_flags = self._window_state_flags
+        if self._previous_state_current_window_flags:
+            previous_state, window_state_flags = (
+                self._previous_state_current_window_flags
+            )
+        else:
+            window_state_flags = None
         if window_state_flags:  # pragma: no branch
-            if window_state_flags & Gdk.WindowState.WITHDRAWN or not self.get_visible():
-                return self._previous_state
+            if (
+                window_state_flags & Gdk.WindowState.WITHDRAWN
+            ) or not self.get_visible():
+                return previous_state
             elif window_state_flags & Gdk.WindowState.MAXIMIZED:
                 return WindowState.MAXIMIZED
             elif window_state_flags & Gdk.WindowState.ICONIFIED:

It works correctly, but is complicated and hard to follow. So, I have gone with a simpler implementation to ensure that the cached flag is always available and cached correctly.

[proneon267]

Apologies for requesting a review very early, but I think there might be another way that also avoids setting the previous state flag.

Since, _window_state_flags is a bit field, so we can add the bitfield of the previous state flag on to the new bit flag if the window is hidden, and so the _window_state_flags would become reliable even when the window would be hidden. I will try it out and report back.

[freakboy3742]

The tracking of _window_state_flags is already a bit of a hack - I'd rather keep the hack all in one place and use a single tuple, rather than multiple state variables. Storing _window_state_flags = (current_state,event.new_window_state) means we can isolate the messy part to a single state variable, further discouraging any attempt to use it in practice.

[proneon267]

Ok, so in the new implementation, I am directly storing the state on _window_state_flags, and have removed the use of any extra state variables.

[proneon267]

It works :D Thanks for suggesting to keep them at one place, I wouldn't have tried it otherwise.

[freakboy3742]

Nice - that's a much cleaner approach.