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

render roomlist under people's header #174

Closed
wants to merge 5 commits into from

Conversation

alanpoon
Copy link
Contributor

@alanpoon alanpoon commented Oct 7, 2024

#139

  • Created a new sigqueue for room_update: PENDING_IS_DIRECT_ROOM_UPDATES
  • Added is_direct boolean to differentiate between rooms belong to which header
  • Added new property for RoomsList widget: room_type

Currently, for some reasons, the program will hang when 2 portallists in RoomsList widget being drawn. (Fixed hang issue)

@alanpoon alanpoon marked this pull request as ready for review October 7, 2024 09:42
@alanpoon
Copy link
Contributor Author

alanpoon commented Oct 7, 2024

Fixed Crash
Screenshot 2024-10-07 172839

@aaravlu
Copy link
Contributor

aaravlu commented Oct 7, 2024

Look nice! : )
It seems that you impl it in the drawing layer

@kevinaboos
Copy link
Member

Thanks. As I mentioned previously in a different issue/PR, I don't believe this approach is going in the correct direction, unfortunately. Here are a few problems with this:

  • Storage of data vs. presentation of data are separate concerns, but this approach conflates those two things.
    • That is, the backend module here should not have to care about whether a room is direct or not, or whether the UI is displaying them separately. The backend should just provide all information to the frontend UI main thread, who can decide what to do with that info and how to display it.
  • Using two separate queues for sending updates from the background to the same widget is not only inefficient, it's overly complex and invites mistakes. Making all future devs deal with two redundant queues like this is a recipe for unmaintainability, as someone will inevitably forget to poll one queue or push to the proper queue.
  • A user may not actually care about separating rooms into different kinds, e.g., direct messages vs regular public rooms, etc, so they shouldn't pay the cost of that overhead if they don't want to. This PR forces that display choice in the actual backend, which is a strange choice.
  • We should not be storing application data-related globals in Cx (just my opinion, but I don't think that's a good use case for them).

There are a bunch more points to be made here, but I think that suffices. The good news is that I think we have different widgets available now that will make this easier, and we should use those. I can't recall who had discussed that, but Julian may know.

To reiterate, we do not need to restructure the backend data or communication paths to support this feature. We merely need to draw the rooms differently in the UI main thread only.

@kevinaboos
Copy link
Member

Closing per my above comment. Feel free to re-open later, but also note that I am restructuring how rooms are stored in the list of all rooms as per #185

@kevinaboos kevinaboos closed this Oct 30, 2024
@kevinaboos
Copy link
Member

See my latest PR #224 for more info on how I intended this design to work, which includes precise examples of the "storage vs. presentation" argument I made above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants