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

Implement replying to messages and reply previews #104

Merged
merged 6 commits into from
Aug 23, 2024
Merged

Implement replying to messages and reply previews #104

merged 6 commits into from
Aug 23, 2024

Conversation

fmzbl
Copy link
Contributor

@fmzbl fmzbl commented Aug 7, 2024 •
edited by kevinaboos
Loading

implements:

still fixing some small todos and styles, but core functionality is done.

[From Kevin]: before this gets merged in, we need to:

  • fix reply button styling and direction
  • fix the issues with usernames and message content not displaying, which is related to how Robrix carefully caches already-drawn timeline events. I'll tackle this shortly.
  • fix remaining to-dos about styling buttons and other UI items.
  • fix horizontal alignment of Condensed* messages -- they're no longer left-aligned with regular messages
  • set cursor: Hand for the ReplyingPreview widget to make it more obvious that they're clickable w/ actions.
    • Huh, this was actually already set, but it's not displaying the cursor as intended. Probably an issue with PortalList or Makepad's hover actions, which have been fixed in a recent version that Robrix hasn't yet upgraded to.
  • for the action ReplyPreviewClicked, we need a few things:
    • include the unique event ID to handle the case where the replied-to message hasn't been retrieved from the server (via pagination) and isn't yet locally available.
      • This is actually quite complex to deal with, so maybe we can save it for a future issue, and for now we can simply log a warning that says something like "TODO: jumping back to a replied-to message that isn't yet locally retrieved is currently unsupported"
    • a smooth scrolling action that scrolls upwards to that message rather than an abrupt jump (Makepad's PortalList may not yet support this?)
    • highlighting or temporarily flashing the background (maybe light blue or green) to indicate which specific message was the one being shown in the just-clicked ReplyingPreview
  • fix avatar font sizing in ReplyingPreview to be properly scaled down.
  • shift the inline reply preview (the one above the message) to the right a bit such that the vertical bar is aligned with the middle of the avatar picture.
  • Refactor the populate_message* logic such that it can be easily re-used for populating the content of a ReplyingPreview. I will also tackle this.

@kevinaboos
Copy link
Member

thanks! let me know when you're done so I can review it (and you can also request a review directly from me here too)

@fmzbl fmzbl requested a review from kevinaboos August 19, 2024 18:20
@fmzbl fmzbl marked this pull request as ready for review August 19, 2024 18:20
@fmzbl
Copy link
Contributor Author

fmzbl commented Aug 19, 2024

there are a few stuff that may seem ugly: they probably are. using the matrix sdk/api was a real pain, and took me a lot of time and refactoring to get something that works and tries to fit with the rest of the code.

@fmzbl
Copy link
Contributor Author

fmzbl commented Aug 19, 2024

i left an issue(#110) for adding the floating menus when we have support inside of makepad

@kevinaboos
Copy link
Member

kevinaboos commented Aug 22, 2024 •
edited
Loading

Thanks, this is an awesome start! I've cleaned up some unrelated formatting issues, fixed the reply button styling (with a new icon), and resolved the merge conflicts by merging in the latest changes from main.

In the future, kindly try to avoid making tons of formatting changes and committing them, as it makes it very difficult to review what's changed. Also, please don't force-push any more commits as it will invalidate the review history here on github. thanks!

EDIT: moved the to-do list that was here to the top of this issue so they're trackable as work-items.

@fmzbl
Copy link
Contributor Author

fmzbl commented Aug 22, 2024

In the future, kindly try to avoid making tons of formatting changes and committing them, as it makes it very difficult to review what's changed. Also, please don't force-push any more commits as it will invalidate the review history here on github. thanks!

yeah, i tried to remove every formatting change that i saw. maybe we can have a rustfmt.toml?

@kevinaboos
Copy link
Member

yea we'll definitely do that in the near future. That, and clippy lints.

@kevinaboos
rename ReplyingPreview --> ReplyPreview, add docs.
96758a3 
Fix avatar font size in `ReplyPreview`
@kevinaboos
fix the issue of message profiles/avatars not being properly written
923eb39 
There is a "bug" (maybe a bug) in Makepad where if you give multiple
DSL elements the same identifier name, then the widget reference you get
when querying it with `id!(...)` may be for a different widget
than the one you expect.

This commit renames the subwidgets of `ReplyPreview` in the DSL
in order to ensure they do not overlay with the same elements
that are part of a `Message` itself.

It also completely restructures the drawing logic for ReplyPreviews
to be done in a separate function and then to be called in the correct
place, i.e., when the item has not been cached.

The "fully-drawn" status of a Message's ReplyPreview is now factored in
to the conditional that determines whether a given message is
considered to be fully drawn (for the purpose of caching).
@kevinaboos
Copy link
Member

fixed the issue of message username/profile info not being properly populated.

@kevinaboos
Fetch event details on a missing InReplyToDetails
2879df3 
as well as for missing user profile info
@kevinaboos
Copy link
Member

fixed all other non-UI issues.

kevinaboos
kevinaboos approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks! I'm going to go ahead and merge this because it is in an acceptable state.

I will file a follow-up issue to track the remaining to-do items, and will also leave issues #82 and #83 open until those UI fixes are complete.

@kevinaboos kevinaboos merged commit 5dd4f4a into project-robius:main Aug 23, 2024
1 check passed
This was referenced Sep 6, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinaboos kevinaboos kevinaboos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Robrix
Status: Done
Milestone
Stage 1: basic Matrix features
Development

Successfully merging this pull request may close these issues.
2 participants
@fmzbl @kevinaboos