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

Add replying and editing support #848

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add replying and editing support #848

wants to merge 2 commits into from

Conversation

rpallai
Copy link
Contributor

@rpallai rpallai commented Feb 8, 2023

Cc #596
Closes #447

@rpallai rpallai force-pushed the replying-and-editing branch 2 times, most recently from 60c550f to 2d66b9b Compare February 9, 2023 10:19
@KitsuneRal KitsuneRal added the enhancement A feature or change request for Quaternion label Feb 10, 2023
@KitsuneRal KitsuneRal self-requested a review February 10, 2023 07:14
@rpallai rpallai force-pushed the replying-and-editing branch 3 times, most recently from 55edb93 to a817aec Compare February 16, 2023 15:38
@rpallai rpallai force-pushed the replying-and-editing branch 3 times, most recently from 71734cc to b59f66e Compare February 21, 2023 11:54
KitsuneRal
KitsuneRal requested changes Apr 16, 2023
View reviewed changes
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry for taking so long to review it. Overall LGTM, just a few nitpicks.

client/chatroomwidget.cpp Outdated Show resolved Hide resolved
client/chatroomwidget.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.cpp Outdated Show resolved Hide resolved
client/models/messageeventmodel.h Outdated Show resolved Hide resolved
client/chatroomwidget.cpp Outdated Show resolved Hide resolved
@rpallai
Copy link
Contributor Author

rpallai commented Apr 17, 2023

Rebased to HEAD and all suggestions from code review have been applied.

@KitsuneRal
Copy link
Member

Thanks for the update! I just tried using it actually and it's a bit rough around a few edges:

  1. The unblended highlight colour is not a good choice for the background; its intended use is for foreground elements. I would suggest blending it with the base color or use RectangularGlow or another kind of frame to mark the edited/replied message.
  2. Pressing some keys (Esc, e.g.) when editing/replying removes the HUD text; the icon, the message selection, and the actual mode remain though. I would suggest using just one way to convey the mode (and I think the QToolButton next to the entry is better for that). And please make Esc actually reset the mode - it's the way Element Web works, in particular.

@KitsuneRal KitsuneRal mentioned this pull request Apr 18, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KitsuneRal KitsuneRal KitsuneRal requested changes

Assignees
No one assigned
Labels
enhancement A feature or change request for Quaternion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support replying
2 participants
@rpallai @KitsuneRal