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.

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

Hide message text input bar if the user cannot send messages in a room #253

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

aaravlu
Copy link
Contributor

@aaravlu aaravlu commented Nov 13, 2024

@aaravlu aaravlu changed the title Don't show message text input bar if the user cannot send messages in a room #231 Hide message text input bar if the user cannot send messages in a room #231 Nov 13, 2024
src/home/room_screen.rs Outdated Show resolved Hide resolved
@tyreseluo tyreseluo added the waiting-on-review This issue is waiting to be reviewed label Nov 13, 2024
src/sliding_sync.rs Outdated Show resolved Hide resolved
src/home/room_screen.rs Outdated Show resolved Hide resolved
src/home/room_screen.rs Outdated Show resolved Hide resolved
src/home/room_screen.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 14, 2024
@aaravlu
Copy link
Contributor Author

aaravlu commented Nov 15, 2024

Screencast.from.2024-11-15.10-34-45.webm

I will never give up, please review:)

@tyreseluo tyreseluo added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 15, 2024
@alanpoon
Copy link
Contributor

Untitled.video.-.Made.with.Clipchamp.mp4

Here are some issues I found:

  • Currently this banner "you don't have permission" is applying to all rooms when the user is muted in a room.
  • The error message not handled when the user sends out message when he is muted in a room after he joins the room

@alanpoon alanpoon self-requested a review November 15, 2024 09:17
@alanpoon
Copy link
Contributor

alanpoon commented Nov 15, 2024

/

/
2. User Ban is timeline event: "m.room.power_levels" https://matrix-org.github.io/matrix-rust-sdk/matrix_sdk/ruma/events/enum.AnyStateEventContent.html#variant.RoomPowerLevels

My advice is to always check the behavior of the element.io for the correct api to use. Use two accounts, both in the same room. One mutes the other, you should already see the message text input bar hidden for the muted user. Hence there is no "can_user_send_message" checks in between.

@aaravlu
Copy link
Contributor Author

aaravlu commented Nov 18, 2024

My advice is to always check the behavior of the element.io for the correct api to use. Use two accounts, both in the same room. One mutes the other, you should already see the message text input bar hidden for the muted user. Hence there is no "can_user_send_message" checks in between.

Done.

@ZhangHanDong
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Nov 18, 2024

PR Reviewer Guide 🔍

(Review updated until commit b700faa)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

231 - Fully compliant

Fully compliant requirements:

  • Hide the message input bar if the user does not have permission to send messages in a room.
  • Display a notice informing the user that they cannot send messages if they lack permissions.
  • Check permission status every time the user opens the room.
  • Subscribe to room state changes to update permission status dynamically.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Code Clarity
The implementation of permission checks and UI updates could be refactored for better clarity and maintainability.

@aaravlu aaravlu changed the title Hide message text input bar if the user cannot send messages in a room #231 Hide message text input bar if the user cannot send messages in a room Nov 18, 2024
@alanpoon
Copy link
Contributor

alanpoon commented Nov 18, 2024

My advice is to always check the behavior of the element.io for the correct api to use. Use two accounts, both in the same room. One mutes the other, you should already see the message text input bar hidden for the muted user. Hence there is no "can_user_send_message" checks in between.

Done.

Nope, you need to also inspect the network using browser dev tool and understand matrix api.
anyevent

  1. Handle RoomPowerLevel change https://docs.rs/matrix-sdk-ui/0.7.0/matrix_sdk_ui/timeline/enum.AnyOtherFullStateEventContent.html here in sliding_sync.rs line 1632.
  2. Remove the check_user_permission function from the process_timeline_updates function
  3. Only call can_send_message async function when entering the room. Nothing to do with timeline.

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit b700faa

@ZhangHanDong
Copy link
Contributor

@Demolemon11

Core requirements:

  • ✅ Correctly implemented the visibility logic for the message input control
  • ✅ Implemented permission checks and controlled the UI based on the results
  • ✅ Implemented clear prompts for when messages cannot be sent

Implementation details:

  • ✅ Used Room:can_user_send_message() to check permissions
  • ✅ Displays a prompt instead of the input box when permissions are insufficient

Suggestions for improvement:

  • ❌ Lacks a subscription mechanism for permission changes
  • ❌ Lacks unsubscribe logic when leaving the room
  • ❌ Only checks permissions once when opening the room, without periodic checks

Code improvement suggestions:

  • Using i64 type for permission values is not intuitive. Please refactor the permission status to a clearer enum type.
  • Missing documentation comments for permission status, please add them.
  • Permission check logic is scattered in multiple places, please centralize the management of permission-related logic.
  • Lacks an event listening mechanism for permission changes.
  • Please add complete error handling.
  • Need to set a reasonable time interval for permission checks.
  • Need to optimize the timing of state updates.

@alanpoon alanpoon added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 19, 2024
@kevinaboos
Copy link
Member

Suggestions for improvement:

  • ❌ Lacks a subscription mechanism for permission changes

this can be done in a future PR, too, if you want to keep this one simple/manageable. Multiple smaller PRs are preferred over one big PR.

  • ❌ Lacks unsubscribe logic when leaving the room

Same comment as above.

  • ❌ Only checks permissions once when opening the room, without periodic checks

I think this is the same thing as the first point. As I mentioned in one of my earlier comments, a subscription mechanism that asynchronously awaits updates to a user's power levels is much better than periodically checking it from the main UI thread, since updates to user power levels are very infrequent.

src/sliding_sync.rs Outdated Show resolved Hide resolved
@alanpoon
Copy link
Contributor

alanpoon commented Nov 21, 2024

another_room buggy

The Permission banner based subscription of the power level change works.

  1. For the Power change message, add "from Default to Custom (-1)" Following Element.io

/

@alanpoon alanpoon added the waiting-on-review This issue is waiting to be reviewed label Nov 26, 2024
@aaravlu aaravlu requested a review from ZhangHanDong November 27, 2024 05:52
@alanpoon alanpoon added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 30, 2024
src/home/room_screen.rs Outdated Show resolved Hide resolved
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.

your name choices for functions and variables are quite inconsistent. You have all of the following:

  • CheckUserPostPermission
  • check_user_post_permission
  • CanUserPost
  • can_user_post
  • no_send_permission_notice
  • NO_POST_PERMISSION_NOTICE
  • "user send permission"

There are too many different names here for what is all the same thing. In this case, let's follow what the Matrix SDK does. Specifically, they use the phrase "can user send message", so let's use variants of that phrase everywhere else, instead of all the different names/phrases that I listed above from your current code.

src/home/room_screen.rs Outdated Show resolved Hide resolved
src/home/room_screen.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Dec 16, 2024
@aaravlu aaravlu added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Dec 17, 2024
@aaravlu aaravlu requested a review from kevinaboos December 17, 2024 03:44
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 for the naming fixes, things are much better now. Just a few more minor comments.

src/sliding_sync.rs Outdated Show resolved Hide resolved
src/home/room_screen.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
src/sliding_sync.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Dec 17, 2024
@aaravlu aaravlu added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review This issue is waiting to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants