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

The status label at the bottom of the RoomsList are supposed to be corrected #188 #248

Closed
wants to merge 3 commits into from

Conversation

aaravlu
Copy link
Contributor

@aaravlu aaravlu commented Nov 10, 2024

Here #188
It should be normal right now, please review.
Don't know whether you like the label style below, just tell me if not suitable.
Screenshot from 2024-11-10 11-49-20

@aaravlu aaravlu marked this pull request as ready for review November 10, 2024 03:55
@aaravlu aaravlu changed the title The status label at the bottom of the RoomsList seems to be corrected #188 The status label at the bottom of the RoomsList are supposed to be corrected #188 Nov 10, 2024
@tyreseluo tyreseluo added the waiting-on-review This issue is waiting to be reviewed label Nov 11, 2024
@kevinaboos
Copy link
Member

Thanks for the PR, but this is not the correct fix for this problem. Also, you've made a few assumptions that are not necessarily correct:

  1. Just because a room isn't yet known about on the client, does not mean it is empty. So your above screenshot that says "Loaded 7 of 16 rooms, 9 are empty" is definitely not correct.
  2. If self.max_known_rooms is None, that does not mean that the client's internet connection is bad. Sometimes a server will just decide to not send the max number of known rooms, or it may change over time.
  • Furthermore, Robrix would never reach this point of code in its execution flow if the internet connection was not working, so this just doesn't make sense.

Please be very careful with your assumptions when displaying status/error messages to the user.

I think the root cause of #188 is that certain rooms list updates are not getting delivered, so this is more than just a simple display problem. We need to look into that more carefully, and it is not this simple/easy to diagnose and fix.

@kevinaboos kevinaboos closed this Nov 11, 2024
@kevinaboos kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Nov 11, 2024
@aaravlu
Copy link
Contributor Author

aaravlu commented Nov 12, 2024

Thanks for explanation.

@aaravlu aaravlu deleted the fix188 branch December 30, 2024 06:31
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