Add minimize to system tray - #789
Conversation
|
The issue with all the systray solutions LLM's come up with is that they don't understand Gremlin's constraints well. As a result they always want to use QtWidget's based solutions which will work right now, but once the input viewer is reworked nothing in Gremlin requires QtWidgets and they will be dropped. As such adding new components that requires that won't work. There is a QML system tray icon but that falls back onto QtWidgets, making it not suitable. This effectively leaves a solution that directly uses Window's systray interface via pywin32 which Gremlin requires as a dependency anyway. |
76e91ef to
d508e36
Compare
|
Reworked to use the Win32 A hidden helper window handles the icon's mouse/menu messages (its wndproc is driven by Qt's running event loop), and window show/hide still goes through the QML |
|
Ran it through my usual merge steps, the LLM comes up with these things it thinks may be troublesome: 0. remove() double-deletes the tray icon, and _on_destroy is a confused fallbackremove() calls NIM_DELETE, then DestroyWindow. DestroyWindow sends WM_DESTROY synchronously, which dispatches to _on_destroy, which calls NIM_DELETE again. The second call silently fails (the icon is already gone), but the two methods now share responsibility for the same side-effect. The intent of _on_destroy is clearly a defensive fallback for abnormal window destruction — it should be the only place NIM_DELETE lives, and remove() should just call DestroyWindow. 1. Upgrade to NOTIFYICON_VERSION_4The current code uses the pre-Vista notification protocol. Calling NIM_SETVERSION after NIM_ADD unlocks: Cursor position packed into wparam (eliminates the GetCursorPos call) NIN_SELECT for keyboard-driven activation (accessibility) 2. Make the helper window invisible to the shellThe current CreateWindow uses WS_OVERLAPPED (= 0) at zero size, which works but relies on the window being invisible by accident. A zero-size overlapped window can briefly flicker in the taskbar on some DWM configurations. Use reateWindowEx with WS_EX_TOOLWINDOW | WS_EX_NOACTIVATE to explicitly exclude it from the taskbar and Alt+Tab. 3. Add NIF_SHOWTIP to the notification dataNIF_SHOWTIP (added in Vista) opts into the modern rich tooltip and ensures the tip text shows even when the user has balloon tips suppressed by group policy. Probably giving these files as context to an LLM will make it understand these inputs: |
Restores the pre-R14 minimize-to-tray behaviour as an opt-in option. When 'minimize-to-tray' is enabled, minimizing the window hides it to a tray icon instead of the taskbar; clicking the icon restores it. The tray menu offers profile activate/deactivate and quit (via the window's close path so unsaved changes are still flagged), and the icon reflects the active state. Implemented directly against the Win32 Shell_NotifyIcon API via pywin32 (already a Gremlin dependency) rather than QtWidgets' QSystemTrayIcon, so it adds no QtWidgets dependency. Window show/hide goes through the QML QWindow (QtGui). Self-contained in gremlin/ui/tray.py plus the option registration and tray creation in joystick_gremlin.py.
- remove() no longer double-deletes the icon: DestroyWindow triggers the WM_DESTROY handler, which is now the sole owner of NIM_DELETE - opt into NOTIFYICON_VERSION_4 (NIM_SETVERSION after NIM_ADD) and mask the low word of lparam in the event handler, since v4 packs the icon id into the high word; also handle NIN_SELECT / NIN_KEYSELECT / WM_CONTEXTMENU for keyboard-driven activation and menu - create the helper window with WS_EX_TOOLWINDOW | WS_EX_NOACTIVATE so the zero-size window can't flicker in the taskbar or steal focus - add NIF_SHOWTIP so the tooltip shows even when balloon tips are suppressed
The top tray-menu entry now reads Hide/Show depending on whether the window is currently visible, mirroring the Activate/Deactivate entry, so the menu can both stash the window into the tray and bring it back.
d508e36 to
17e5174
Compare
|
Thanks for the detailed review — addressed all four in a follow-up commit, rebased on current
pywin32 doesn't expose There's also one small enhancement in a separate commit: the top menu entry now toggles Hide/Show based on window visibility (mirroring the Activate/Deactivate entry) so the menu can both stash the window into the tray and bring it back. Happy to drop that commit if you'd rather keep this PR to just the fixes. |
| @@ -0,0 +1,234 @@ | |||
| # -*- coding: utf-8; -*- | |||
There was a problem hiding this comment.
Better to name this system_tray.py, that way it's obvious what this is for.
| # Shell_NotifyIcon values that pywin32 does not expose as constants. | ||
| # Opting into version 4 unlocks the modern notification protocol (cursor | ||
| # position packed into wparam, keyboard selection events, better multi-monitor | ||
| # menu placement); NIF_SHOWTIP opts into the standard tooltip even when the | ||
| # user has balloon tips suppressed by policy. |
There was a problem hiding this comment.
That's the kind of comment LLMs like to write. I don't think they are very good, since the LLM always wants to explain its reasoning to the user, which is fair, to let the human agree or disagree. But it has no place in the code. If the reader doesn't understand the meaning, they should read up on it to understand. So here I'd expect a human to write a comment like
# Use notify protocol v4 and force standard tooltip behavior even if OS policies override them.
This is in part personal preference and in part experience from working on large code bases. The code LLMs produce is usually of fair quality but needs massaging to properly adhere to standards and not overlook specific things. The comments is where they fail in my eyes since they don't very well distinguish between "discussion with the user and explaining" and "code comment generation". Also asking them to be "concise and precise" often doesn't produce good comments either sadly. So far I always find it that manually rewriting the comments is required to get a comment that's worth having. I may try to let Claude run over Gremlin's code to build a "write acceptable comments" skill.
The above is more of an explanation of why I probably will "complain" about most comments that feel too "explanatory". I'm fine with those being added as "this is what the code is meant to do" to aid in code review, though.
| Gremlin is moving away from any QtWidgets dependency, so the tray can't be a | ||
| QSystemTrayIcon (QtWidgets, as is the QML tray fallback). It talks to the | ||
| Windows shell directly instead, via pywin32 which Gremlin already requires. | ||
| Window show/hide still goes through the QML window, a QWindow that lives in | ||
| QtGui rather than QtWidgets. | ||
|
|
||
| When the minimize-to-tray option is enabled, minimizing the main window | ||
| hides it into the tray instead of the taskbar; clicking the icon restores | ||
| it. The tray menu offers profile activate/deactivate and quit, and the icon | ||
| reflects whether a profile is currently active. |
There was a problem hiding this comment.
Falls under the same not-very-useful comment stuff. Especially the first paragraph is pure history but irrelevant to the code.
| self._config = Configuration() | ||
| self._backend = Backend() |
There was a problem hiding this comment.
Both are singletons so can be retrieved at any time on demand. The Backend I'd say is fine to keep here but the Configuration being used just once makes little sense. The "time" required to get it is irrelevant in the scheme of things.
| """Removes the tray icon and helper window (call on shutdown). | ||
|
|
||
| DestroyWindow dispatches WM_DESTROY synchronously, and its handler is | ||
| the sole owner of the icon removal, so tearing down the window here is | ||
| enough -- deleting the icon as well would double-delete it. | ||
| """ |
There was a problem hiding this comment.
Idem for this comment, it actually raises questions:
- Is this required to be called? What if one doesn't call it?
- The comment will end in user facing documentation but exposes implementation detail which is bad.
If the goal is to prevent future us from going in and deleting the icon again, the comment should be inside the code block along the lines of:
# The main window manages the icon resource lifetime.
Though I also don't see any icon resource that could be freed, so unsure if this comment even applies still.
| active = self._backend.gremlinActive | ||
| return self._active_icon if active else self._idle_icon | ||
|
|
||
| def _add_icon(self) -> None: |
There was a problem hiding this comment.
Isn't this more of a _create_icon / _install_icon?
| return win32gui.LoadImage( | ||
| 0, gremlin.util.resource_path(relative_path), win32con.IMAGE_ICON, | ||
| size, size, win32con.LR_LOADFROMFILE | ||
| ) |
There was a problem hiding this comment.
The entire code base is now Ruff-conformant, so this likely would reformat to something like:
return win32gui.LoadImage(
0,
gremlin.util.resource_path(relative_path),
win32con.IMAGE_ICON,
size,
size,
win32con.LR_LOADFROMFILE,
)
| def _load_icon(self, relative_path: str) -> int: | ||
| """Loads an .ico at the system tray icon size.""" |
There was a problem hiding this comment.
I try to document the functions in full, i.e. with args and returns such that if I ever get around to run sphinx over it there is a complete api doc.
| active = self._backend.gremlinActive | ||
| return self._active_icon if active else self._idle_icon |
There was a problem hiding this comment.
| active = self._backend.gremlinActive | |
| return self._active_icon if active else self._idle_icon | |
| return self._active_icon if self._backend.gremlinActive else self._idle_icon |
| return self._active_icon if active else self._idle_icon | ||
|
|
||
| def _add_icon(self) -> None: | ||
| win32gui.Shell_NotifyIcon(win32gui.NIM_ADD, ( |
There was a problem hiding this comment.
Just running ruff format and check over the file should fix all formatting bits.
| # Select the modern notification behaviour. Must follow NIM_ADD; the | ||
| # version is carried in the timeout/version field of the data tuple. |
There was a problem hiding this comment.
I'd simplify this and put it above the first one. There it makes sense to say "using v4 which first does a before 2 can be done".
| def _refresh_icon(self) -> None: | ||
| """Keeps the icon in sync with the profile activation state.""" |
There was a problem hiding this comment.
| def _refresh_icon(self) -> None: | |
| """Keeps the icon in sync with the profile activation state.""" | |
| def _activity_changed_cb(self) -> None: | |
| """Updates the icon to match Gremlin's activation state.""" |
| self._window.visibilityChanged.connect(self._on_visibility_changed) | ||
| self._backend.activityChanged.connect(self._refresh_icon) |
There was a problem hiding this comment.
| self._window.visibilityChanged.connect(self._on_visibility_changed) | |
| self._backend.activityChanged.connect(self._refresh_icon) | |
| self._window.visibilityChanged.connect(self._visibility_changed_cb) | |
| self._backend.activityChanged.connect(self._activity_changed_cb) |
In the Python part of Gremlin I use some_signal_cb for callback functions. The on_some_signal would be mirroring what the QML side does.
| """Restores on a left click / double click / keyboard selection of the | ||
| icon, and shows the context menu on a right click / menu key.""" |
There was a problem hiding this comment.
| """Restores on a left click / double click / keyboard selection of the | |
| icon, and shows the context menu on a right click / menu key.""" | |
| """Handles tray icon interactions. | |
| Restores on a left click, double click, or keyboard selection of the icon. Right click or menu key display the context menu.""" |
| event = lparam & 0xFFFF | ||
| if event in ( |
There was a problem hiding this comment.
This might be a case where extracting a simple operation into a function is a good idea.
- It's reusable and testable if we need
- You can put explaining comment there
- The call site stays clean and easily understandable
| self, hwnd: int, msg: int, wparam: int, lparam: int | ||
| ) -> int: | ||
| """Dispatches a tray menu selection.""" | ||
| command = wparam & 0xFFFF |
There was a problem hiding this comment.
This is similar to the above a "magic" parsing which I'd probably factor out into a helper.
You can take a look at these places in Gremlin where I deal a lot with windows types and fucntions to get an idea of what that might look like. I haven't revisited them in some time so there may also be bits I would do differently now.
| if self._window.isVisible(): | ||
| self._window.hide() | ||
| else: | ||
| self.restore() |
There was a problem hiding this comment.
Possibly this works too? But maybe is a bit too "terse"?
self._window.hide() if self._window.isVisible() else self.restore()
| def _on_taskbar_created( | ||
| self, hwnd: int, msg: int, wparam: int, lparam: int | ||
| ) -> int: | ||
| """Re-adds the icon after an Explorer restart.""" | ||
| self._add_icon() | ||
| return 0 |
There was a problem hiding this comment.
- Since this is a callback I'd stick to
_taskbar_created_cb. - Is this guaranteed to only run on that event to make sure we don't end up adding icon after icon due to some funny issue
- Or can we check whether the icon still exists?
| self._add_icon() | ||
| return 0 | ||
|
|
||
| def _on_destroy( |
There was a problem hiding this comment.
- Same bit about the name, I'd prefer
_destruction/destroy_cb. - Is there a need for guards around the actual variables and the call and its return value?
| # TrackPopupMenu needs the owning window in the foreground to dismiss | ||
| # correctly, plus a follow-up message after it returns (a documented | ||
| # Win32 quirk). |
There was a problem hiding this comment.
Similar comment to other LLM comments, they are not of good quality.
- Doesn't say what the purpose is
- Is verbose for what it says (low signal to noise ratio)
- Claims it works around some issue that allegedly is documented but doesn't provide a link
| win32gui.DestroyMenu(menu) | ||
|
|
||
| def _quit(self) -> None: | ||
| """Quits via the window's close path so unsaved changes are flagged.""" |
There was a problem hiding this comment.
Unclear what this quits, i.e. the taskbar or Gremlin. I assume the latter but that doesn't become clear.
| cfg.register( | ||
| "global", | ||
| "general", | ||
| "minimize-to-tray", | ||
| PropertyType.Bool, | ||
| False, | ||
| "Minimize to the system tray instead of the taskbar.", | ||
| {}, | ||
| True, | ||
| ) |
There was a problem hiding this comment.
The other option we should add together with this is the "close minimizes instead of closing".
| "minimize-to-tray", | ||
| PropertyType.Bool, | ||
| False, | ||
| "Minimize to the system tray instead of the taskbar.", |
There was a problem hiding this comment.
| "Minimize to the system tray instead of the taskbar.", | |
| "Minimize the Gremlin window to the system tray instead of the taskbar.", |
These descriptions are user facing documentation, so ideally are not overly terse.
| # Create the system tray icon so the window can hide to and be | ||
| # restored from the tray, and tear it down cleanly on exit. | ||
| self.tray_icon = gremlin.ui.tray.SystemTrayIcon(self.main_window) | ||
| self.aboutToQuit.connect(self.tray_icon.remove) |
There was a problem hiding this comment.
I'd prefer to group the other aboutToQuit signal connection below, that way they are in one place and it's easy to konw where these are meant to go.
| # Create the system tray icon so the window can hide to and be | ||
| # restored from the tray, and tear it down cleanly on exit. |
There was a problem hiding this comment.
Doesn't provide much value, it's a systray icon it will do what a systray icon is there for :-)
|
I wrote a skill to generate comments (https://github.com/WhiteMagic/JoystickGremlin/tree/develop/.claude/skills/comment-code) it seems to produce passable results, maybe on the "too little comment" side, which honestly is better than ending up with a novel. Model wise this uses Claude with Sonnet 5, so may work better or worse depending on model used I'd bet. |
2 participants
Restores the pre-R14 minimize-to-tray behaviour as an opt-in option.
What
minimize-to-trayoption (off by default). When enabled, minimizing the main window hides it to a tray icon instead of the taskbar; clicking the icon restores and focuses it.QSystemTrayIcon.isSystemTrayAvailable().Backend.activityChanged) and quit — quit routes through the window's normal close path so the unsaved-changes prompt still fires.Notes
gremlin/ui/tray.py;joystick_gremlin.pyonly registers the option and creates the icon. No behaviour change when the option is off.gfx/icon*.icoassets andBackendsignals; nothing added tosignal.