project-robius / robrix

Robrix: a multi-platform Matrix chat client written in Rust using the Makepad UI toolkit and the Robius app dev framework
MIT License
107 stars 18 forks source link

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

Open Demolemon11 opened 1 week ago

Demolemon11 commented 1 week ago

issue231

Demolemon11 commented 1 week ago

Screencast from 2024-11-15 10-34-45.webm

I will never give up, please review:)

alanpoon commented 1 week ago

https://github.com/user-attachments/assets/cc8531eb-d67e-4773-a288-f8c3fa40a4c6

Here are some issues I found:

alanpoon commented 1 week ago

/

/

  1. 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.

Demolemon11 commented 4 days ago

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 commented 4 days ago

@CodiumAI-Agent /review

CodiumAI-Agent commented 4 days ago

PR Reviewer Guide ๐Ÿ”

(Review updated until commit https://github.com/project-robius/robrix/commit/b700faad885a73329655de5a70601a2c998d89db)

Here are some key observations to aid the review process:

**๐ŸŽซ Ticket compliance analysis โœ…** **[231](https://github.com/project-robius/robrix/issues/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.
alanpoon commented 4 days ago

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 commented 4 days ago

Persistent review updated to latest commit https://github.com/project-robius/robrix/commit/b700faad885a73329655de5a70601a2c998d89db

ZhangHanDong commented 4 days ago

@Demolemon11

Core requirements:

Implementation details:

Suggestions for improvement:

Code improvement suggestions:

kevinaboos commented 3 days ago

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.

alanpoon commented 1 day ago
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
  2. The Permission banner is buggy. It went into another room.