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.

Sign up for GitHub

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

Jump to bottom

Align sync status and icon #8858

Merged
merged 2 commits into from
Aug 6, 2021
Merged

Align sync status and icon #8858

merged 2 commits into from
Aug 6, 2021

Conversation

TheOneRing
Copy link
Contributor

With this change we will display the problem state if we have blacklisted errors.
This change also aligns the icon in the tray icon with the icon displayed for the folder state.

image

@TheOneRing
Copy link
Contributor Author

image

@TheOneRing TheOneRing force-pushed the work/align_icons branch 2 times, most recently from b279882 to 5989c12 Compare August 4, 2021 14:17
@TheOneRing TheOneRing changed the title WIP: Align sync status icons Align sync status and icon Aug 4, 2021
@TheOneRing TheOneRing marked this pull request as ready for review August 4, 2021 14:17
@TheOneRing TheOneRing requested review from fmoc and dschmidt August 4, 2021 14:17
michaelstingl
michaelstingl reviewed Aug 4, 2021
View reviewed changes
Copy link
Contributor

@michaelstingl michaelstingl left a comment

Choose a reason for hiding this comment

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

Changes the "works as designed" behaviour explained in #2432

michaelstingl
michaelstingl approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@michaelstingl michaelstingl 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 this is a good change 👍

@TheOneRing TheOneRing changed the base branch from master to 2.9 August 5, 2021 13:04
fmoc
fmoc suggested changes Aug 5, 2021
View reviewed changes
src/libsync/theme.cpp
case SyncResult::Success:
if (status.hasUnresolvedConflicts()) {
return syncStateIcon(SyncResult { SyncResult::Problem }, sysTray, sysTrayMenuVisible);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to do something about the formatter here. The placing of these braces is misleading. I needed to read it a second time to see that it's constructing a new object.

src/libsync/theme.h
@@ -101,7 +101,9 @@ class OWNCLOUDSYNC_EXPORT Theme : public QObject
/**
* get an sync state icon
*/
virtual QIcon syncStateIcon(SyncResult::Status, bool sysTray = false, bool sysTrayMenuVisible = false) const;
QIcon syncStateIcon(const SyncResult &status, bool sysTray = false, bool sysTrayMenuVisible = false) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming the first param to result.

@TheOneRing
Align sync status icons
7cb8390 
Display info state if we have ignored issues

Fixes: #7715
Fixes: #7365
Fixes: #7200
Fixes: #5860
@TheOneRing
Count ignored errors
cc5000e
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 6, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing merged commit 39b6360 into 2.9 Aug 6, 2021
@delete-merged-branch delete-merged-branch bot deleted the work/align_icons branch August 6, 2021 11:38
@jnweiger jnweiger mentioned this pull request Aug 23, 2021
Closed
20 tasks
@avrhack
Copy link

avrhack commented Sep 9, 2021

I think this is a massive mistake - I have loads of deliberately ignored files and hidden files which I choose not to sync, so the client always shows the icon instead of success which is completely misleading. This should not happen where the 'errors' are warnings caused by specific user choices that are known about.

@TheOneRing
Copy link
Contributor Author

@avrhack Please see the discussion in #8970 hidden files won't display the i icon.

@avrhack
Copy link

avrhack commented Sep 9, 2021

@avrhack Please see the discussion in #8970 hidden files won't display the i icon.

Thanks Hannah - I think that discussion just confirms my point; this change is a huge step backwards.

You say hidden files won't display the icon but all my errors are either hidden or ignored files and the icon DOES show so I'm afraid I have to respectively disagree with you. The icon should not be showing in this circumstance but it is.

I've had to revert to 2.8.2 as 2.9.0 is now unusable for me with this 'enhancement' and from that discussion others feel the same.

@s1mm0n
Copy link

s1mm0n commented Sep 13, 2021

Also this seems to affect VFS.

All Virtual Files will be shown as "ignored" with the yellow icon.

MacOS 11.5.1 - Client 2.9.0 Build 5150

image

@TheOneRing
Copy link
Contributor Author

Can you check whether there is a file2.jpeg.owncloud on your server? Stuff like that used to happen in the past.
If not please open a new issue and don't comment on pull requests.

@s1mm0n s1mm0n mentioned this pull request Sep 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelstingl michaelstingl michaelstingl approved these changes

@fmoc fmoc fmoc approved these changes

@dragotin dragotin Awaiting requested review from dragotin

@dschmidt dschmidt Awaiting requested review from dschmidt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TheOneRing @avrhack @s1mm0n @michaelstingl @fmoc