Set user status by FlexW · Pull Request #3639 · nextcloud/desktop

FlexW · 2021-08-09T14:24:12Z

Fixes a part of #2318
Related #3045

Set the user status from the main dialog:

A dialog to set a custom user status:

Emoji picker:

Note to myself: Upgrade to Qt 5.15 before this can be merged.

jancborchardt · 2021-08-09T15:38:11Z

Very cool! As usual, mostly detail feedback on design:

"Set user status" should be first in action menu
Wording: "Set status" rather than "Set user status", like on Android

In the dialog:

Can the text of the "Online status" buttons have the icons on the left?
The status message emoji button would look better square?
There should be less whitespace between custom status message input and the 5 suggestions as they belong together
Is it possible to not make the 5 suggestions be styled like buttons, because that’s a bit much?
~~And possibly the suggestions could be left-aligned?~~ → Align the button text/icon to the left is not easily possible in Qml. We would need to design our own button. I would prefer to avoid that now because it is too much work for the added value.
Is it possible to style "Set status message" as a sort of primary button? Otherwise all buttons look the same.

FlexW · 2021-08-17T15:10:01Z

@jancborchardt Does that work?

Align the button text/icon to the left is not easily possible in Qml. We would need to design our own button. I would prefer to avoid that now because it is too much work for the added value.

jancborchardt · 2021-08-18T12:39:00Z

@FlexW very nice! :) I converted the feedback above into checkboxes, can you check off if that’s done?

And the dialog could use a little bit more vertical whitespace between the logical sections and on the top, like so:

FlexW · 2021-08-19T11:57:31Z

@jancborchardt done

jancborchardt

Super nice! :)

mgallien

expect more review rounds after this one

mgallien · 2021-08-23T15:01:05Z

+
+SetUserStatusDialogModel::SetUserStatusDialogModel(QObject *parent)
+    : QObject(parent)
+    , _dateTimeProvider(new DateTimeProvider)


use std::make_unique<DateTimeProvider>()

What is the benefit?

the object dynamically allocated by std::make_unique<DateTimeProvider>() is already managed by an instance of std::unique_ptr so it can never leak
direct usage of new can lead to a leak if there is something happening like an exception being fired between the call to new and the call to the constructor of the std::unique_ptr

Not sure because that is the implementation of std::make_unique() in my stl:

/// std::make_unique for single objects template<typename _Tp, typename... _Args> inline typename _MakeUniq<_Tp>::__single_object make_unique(_Args&&... __args) { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }

Seems to be just that what I do.

mgallien · 2021-08-23T15:01:22Z

+    , _userStatusJob(std::move(userStatusJob))
+    , _userStatus("no-id", "", "😀",
+          UserStatus::OnlineStatus::Online, false, {})
+    , _dateTimeProvider(new DateTimeProvider)


use std::make_unique<DateTimeProvider>()

Same comment

mgallien · 2021-08-23T15:02:01Z

+
+Q_LOGGING_CATEGORY(lcUserStatusDialogModel, "nextcloud.gui.userstatusdialogmodel", QtInfoMsg)
+
+SetUserStatusDialogModel::SetUserStatusDialogModel(QObject *parent)


this will leave the object in a working state or an invalid one ?

Invalid. But it's needed to make it available for Qml.

allexzander · 2021-08-27T08:51:02Z

@FlexW Left a few comments. Some of them could be outdated as I have been reviewing it commit-by-commit. Feel free to ignore those. The functionality seems to be working. Tested locally.

FlexW · 2021-09-08T11:12:19Z

/rebase

allexzander

Other than emoji not being displayed in the button and minor issues, I think it is good to go as version 1.

Signed-off-by: Felix Weilbach <[email protected]>

nextcloud-desktop-bot · 2021-09-09T09:26:08Z

AppImage file: Nextcloud-PR-3639-8a8d488454405356b5d11f63bebff2d69be43b02-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.

FlexW requested review from allexzander, camilasan, jancborchardt and mgallien August 9, 2021 14:24

FlexW force-pushed the feature/set-user-status branch 2 times, most recently from 874f7cf to d5675a9 Compare August 9, 2021 15:10

splitt3r reviewed Aug 10, 2021

View reviewed changes

Comment thread src/gui/emojimodel.cpp Outdated

mgallien reviewed Aug 10, 2021

View reviewed changes

Comment thread src/gui/SetUserStatusView.qml Outdated

Comment thread src/gui/SetUserStatusView.qml Outdated

Comment thread src/gui/EmojiPicker.qml

Comment thread src/gui/userstatusdialogmodel.h Outdated

tobiasKaminsky mentioned this pull request Aug 11, 2021

Feature Parity: User Status nextcloud/server#22452

Closed

4 tasks done

FlexW force-pushed the feature/set-user-status branch from 127d2ad to cbad873 Compare August 13, 2021 10:14

FlexW force-pushed the feature/set-user-status branch 5 times, most recently from cbafdf3 to 2dd8012 Compare August 18, 2021 10:02

FlexW marked this pull request as ready for review August 18, 2021 10:03

FlexW force-pushed the feature/set-user-status branch from 2dd8012 to c00bbda Compare August 18, 2021 10:51

FlexW force-pushed the feature/set-user-status branch from c00bbda to 87677a8 Compare August 19, 2021 11:52

FlexW force-pushed the feature/set-user-status branch from 87677a8 to 33874b3 Compare August 19, 2021 11:58

jancborchardt approved these changes Aug 19, 2021

View reviewed changes

jancborchardt requested a review from nimishavijay August 19, 2021 12:14

FlexW force-pushed the feature/set-user-status branch 3 times, most recently from be0d4d9 to ccb1665 Compare August 19, 2021 13:04

tobiasKaminsky requested a review from mgallien August 23, 2021 10:41

mgallien requested changes Aug 23, 2021

View reviewed changes

allexzander reviewed Aug 27, 2021

View reviewed changes

Comment thread src/libsync/ocsuserstatusconnector.cpp

allexzander reviewed Aug 27, 2021

View reviewed changes

Comment thread src/gui/ErrorBox.qml

FlexW force-pushed the feature/set-user-status branch 2 times, most recently from db5f453 to 0794266 Compare September 1, 2021 11:12

FlexW requested a review from mgallien September 1, 2021 11:14

tobiasKaminsky linked an issue Sep 3, 2021 that may be closed by this pull request

Status feature in the desktop client #2318

Closed

FlexW force-pushed the feature/set-user-status branch from 0794266 to aa7f148 Compare September 3, 2021 13:43

mgallien reviewed Sep 6, 2021

View reviewed changes

Comment thread src/gui/EmojiPicker.qml Outdated

mgallien reviewed Sep 6, 2021

View reviewed changes

Comment thread src/gui/userstatusselectormodel.h

FlexW force-pushed the feature/set-user-status branch 6 times, most recently from ade8341 to aebea6b Compare September 8, 2021 08:52

mgallien approved these changes Sep 8, 2021

View reviewed changes

nextcloud-command force-pushed the feature/set-user-status branch from 55e094b to c6e961f Compare September 8, 2021 11:13

allexzander reviewed Sep 8, 2021

View reviewed changes

Comment thread src/gui/socketapi/socketapi.cpp

allexzander self-requested a review September 8, 2021 15:03

allexzander approved these changes Sep 8, 2021

View reviewed changes

Add dialog to set user status

8a8d488

Signed-off-by: Felix Weilbach <[email protected]>

FlexW force-pushed the feature/set-user-status branch from c1d6e77 to 8a8d488 Compare September 9, 2021 09:21

FlexW merged commit ea6a56d into master Sep 9, 2021

FlexW deleted the feature/set-user-status branch September 9, 2021 09:52

FlexW added this to the 3.3.4 milestone Sep 9, 2021

mgallien modified the milestones: 3.3.4, 3.4.0 Sep 20, 2021


		Q_LOGGING_CATEGORY(lcUserStatusDialogModel, "nextcloud.gui.userstatusdialogmodel", QtInfoMsg)

		SetUserStatusDialogModel::SetUserStatusDialogModel(QObject *parent)

Conversation

FlexW commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt commented Aug 9, 2021 • edited by FlexW Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FlexW commented Aug 17, 2021

Uh oh!

jancborchardt commented Aug 18, 2021

Uh oh!

FlexW commented Aug 19, 2021

Uh oh!

jancborchardt left a comment

Choose a reason for hiding this comment

Uh oh!

mgallien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allexzander commented Aug 27, 2021

Uh oh!

Uh oh!

Uh oh!

FlexW commented Sep 8, 2021

Uh oh!

Uh oh!

allexzander left a comment

Choose a reason for hiding this comment

Uh oh!

nextcloud-desktop-bot commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

FlexW commented Aug 9, 2021 •

edited

Loading

jancborchardt commented Aug 9, 2021 •

edited by FlexW

Loading