Skip to content

Improve ServerChooser UI and use server avatar#6990

Open
TimoPtr wants to merge 5 commits into
mainfrom
feature/refactor_server_chooser
Open

Improve ServerChooser UI and use server avatar#6990
TimoPtr wants to merge 5 commits into
mainfrom
feature/refactor_server_chooser

Conversation

@TimoPtr

@TimoPtr TimoPtr commented Jun 10, 2026

Copy link
Copy Markdown
Member

Summary

This PR refactor the ServerChooser to be more modern and migrate to our new theme. While at it this PR also add the avatar of the server and make the bottom sheet more friendly.

Checklist

  • New or updated tests have been added to cover the changes following the testing guidelines.
  • The code follows the project's code style and best_practices.
  • The changes have been thoroughly tested, and edge cases have been considered.
  • Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Screenshots

image image image

Any other notes

This is based on #6989

@TimoPtr TimoPtr requested a review from jpelgrom June 10, 2026 09:34
@TimoPtr TimoPtr force-pushed the feature/refactor_server_chooser branch from 970b392 to 1d66efe Compare June 10, 2026 12:56
@jpelgrom

Copy link
Copy Markdown
Member

This PR is also changing quite a lot in the deeplink handling which doesn't seem completely necessary (keep the PR small). Is it possible to split the changes?

@jpelgrom

Copy link
Copy Markdown
Member

The active checkmark always appears to be added to the sheet, but I'm not sure this makes sense in situations where the active server is unclear to the user, so when used outside the webview context when opening links or syncing Thread credentials from settings.

@TimoPtr

TimoPtr commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

This PR is also changing quite a lot in the deeplink handling which doesn't seem completely necessary (keep the PR small). Is it possible to split the changes?

It doesn't change the deep link it adds a viewModel to extract the logic out of the activity. Do you want a dedicated PR that introduce the viewModel?

@TimoPtr

TimoPtr commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

The active checkmark always appears to be added to the sheet, but I'm not sure this makes sense in situations where the active server is unclear to the user, so when used outside the webview context when opening links or syncing Thread credentials from settings.

Actually it is helpful for people that have the same name on two servers that have the same username. So you know which one is the currently activated.

@TimoPtr TimoPtr force-pushed the feature/refactor_server_chooser branch from 1d66efe to c3f9e27 Compare June 18, 2026 08:55
Base automatically changed from feature/avatar_manager to main June 19, 2026 06:03
@TimoPtr TimoPtr force-pushed the feature/refactor_server_chooser branch from c3f9e27 to 7c1b5bb Compare June 19, 2026 06:05

@jpelgrom jpelgrom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The server chooser waits for network requests to complete before showing servers. If a server is offline or slow to reach, you may be waiting up to okhttp's network timeout for that to happen. This shouldn't happen; any network requests to load the image should be async to showing the actual sheet.

Simulated example with 5s:

delay-server.mp4

Comment on lines +51 to +54
private val ROW_MIN_HEIGHT = HADimens.SPACE20
private val AVATAR_SIZE = HADimens.SPACE12
private val AVATAR_BADGE_SIZE = HADimens.SPACE5
private val AVATAR_BADGE_ICON_SIZE = HADimens.SPACE3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Everything here feels a bit too big to me, but maybe that is personal preference. Material 3 guidelines for a two-line list suggest 72dp min height (vs 80dp), 40dp avatar size (vs 48dp).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants