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.

Sign up for GitHub

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

Jump to bottom

Support displaying user visible error messages #935

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Support displaying user visible error messages #935

merged 3 commits into from
Sep 24, 2024

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Sep 11, 2024

Fixes part of #555. The UX needs to be completed in #868.

@PIG208 PIG208 mentioned this pull request Sep 11, 2024
Merged
@PIG208
Copy link
Member Author

PIG208 commented Sep 11, 2024 •
edited
Loading

Pushing one additional commit here. Returning Future gives the caller some information about the snack bars that is otherwise unobtainable. This will be ready for review once I fixup the tests.

@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Sep 11, 2024
@PIG208 PIG208 requested review from gnprice and removed request for chrisbobbe September 11, 2024 22:04
@PIG208 PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Sep 11, 2024
@PIG208 PIG208 force-pushed the pr-report-error branch 2 times, most recently from 24b59d5 to 1c095b2 Compare September 12, 2024 20:27
@PIG208
Copy link
Member Author

PIG208 commented Sep 12, 2024 •
edited
Loading

Dropped the asynchronous implementation of reportErrorToUserBriefly, updated a commit message, and reworded/reformatted some parts of ZulipApp._reportErrorToUserBriefly.

gnprice
gnprice reviewed Sep 13, 2024
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM except one nit below.

lib/widgets/app.dart Outdated
Comment on lines 123 to 125
action: (details == null) ? null : SnackBarAction(
label: localizations.snackBarDetails,
onPressed: () => showErrorDialog(context: navigatorKey.currentContext!,
Copy link
Member

Choose a reason for hiding this comment

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

nit: indentation

@PIG208
Copy link
Member Author

PIG208 commented Sep 14, 2024

Thanks! Updated the PR.

@chrisbobbe
Copy link
Collaborator

The first commit has a (FIXME temp broken link).

@PIG208
Copy link
Member Author

PIG208 commented Sep 20, 2024 •
edited
Loading

Yeah, this is for preventing GitHub from auto-linking the upstream on pushes.

image

gnprice and others added 3 commits September 23, 2024 20:27
@gnprice @PIG208
app: Add ZulipApp.scaffoldMessenger
9aa0471 
Also generalize ValueNotifierChecks to ValueListenableChecks.

We can't write checks-extensions for ZulipApp.scaffoldMessenger
or ZulipApp.ready, though, because Dart doesn't have extensions
for static members.  Apparently they're currently working on them:
  dart-lang/language#723

Co-authored-by: Zixuan James Li <[email protected]>
@gnprice @PIG208
log: Support reportErrorToUserBriefly
2d5624c 
The motivation of having this indirection, rather than using
`showErrorDialog` and `showSnackBar` directly, is to keep
dependencies of `lib/log.dart` from relying on widget code.

Co-authored-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
@gnprice
log [nfc]: Revise reportErrorToUserBriefly doc; tweak related comments
e36f984
@gnprice gnprice merged commit e36f984 into zulip:main Sep 24, 2024
@gnprice
Copy link
Member

gnprice commented Sep 24, 2024

Thanks! Looks good; merging, with some commit-message edits:

      ## Commit message ##
    -    (FIXME temp broken link) app: add ZulipApp.scaffoldMessenger
    +    app: Add ZulipApp.scaffoldMessenger
     
    -    Because static extensions are not supported, we cannot create
    -    `app_checks.dart` for `ZulipApp.ready ` or
    -    `ZulipApp.scaffoldMessenger`.
    +    Also generalize ValueNotifierChecks to ValueListenableChecks.
     
    -    This generalizes `ValueNotifierChecks` to `ValueListenableChecks`,
    -    so that we can check `ZulipApp.ready`'s value directly.
    +    We can't write checks-extensions for ZulipApp.scaffoldMessenger
    +    or ZulipApp.ready, though, because Dart doesn't have extensions
    +    for static members.  Apparently they're currently working on them:
    +      https://github.com/dart-lang/language/issues/723
     
    -    See: https://github.com/dart-lang/language/issues/ 723
    +    Co-authored-by: Zixuan James Li <[email protected]>
     
      ## lib/widgets/app.dart ##
     @@ lib/widgets/app.dart: class ZulipApp extends StatefulWidget {
2:  c5f70731a ! 2:  2d5624c20 log: Support reportErrorToUserBriefly
    @@ Commit message
         `showErrorDialog` and `showSnackBar` directly, is to keep
         dependencies of `lib/log.dart` from relying on widget code.
     
    +    Co-authored-by: Zixuan James Li <[email protected]>
         Signed-off-by: Zixuan James Li <[email protected]>

and a doc/comment commit added on top:
e36f984 log [nfc]: Revise reportErrorToUserBriefly doc; tweak related comments

@PIG208 PIG208 deleted the pr-report-error branch September 24, 2024 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

Assignees

@gnprice gnprice

Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PIG208 @chrisbobbe @gnprice