Repository navigation
Status feature - #2505
Merged
Merged
Status feature#2505
Conversation
camilasan
marked this pull request as draft
October 3, 2020 15:19
er-vin
reviewed
Oct 5, 2020
er-vin
left a comment
er-vin
left a comment
Member
There was a problem hiding this comment.
"Obviously" UserStatus looks like a good target for tests. Needs stubbing the server part somehow, I think we got something which might help there. We got that unused mockserver thing... maybe needs to be revived a bit.
camilasan
force-pushed
the
status-feature
branch
from
October 6, 2020 14:33
6a5803e to
00605bb
Compare
nicolasfella
reviewed
Oct 6, 2020
er-vin
requested changes
Oct 7, 2020
camilasan
force-pushed
the
status-feature
branch
from
October 7, 2020 17:51
00605bb to
d56655b
Compare
er-vin
reviewed
Oct 8, 2020
camilasan
force-pushed
the
status-feature
branch
from
November 3, 2020 11:19
d56655b to
fc62de2
Compare
Member
|
@camilasan feel free to post screenshots if you need more design feedback. :) |
4 tasks done
camilasan
force-pushed
the
status-feature
branch
3 times, most recently
from
March 17, 2021 22:20
4b8cd69 to
a89f27f
Compare
camilasan
marked this pull request as ready for review
March 17, 2021 22:49
FlexW
reviewed
Mar 18, 2021
allexzander
reviewed
Mar 18, 2021
camilasan
force-pushed
the
status-feature
branch
3 times, most recently
from
March 18, 2021 15:22
684a574 to
934d6a7
Compare
allexzander
reviewed
Mar 18, 2021
allexzander
reviewed
Mar 18, 2021
camilasan
force-pushed
the
status-feature
branch
from
March 19, 2021 11:09
934d6a7 to
fb08b5d
Compare
Contributor
|
@camilasan Any chance you can fix those clang-tidy errors? |
er-vin
reviewed
Mar 19, 2021
camilasan
force-pushed
the
status-feature
branch
2 times, most recently
from
March 19, 2021 18:16
aa920ef to
0b4b8e0
Compare
allexzander
reviewed
Mar 22, 2021
allexzander
reviewed
Mar 22, 2021
allexzander
reviewed
Mar 22, 2021
allexzander
reviewed
Mar 22, 2021
camilasan
force-pushed
the
status-feature
branch
2 times, most recently
from
March 23, 2021 07:49
4344b41 to
6134e2b
Compare
allexzander
reviewed
Mar 23, 2021
allexzander
reviewed
Mar 23, 2021
Contributor
|
@camilasan I guess, those clang-tidy issues need to be addressed too :) |
camilasan
force-pushed
the
status-feature
branch
from
March 23, 2021 10:59
20cde38 to
9adaafe
Compare
Contributor
|
@camilasan @er-vin @FlexW Maybe we should add tests after this PR is merged? |
camilasan
force-pushed
the
status-feature
branch
2 times, most recently
from
March 24, 2021 14:23
7b73840 to
34ff16d
Compare
added 4 commits
March 24, 2021 15:39
- AccountState controls UserStatus. - Display user status in the system tray menu next to user's avatar. Signed-off-by: Camila <[email protected]>
- This information is retrieved from the notifications endpoint. - Add icons for the different pre defined status. - Make functions available to QML to decide which status icon to display. - Display the user status icon on the avatar and move the online/offline connection status to the folder icon. Signed-off-by: Camila <[email protected]>
- User QHash to map status strings and Status enum. Signed-off-by: Camila <[email protected]>
Signed-off-by: Camila <[email protected]>
camilasan
force-pushed
the
status-feature
branch
from
March 24, 2021 14:43
34ff16d to
9219926
Compare
allexzander
approved these changes
Mar 24, 2021
|
AppImage file: Nextcloud-PR-2505-9219926a74b954b5915f2a9e4e3900604aca577b-x86_64.AppImage |
er-vin
reviewed
Mar 24, 2021
| } | ||
|
|
||
| void User::slotRefreshUserStatus() { | ||
| // TODO: check for _account->account()->capabilities().userStatus() |
| * @brief desktopNotificationStatusReceived - signal to report if notifications are allowed | ||
| * @param status - set desktop notifications allowed status | ||
| */ | ||
| void allowDesktopNotificationsChanged(const bool isAllowed); |
Member
There was a problem hiding this comment.
This strikes me as strange design wise that this sits on the very general JsonApiJob... I'd have expected the corresponding header to be checked in UserStatus (IIRC you should have access to the reply there).
Member
|
@er-vin I always wondered about this, if it makes sense to make primitive
(base) types `const`. In theory, you could modify `isAllowed` in the
function if it is not const so there would be some benefit if one makes
them `const`.
Yes, this would avoid this potential mistake on the implementation side.
The downside I see is that you leak an implementation detail ("I shall not
modify this parameter internally") through the API of the class, which means
that once that implementation changes you might need to modify the header and
get every other type depending on this class to be rebuilt.
As far as API is concerned, there's also the consistency argument: it's
extremely rarely done, so it's more noise for little gain. Since I generally
consider APIs to be most important (long story, for another time), that
explains my bias. ;-)
Another thing to take into consideration for libraries is: would that change
(const to non const) be binary compatible? I think it would be on most
platforms but I'd have to check, typically I always have doubts regarding MSVC
for this kind of things. Granted this doesn't quite apply to this project. :-)
|
8 of 9 tasks
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix part of #2318:
Do not disturb:
Online:
Online with no message:
Away:
Invisible:
Offline (no status set):
Connected:
Disconnected:
