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

Feature/Talk Reply v1 #4200

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Feature/Talk Reply v1 #4200

merged 1 commit into from
Mar 17, 2022

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented Jan 24, 2022 •
edited
Loading

#2908

TO DO:

  • the same message is being sent more than once (I know why)
  • the UI at the moment is meh
  • there is too much logic in the QML (?!)
  • check if server has talk
  • clean up code
  • clean up commit history
  • add new screenshots
  • get this first version merged for 3.5 rc1
    • better error/success handling

To test this

You need Talk on the server - not sure yet against what version. I am testing against sermo.nextcloud com where they have the change Joas made - see #2908 (comment). If you only have Talk you can send a message to the chat, if you have sermo.nextcloud.com you can directly reply to a specific message.

talkreply

@camilasan camilasan marked this pull request as draft January 24, 2022 19:15
@camilasan camilasan changed the title Feature/talk reply Draft: Feature/talk reply Jan 24, 2022
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

45.5% 45.5% Coverage
0.0% 0.0% Duplication

@camilasan camilasan mentioned this pull request Jan 25, 2022
Merged
@jancborchardt
Copy link
Member

@camilasan as you requested, here is the mockup for how reply should look, based on the work done by @allexzander in #4186 :)

Ideally the "Reply" button could animate to morph into the input field if possible (quickly, in 100ms or so). It’s also fine if that’s not possible, but it’s very important that there is no vertical shifting of things. (Also note the reply part would be below the timestamp of the original message instead of inbetween message and timestamp.)
image

@jancborchardt jancborchardt mentioned this pull request Jan 26, 2022
Open
@camilasan camilasan force-pushed the feature/talk-reply branch 3 times, most recently from 14db11e to 1d64965 Compare February 6, 2022 13:53
@tobiasKaminsky tobiasKaminsky linked an issue Feb 7, 2022 that may be closed by this pull request
Add in-line reply functionality to notifications from Nextcloud Talk #2908
Closed
@allexzander
Copy link
Contributor

@camilasan I guess you can rebase it with master and have a final PR ready for review

allexzander
allexzander reviewed Feb 9, 2022
View reviewed changes
src/gui/tray/TalkReply.qml Outdated
id: talkMessage
font.pixelSize: Style.topLinePixelSize
height: activityTextInfo.height
width: parent.width
Copy link
Contributor

Choose a reason for hiding this comment

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

@camilasan @mgallien @claucambra My QML knowledge might be failing me, but, shouldn't we then use anchors.left: parent, anchors.right: parent since we are using anchors.verticalCenter and, should no use TextInput.width in this case?

allexzander
allexzander reviewed Feb 9, 2022
View reviewed changes
src/gui/tray/TalkReply.qml Outdated
width: 24
height: parent.height
opacity: 0.8
anchors.right: rectangleInput.right
Copy link
Contributor

Choose a reason for hiding this comment

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

@camilasan @mgallien @claucambra I have a feeling that we shouldn't mix anchors with width/height?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am actually not sure.

allexzander
allexzander suggested changes Feb 9, 2022
View reviewed changes
src/gui/tray/activitydata.h Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/notificationhandler.cpp
@@ -100,6 +100,17 @@ void ServerNotificationHandler::slotNotificationsReceived(const QJsonDocument &j

//need to know, specially for remote_share
a._objectType = json.value("object_type").toString();

// server 24: notification type chat contains conversationToken/messageId in object_type
if (a._objectType == "chat" || a._objectType == "call") {
Copy link
Contributor

Choose a reason for hiding this comment

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

@camilasan just curious if "room" should also be handled similarly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I will have to check it. I noticed in your recent changes that you were checking for 'room'.

src/gui/tray/notificationhandler.cpp Outdated Show resolved Hide resolved
src/gui/tray/talkreply.h Outdated Show resolved Hide resolved
src/gui/tray/usermodel.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/usermodel.h Outdated Show resolved Hide resolved
@camilasan camilasan force-pushed the feature/talk-reply branch from 55019b2 to c57d70f Compare March 7, 2022 18:02
@camilasan camilasan changed the title Draft: Feature/talk reply Feature/Talk Reply v1 Mar 8, 2022
@camilasan camilasan force-pushed the feature/talk-reply branch from c57d70f to fd79c80 Compare March 8, 2022 17:45
@mgallien
Copy link
Collaborator

works very well a small last comment and we are good to go

please do not forget to clean history

allexzander
allexzander suggested changes Mar 16, 2022
View reviewed changes
src/gui/tray/TalkReplyTextField.qml Outdated Show resolved Hide resolved
src/gui/tray/TalkReplyTextField.qml Outdated Show resolved Hide resolved
src/gui/tray/talkreply.cpp Show resolved Hide resolved
src/gui/tray/usermodel.cpp Show resolved Hide resolved
src/gui/tray/usermodel.h Outdated Show resolved Hide resolved
@camilasan camilasan force-pushed the feature/talk-reply branch 2 times, most recently from bfca41a to 4fda021 Compare March 17, 2022 09:41
allexzander
allexzander reviewed Mar 17, 2022
View reviewed changes
src/gui/tray/TalkReplyTextField.qml Outdated
@@ -37,6 +28,8 @@ Item {
width: 250

onAccepted: root.sendReplyMessage()
onTextEdited: sendReplyMessageButton.enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

@camilasan What if the text will get deleted after being edited? sendReplyMessageButton.enabled will not get reset, right? This sounds a bit complicated. Why not add binding of Text.text !== "" to sendReplyMessageButton.enabled?

@camilasan camilasan force-pushed the feature/talk-reply branch from e44cae3 to 763b828 Compare March 17, 2022 11:42
@camilasan camilasan force-pushed the feature/talk-reply branch 2 times, most recently from b98293d to 0f49ebb Compare March 17, 2022 11:48
@allexzander allexzander self-requested a review March 17, 2022 12:16
@camilasan camilasan force-pushed the feature/talk-reply branch from 0f49ebb to 2c79c61 Compare March 17, 2022 12:34
@allexzander allexzander self-requested a review March 17, 2022 12:43
allexzander
allexzander suggested changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@allexzander allexzander left a comment

Choose a reason for hiding this comment

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

I was too quick to approve. Buttons on notifications are broken.

image

@camilasan camilasan force-pushed the feature/talk-reply branch from 2c79c61 to 01b27f0 Compare March 17, 2022 15:52
@camilasan
Copy link
Member Author

I was too quick to approve. Buttons on notifications are broken.

So I did this now:
state-1

@camilasan camilasan force-pushed the feature/talk-reply branch from 01b27f0 to cfda8ba Compare March 17, 2022 16:20
@allexzander allexzander self-requested a review March 17, 2022 16:36
Add TalkReply class and tests.
73bae8c 
- Add struct TalkNotificationData to handle token and messageId.
- Handle chat and call notifications with the new struct.
- Add talk token and messageId to data roles in ActivityListModel.
- Add Talk Reply component to the ActivityList.
- User Loader to display the TalkReply component.
- Move Talk Reply from ActivityItem to ActivityItemContent due to PR #4186.
- Use TextField instead of Text.
- Disable send reply button instead of changing border color when field is empty.

Signed-off-by: Camila <hello@camila.codes>
@camilasan camilasan force-pushed the feature/talk-reply branch from cfda8ba to 73bae8c Compare March 17, 2022 16:50
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

58.5% 58.5% Coverage
0.0% 0.0% Duplication

@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4200-73bae8cd309e555d3caf932ffec31906c1f4b4cd-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.

@mgallien
Copy link
Collaborator

/rebase

@mgallien mgallien merged commit d2b67ff into master Mar 17, 2022
@mgallien mgallien deleted the feature/talk-reply branch March 17, 2022 22:36
@mgallien mgallien added this to the 3.5.0 milestone Mar 17, 2022
@joshtrichards joshtrichards mentioned this pull request Nov 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgallien mgallien mgallien approved these changes

@claucambra claucambra claucambra requested changes

@allexzander allexzander allexzander approved these changes

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

Assignees

@camilasan camilasan

Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

Add in-line reply functionality to notifications from Nextcloud Talk
6 participants
@camilasan @jancborchardt @allexzander @mgallien @nextcloud-desktop-bot @claucambra