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

Add some extra syncing details to the tray icon tooltip #7057

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

nilsding
Copy link
Contributor

This pull request adds some more details to the tray icon tooltip text.

There was some unused code that updated some status bar text, so I opted to re-use that. As of now it displays what the old code did -- the file size to be synced and the estimated time left if applicable.

On my Linux (KDE) system it will display that information per folder:
Screenshot_20240829_172644

On Windows it will only display if there is only one folder to be synced. The tooltip message will roughly look like that (still need to check that it doesn't get truncated):
Screenshot_20240829_171619

@camilasan
Copy link
Member

adding @nextcloud/designers for feedback.

@nilsding nilsding force-pushed the feature/tooltip-sync-details branch from fefa307 to ce63577 Compare August 29, 2024 16:52
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nilsding thanks for your contribution! :) Small piece of feedback:
I assume the first "Nextcloud" in the text refers to the app name, and the second to the folder name? Considering that this is the default and it makes the text a bit complex, we could leave the first "Nextcloud: Folder" part out? So basically show it like in your Windows example, only referring to the folder name directly:

Nextcloud: Syncing 19 MB (3 minutes left)

Where "Nextcloud" is the folder name.

@nilsding
Copy link
Contributor Author

Hey @jancborchardt, thanks for the feedback :D

Getting rid of the Nextcloud: application name prefix would be as easy as modifying/removing the overridden method in systray.cpp.

desktop/src/gui/systray.cpp

Lines 569 to 572 in 99aa255

void Systray::setToolTip(const QString &tip)
{
QSystemTrayIcon::setToolTip(tr("%1: %2").arg(Theme::instance()->appNameGUI(), tip));
}

I think we could keep it for the Windows platform though, as it will display a summary of everything anyway.

On other platforms the tooltip can be more even more detailed, e.g. display the sync status of all possible folder sync configurations (and on macOS even the accounts using the VFS), e.g.

Screenshot_20240830_091944

I think in the case of more than one active sync configuration it'd be better to display the Folder prefix too, just to be extra clear. And if there's only one sync configuration, that prefix can be dropped entirely. What do you think? :)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the case of more than one active sync configuration it'd be better to display the Folder prefix too, just to be extra clear. And if there's only one sync configuration, that prefix can be dropped entirely. What do you think? :)

I would say the "Folder" prefix is not needed in either case, as people set up the connections. But I'm fine either way. :)

@nilsding nilsding force-pushed the feature/tooltip-sync-details branch from ce63577 to c9fbcec Compare August 30, 2024 08:17
@nilsding
Copy link
Contributor Author

rebased and updated. Now it looks more clean:

Screenshot_20240830_101724

...whereas on Windows it segfaults on with more than one connection. Will fix that shortly.

@nilsding
Copy link
Contributor Author

nilsding commented Aug 30, 2024

this is what the tooltip will look like on Windows with multiple sync connections now:

Screenshot_20240830_103123

With only one connection it will be still the same as before:
Screenshot_20240830_103633

I'll squash that fixup commit into the first one when this PR gets some approvals. :>

@jancborchardt
Copy link
Member

@nilsding looks great now! Only a super small detail, we can shorten the time display from "3 minutes 15 seconds remaining" to "3:15 minutes remaining"
Then it also usually doesn't line break.

But it's also fine to do it in a follow-up if it just uses some desktop native time format.

@nilsding
Copy link
Contributor Author

Sadly there is no utility function to convert the time in milliseconds to a short time like 3:15 yet, but I think for the tooltip it's more than enough to say 3 minutes instead:

Screenshot_20240830_120707

I also added some handling for the special case when the estimated ETA is 0; i.e. instead of 0 seconds left it will now display A few seconds left, similar to how the usual folder status view does it.

Screenshot_20240830_120655

src/libsync/syncengine.h Outdated Show resolved Hide resolved
src/gui/systray.cpp Outdated Show resolved Hide resolved
src/gui/folderman.cpp Outdated Show resolved Hide resolved
src/gui/folderman.cpp Outdated Show resolved Hide resolved
src/gui/folderman.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 😄
please have a look at my inline comments

@nilsding
Copy link
Contributor Author

nilsding commented Sep 2, 2024

@mgallien thanks for the review :)

re. the app name prefix on Windows I'm waiting on Jan's opinion first -- if neither of us have a strong opinion about that I'll just drop that override altogether

@camilasan
Copy link
Member

re. the app name prefix on Windows I'm waiting on Jan's opinion first -- if neither of us have a strong opinion about that I'll just drop that override altogether

I would just drop it, we usually try to keep the same between the 3 platforms unless there is specific guidelines saying otherwise.

@nilsding
Copy link
Contributor Author

nilsding commented Sep 2, 2024

we usually try to keep the same between the 3 platforms unless there is specific guidelines saying otherwise.

ah, good to know. Will remove it then :>

@nilsding nilsding force-pushed the feature/tooltip-sync-details branch from 239faec to 9dddad5 Compare September 4, 2024 16:04
@nilsding
Copy link
Contributor Author

nilsding commented Sep 4, 2024

just cleaned up the commits a bit and rebased on top of master again

@mgallien mgallien force-pushed the feature/tooltip-sync-details branch from 9dddad5 to a5fe49d Compare September 5, 2024 16:28
@mgallien
Copy link
Collaborator

mgallien commented Sep 5, 2024

@nilsding thanks again 😄
I will take care of the merge as PR from external repositories (i.e. clone) will not be run at our drone CI
sonarcloud check will also fail due to access rights again

@mgallien
Copy link
Collaborator

mgallien commented Sep 5, 2024

/backport to stable-3.14

@mgallien mgallien force-pushed the feature/tooltip-sync-details branch from a5fe49d to 233b7e0 Compare September 9, 2024 06:42
There was some unused code that updated some status bar text -- this is
now part of the tooltip during syncing.

Signed-off-by: Jyrki Gadinger <[email protected]>
@mgallien mgallien force-pushed the feature/tooltip-sync-details branch from 233b7e0 to 8f705e5 Compare September 9, 2024 07:57
@mgallien mgallien merged commit 13425f4 into nextcloud:master Sep 9, 2024
9 of 13 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.

4 participants