Skip to content
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

chat: add close to tray #3109

Merged
merged 12 commits into from
Oct 25, 2024
Merged

chat: add close to tray #3109

merged 12 commits into from
Oct 25, 2024

Conversation

bgallois
Copy link
Contributor

As suggested in issue #2844, added the option to close to the tray:

  • An option to activate or deactivate this behavior (default is no tray, as before).
  • Minimize to the tray.
  • Restore or quit from the tray icon.

@manyoso
Copy link
Collaborator

manyoso commented Oct 18, 2024

Lemme know when this is ready to review by taking it out of draft and awesome first contribution!

@bgallois bgallois force-pushed the add_tray branch 2 times, most recently from 184e828 to d46b511 Compare October 19, 2024 08:59
@bgallois
Copy link
Contributor Author

The conditionals are a little bit trickier than I anticipated with the recursive close form on onSaveChatsFinished, but the behavior is as follows:

  • Same as before without the tray activated.

With the tray:

  • Save chats when the window is closed to the tray (to avoid losing chats if the computer is shut down while the app is still in the tray, the window is invisible, or the system kills the app).
  • Restore from the tray to make the window visible again.
  • Quitting from the tray triggers the usual close sequence by saving and closing the app.

Functionality can be built upon this, such as displaying a tray alert when generating something significant while the app is in the tray, etc...

@bgallois bgallois marked this pull request as ready for review October 19, 2024 09:10
@manyoso
Copy link
Collaborator

manyoso commented Oct 20, 2024

I see this error when I attempt to build your branch:

-- Found QtIFW 4.5.2 version
CMake Error at /home/atreat/Qt/6.7.2/gcc_64/lib/cmake/Qt6/QtPublicWalkLibsHelpers.cmake:259 (message):
The Widgets target is mentioned as a dependency for SingleApplication, but
not declared.

@bgallois
Copy link
Contributor Author

What command and flags are you using to compile? I'm unable to reproduce the issue. Have you started from a clean slate? I noticed that after changing the SingleApplication dependency, it was necessary to remove any old build artifacts completely.

The Widget target should normally be handled in the back by SingleApplication.

@manyoso
Copy link
Collaborator

manyoso commented Oct 22, 2024

See: bgallois#1

@bgallois
Copy link
Contributor Author

bgallois commented Oct 22, 2024

Okay, the simplification is not working as expected, or at least it doesn't implement the same set of features:

Feature Changes:

  • Right-clicking on the tray icon maximizes the window, making the "Restore" button useless. The "Restore" button should be removed if this behavior is preferred.
  • The main simplification was removing the chat saving when minimized, so during a computer shutdown, the chat will not be saved if the user doesn't manually close the program. This seems risky to me because users usually tend to forget the minimized program.

Bugs:

  • Quitting the program is now impossible in tray mode. Clicking "Quit" in the tray will hide the tray icon but doesn't close the software, and the window also needs to be closed so the program finally closes.
  • Adding "widget" to the CMakeLists.txt fixes the problem that I can reproduce, but it seems illogical because previously "gui" was required but wasn't explicitly added to CMakeLists.txt, as it was handled by the SingleApplication dependency. "widget" should follow the same principle and be added automatically (that what appends on my system).

You are right that using raise() and requestActivate() is better for restoring the window, and that left-clicking the icon should restore the window.

@manyoso
Copy link
Collaborator

manyoso commented Oct 22, 2024

Okay, the simplification is not working as expected, or at least it doesn't implement the same set of features:

Feature Changes:

  • Right-clicking on the tray icon maximizes the window, making the "Restore" button useless. The "Restore" button should be removed if this behavior is preferred.

I'm testing on linux. Right clicking only activates the menu for me for some reason. I will fix this pre-emptively by looking at the 'reason' arg in onActivated. The 'restore' can menu item can stay even if it is redundant as you can left click or middle click to restore.

  • The main simplification was removing the chat saving when minimized, so during a computer shutdown, the chat will not be saved if the user doesn't manually close the program. This seems risky to me because users usually tend to forget the minimized program.

I'll fix this.

Bugs:

  • Quitting the program is now impossible in tray mode. Clicking "Quit" in the tray will hide the tray icon but doesn't close the software, and the window also needs to be closed so the program finally closes.

Clicking 'Quit' does quit the program for me. You're testing on mac or windows? I'm on linux, so perhaps we have different behavior?

  • Adding "widget" to the CMakeLists.txt fixes the problem that I can reproduce, but it seems illogical because previously "gui" was required but wasn't explicitly added to CMakeLists.txt, as it was handled by the SingleApplication dependency. "widget" should follow the same principle and be added automatically (that what appends on my system).

Without this I was getting build error.

@bgallois
Copy link
Contributor Author

bgallois commented Oct 23, 2024

I'm also testing on Linux (and after a system restart and a clean build the program is now quitting correctly but not restored by left-click, so something is not stable). The build error is likely masking another error because it isn't logical and reproducible, and maybe it should be investigated more closely.

Feel free to push directly to this branch to complete and close this PR with the features as you see fit.

@manyoso
Copy link
Collaborator

manyoso commented Oct 23, 2024

I'm also testing on Linux (and after a system restart and a clean build the program is now quitting correctly but not restored by left-click, so something is not stable). The build error is likely masking another error because it isn't logical and reproducible, and maybe it should be investigated more closely.

Feel free to push directly to this branch to complete and close this PR with the features as you see fit.

Unfortunately, I can't push directly to this PR as it is open on your fork I think. The widgets dependency I'll look into as I guess your thesis is that it should be handled by the SingleApplication cmake file?

More concerning is the weird behavior on left click not working for you and right click not working for me... seems unstable. This is in "labs" and is it possible that the underlying Qt qml class is just buggy?? If so, we can't add it yet...

@bgallois
Copy link
Contributor Author

I allowed "Allow edits by maintainers" so you should be able to push on it.

The widgets dependency I'll look into as I guess your thesis is that it should be handled by the SingleApplication cmake file?

Yes, like it was the case before with the Gui dependency necessary for the QGuiApplication.

More concerning is the weird behavior on left click not working for you and right click not working for me... seems unstable. This is in "labs" and is it possible that the underlying Qt qml class is just buggy?? If so, we can't add it yet...

Maybe it is better to implement it with the reason as you suggested, to remove any "default" behavior that can depend on the platforms and systems.

@manyoso
Copy link
Collaborator

manyoso commented Oct 23, 2024

@bgallois can you test the latest version I just pushed and review to see if you're happy with it?

@bgallois
Copy link
Contributor Author

It is working perfectly for me.

@manyoso
Copy link
Collaborator

manyoso commented Oct 24, 2024

There are some inconsistencies and problems across the three platforms i'm testing on now: linux, windows, and macos. Trying to resolve them all... but still no dice.

Signed-off-by: Adam Treat <[email protected]>
@manyoso manyoso merged commit 57c0974 into nomic-ai:main Oct 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants