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

Bugfix/user status #3182

Merged
merged 19 commits into from
May 25, 2021
Merged

Bugfix/user status #3182

merged 19 commits into from
May 25, 2021

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented Apr 21, 2021 •
edited
Loading

It should help with #3147, addresses some points of #3045:

  • Fix alignment:
    aligned

  • When is user is connected to the server:
    user-is-connected

  • When user is connected to the server and do NOT have a user defined message:
    no-user-defined-message

  • When user is connected to the server and do have a user defined message:
    user-defined-message

  • When is user is NOT connected to the server:
    user-is-disconnected

  • A Server with NO user status feature (<20):
    server-without-user-status

  • If the user has only one folder configured for synchronisation and then removes the synchronisation and opens the main dialog again. The folder is still displayed in the main dialog. The folder should not be displayed anymore. (For questions regarding this contact @FlexW)

  • If the user has zero folders configured for synchronisation, the main dialog should not display the green icon that the sync was successful on the right hand side of the account information (For questions regarding this contact @FlexW)

With folders configured:
with-folders-configured

With no folders configured:
with-no-folders-configured

  • Alignment on Windows:
    align

this is on the server too:
alignmentissueontheservertoo

  • inconsistency between the status icon (online/away, etc.) in the dropdown opened on the account and the account itself, the correct status on this screenshot is the one that looks like "away"
    refresh

@camilasan camilasan marked this pull request as draft April 21, 2021 19:55
@camilasan camilasan marked this pull request as ready for review April 21, 2021 20:22
@camilasan camilasan requested a review from jancborchardt April 21, 2021 20:22
@camilasan camilasan added this to the Desktop 3.2 milestone Apr 21, 2021
@camilasan camilasan mentioned this pull request Apr 21, 2021
Closed
er-vin
er-vin reviewed Apr 22, 2021
View reviewed changes
src/gui/userstatus.cpp Outdated Show resolved Hide resolved
src/gui/userstatus.cpp Outdated Show resolved Hide resolved
src/gui/tray/UserLine.qml Outdated Show resolved Hide resolved
src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved
src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved
src/gui/accountstate.cpp Show resolved Hide resolved
src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved
src/gui/tray/UserModel.h Outdated Show resolved Hide resolved
src/gui/tray/Window.qml Outdated Show resolved Hide resolved
src/gui/tray/UserLine.qml Outdated Show resolved Hide resolved
@jancborchardt
Copy link
Member

Oh this is great @camilasan! All the cases covered, hopefully no room for confusion anymore. :)

One little feedback from my side: You write "Fix alignment" but in the screenshot the text is not left-aligned at the same line, but there’s 2 different lines? Would look better if it’s all left-aligned along the same line.

  • Fix alignment:
    align

@camilasan
Copy link
Member Author

You write "Fix alignment" but in the screenshot the text is not left-aligned at the same line, but there’s 2 different lines?

I was just trying to align username + status in each of the places where they are displayed. I was not trying to align the top one with the bottom. But I can try to fix it.

@camilasan camilasan force-pushed the bugfix/user-status branch from d974e09 to 5d68484 Compare April 26, 2021 18:47
@camilasan
Copy link
Member Author

camilasan commented Apr 27, 2021 •
edited
Loading

Oh this is great @camilasan! All the cases covered, hopefully no room for confusion anymore. :)

One little feedback from my side: You write "Fix alignment" but in the screenshot the text is not left-aligned at the same line, but there’s 2 different lines? Would look better if it’s all left-aligned along the same line.

  • Fix alignment:
    align

What about now?
aligned

aligned2

@camilasan camilasan force-pushed the bugfix/user-status branch from 2b14fb0 to ac67d79 Compare April 27, 2021 11:47
allexzander
allexzander reviewed Apr 27, 2021
View reviewed changes
src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved
src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved
src/gui/tray/Window.qml Outdated Show resolved Hide resolved
src/gui/tray/Window.qml Outdated Show resolved Hide resolved
mgallien
mgallien reviewed Apr 27, 2021
View reviewed changes
src/gui/tray/UserModel.h Outdated Show resolved Hide resolved
mgallien
mgallien reviewed May 11, 2021
View reviewed changes
src/gui/tray/Window.qml Outdated Show resolved Hide resolved
mgallien
mgallien reviewed May 11, 2021
View reviewed changes
src/gui/userstatus.cpp Outdated Show resolved Hide resolved
@camilasan camilasan force-pushed the bugfix/user-status branch from 37b1104 to 3a0dcea Compare May 17, 2021 19:40
jancborchardt
jancborchardt approved these changes May 18, 2021
View reviewed changes
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.

Looks great now with fixed alignment, thanks @camilasan! :)

mgallien
mgallien requested changes May 18, 2021
View reviewed changes
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.

A few last things
this is really going to be a very nice improvement !

src/gui/userstatus.cpp Outdated Show resolved Hide resolved
src/gui/userstatus.cpp Outdated Show resolved Hide resolved
@camilasan camilasan force-pushed the bugfix/user-status branch 6 times, most recently from 2ca921b to 0ca9f7f Compare May 19, 2021 13:10
mgallien
mgallien reviewed May 19, 2021
View reviewed changes
src/gui/tray/UserLine.qml Outdated
text: statusMessage
elide: Text.ElideRight
color: "black"
font.pixelSize: 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure that is not needed also for the emoji in the previous label ?
I think that we could run into problem if the font inherited from system has a different size than the one you tested with

Copy link

@FlexW FlexW May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we shouldn't force any pixel size. This will look wrong on hidpi.

Copy link
Member Author

@camilasan camilasan May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

OK, the font pixel size is used everywhere at the moment.
I just to want point out that this file is outdated. I am afraid you haven't seen my recent changes - the ones I really would like to get feedback on.

@camilasan
Copy link
Member Author

/rebase

Camila added 19 commits May 25, 2021 06:55
Fix user status message alignment.
1f61d25 
Signed-off-by: Camila <hello@camila.codes>
Move stringToEnum and enumToString to an anonymous namespace.
e3b96dd 
Signed-off-by: Camila <hello@camila.codes>
Check if the user is connected and if the server supports user status.
671b099 
Signed-off-by: Camila <hello@camila.codes>
Remove unused methods in UserModel.
bd65eee 
Signed-off-by: Camila <hello@camila.codes>
Use DesktopNotificationsAllowedRole to check if user is online.
419595c 
Rename ServerUserStatus to ServerUserStatusRole.

Signed-off-by: Camila <hello@camila.codes>
Always display the server url in the drop down account menu.
c793cbe 
Signed-off-by: Camila <hello@camila.codes>
Do not display the user status if there is no pre defined message.
820e97c 
Display the server url instead.

Signed-off-by: Camila <hello@camila.codes>
Rename isDesktopNotificationsAllowed property to desktopNotifications…
f7e9e31 
…Allowed.

- User !== instead of !=.
- Fix code style.

Signed-off-by: Camila <hello@camila.codes>
Check if new value is different from the current one.
93227ab 
In AccountState::setDesktopNotificationsAllowed.

Signed-off-by: Camila <hello@camila.codes>
Fix code style.
dc01b76 
Signed-off-by: Camila <hello@camila.codes>
Fix margins and sizes to align both avatar + user status displays.
4326eb5 
Signed-off-by: Camila <hello@camila.codes>
Do not display the folder status if there are no folders being synced.
828881d 
Signed-off-by: Camila <hello@camila.codes>
Rename ServerUserStatusRole to ServerHasUserStatusRole.
73da140 
- serverHasUserStatus Q_PROPERTY is CONSTANT.
- Fix code style.

Signed-off-by: Camila <hello@camila.codes>
Add Q_UNREACHABLE() after switch statements in UserStatus.
2caa9cc 
Signed-off-by: Camila <hello@camila.codes>
serverUserStatus => serverHasUserStatus.
1adde8d 
Signed-off-by: Camila <hello@camila.codes>
User property isConnected instead of a invokable.
46f7cd3 
Remove unnecessary connection to refreshCurrentUserGui signal.

Signed-off-by: Camila <hello@camila.codes>
Remove signal refreshCurrentUserGui.
1cb7f63 
Use only QML property bindings to refresh the UI.

Signed-off-by: Camila <hello@camila.codes>
Display the user status emoji separated from the message.
8d2264e 
It allows to align the emoji with the message.

Signed-off-by: Camila <hello@camila.codes>
Fix aligment emoji + message on Windows.
f758157 
Signed-off-by: Camila <hello@camila.codes>
@github-actions github-actions bot force-pushed the bugfix/user-status branch from d604c2c to f758157 Compare May 25, 2021 06:55
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-3182-f7581579732642d62766df11d0ec8d751d58e5d2-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@camilasan camilasan merged commit 5d2cfd8 into master May 25, 2021
@camilasan camilasan deleted the bugfix/user-status branch May 25, 2021 07:45
@allexzander
Copy link
Contributor

/backport to stable-3.2

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request May 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@er-vin er-vin er-vin left review comments

@mgallien mgallien mgallien approved these changes

@FlexW FlexW FlexW approved these changes

@allexzander allexzander allexzander approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Desktop 3.2
Development

Successfully merging this pull request may close these issues.
7 participants
@camilasan @jancborchardt @nextcloud-desktop-bot @allexzander @mgallien @er-vin @FlexW