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

Giving avatar/display name error dialogs human readable error messages #5419

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Mar 3, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Reuses the error dialog logic which translates exceptions to human readable strings in the settings screen

Motivation and context

Fixes #5418 - The error messages for display name/avatar changing are making use of the raw exception instead of the nicely formatted message included within the exception

Screenshots / GIFs

BEFORE AFTER
Screenshot_20220303_155653 Screenshot_20220303_160945

Tests

  • Via the settings
  • Attempt to change the display name to one longer than 256 characters

Tested devices

  • Physical
  • Emulator
  • OS version(s): 31 (Sv2)

- reuses the ErrorDialog logic which translates exceptions to human readable strings
@ouchadam ouchadam force-pushed the feature/adm/settings-error-dialog branch from 5f0b0db to 54e23a2 Compare March 3, 2022 16:38
@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Unit Test Results

  92 files  ±0    92 suites  ±0   1m 3s ⏱️ -6s
162 tests ±0  162 ✔️ ±0  0 💤 ±0  0 ±0 
524 runs  ±0  524 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 54e23a2. ± Comparison against base commit 6e6b04c.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks!

* @param errorMessage the error message
*/
protected fun onCommonDone(errorMessage: String?) {
if (!isAdded) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is lost. Now we should probably use launchWhenResumed. If the operation takes time and the user leave the screen before it ends we can have crashes.

EDIT My bad, we are already in the lifecycleScope, so I guess it's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep exactly ^^^ seemed like left over code from before coroutines were introduced

@bmarty bmarty merged commit 1690a0b into develop Mar 3, 2022
@bmarty bmarty deleted the feature/adm/settings-error-dialog branch March 3, 2022 17:29
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.

Unfriendly message in display name error dialog
2 participants