Rewrote systray implementaiton#164
Rewrote systray implementaiton#164SieteSieteSiete wants to merge 11 commits intoLeagueToolkit:masterfrom
Conversation
This failed due to a crash in the context menu of the system tray icon.
…ystray icon This fixed the crash, but left some of the functionality to be implemented.
This occured when the switch was toggled off and then on again, causing the context menu to not show up. This was fixed by creating and destroying the system tray icon insetad of toggling the visibility.
This caused the menu to display the wrong state. This was fixed by making sure that the state was updated every time the systray icon was created.
moonshadow565
left a comment
moonshadow565
left a comment
There was a problem hiding this comment.
What's the point of C++ class here exactly?
I'm not convinced we need full rewrite and C++ to support starting minimized to system tray like this,
| @@ -0,0 +1,52 @@ | |||
| #ifndef CSLOLSYSTEMTRAYMANAGER_H | |||
| #define CSLOLSYSTEMTRAYMANAGER_H | |||
There was a problem hiding this comment.
I'm not sure what's the point of adding this C++ class, qml provides SystemTrayIcon implementation: https://doc.qt.io/qt-6/qml-qt-labs-platform-systemtrayicon.html
In fact i'm pretty sure that's what we used before
There was a problem hiding this comment.
Yes, that is what you used before. However, when you try and interact with the system tray icon when the initial visibility for the application window is set to false, trying to open the context menu immediately crashes the program. You can work around this by having the window start visible and then hiding it in the Component.onCompleted section, but this results in the window flashing open briefly on startup, where I wanted the program to start more unobtrusively. I went with this approach to separate the tray from the actual application window to prevent these behaviors, but I understand if this is too big of a change.
| } | ||
| function createSystemTrayIcon() { | ||
| if (!systemTrayIcon) { | ||
| var component = Qt.createComponent("CSLOLSystemTrayIcon.qml"); |
There was a problem hiding this comment.
Do we really have to resort to dynamically creating this?
There was a problem hiding this comment.
I initially went with a visibility based approach, but I discovered that when I toggled the Enable systray switch multiple times, the context menu would stop working altogether, while the icon still appeared. Maybe I was missing something obvious (this iteration of the program is commit ac90a9e), but with my previous problems with the application window visibility, I figured I could just do it this way and sidestep that altogether.
| Layout.fillWidth: true | ||
|
|
||
| onCheckedChanged: { | ||
| if (checked) { |
There was a problem hiding this comment.
I'm not sure this is best way to handle this, wouldn't be much simpler to bind to enabled/visible property of system tray itself
| createSystemTrayIcon(); | ||
| // Update the patcher state immediately after creation | ||
| if (systemTrayIcon) { | ||
| systemTrayIcon.updatePatcherRunning(cslolTools.state === CSLOLTools.StateRunning); |
There was a problem hiding this comment.
[windowMaximized](qrc:/CSLOLDialogSettings.qml:151: ReferenceError: CSLOLTools is not defined)
There was a problem hiding this comment.
That's my bad, this was left over from a previous change. This if statement can be removed.
| #include <QDesktopServices> | ||
| #include <QUrl> | ||
| #include <QCoreApplication> | ||
| #include <QSystemTrayIcon> |
There was a problem hiding this comment.
This depends on Qt widgets.
We are not building with Qt widgets enabled in cmake, so this will fail.
In fact i'm not sure if our build of Qt even has widgets enabled: https://github.com/LeagueToolkit/cslol-manager/blob/master/.github/workflows/qt.yaml#L16
There was a problem hiding this comment.
Fair enough. Is this the build of Qt that the buildit workflow uses? I only ask because that was what I was using to test the commits.
2 participants
I wanted the program to be able to start hidden with the systray icon visible and functional. I did this by adding a separate C++ implementation for the system tray.