Skip to content

startup: Add beta-complete dialog #1797

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

Open
wants to merge 3 commits into
base: beta-prelaunch
Choose a base branch
from

Conversation

chrisbobbe
Copy link
Collaborator

Fixes #1603.

cc @alya

image image

@chrisbobbe chrisbobbe requested a review from gnprice August 8, 2025 22:26
@alya
Copy link
Collaborator

alya commented Aug 9, 2025

The screenshots look good to me!

It's a bit annoying that the heading might not fit on one line, but I think it's all right.

@chrisbobbe chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Aug 15, 2025
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 for taking care of this! Generally looks good; mostly small comments.

Comment on lines 151 to 179
unawaited(showDialog(
context: context,
builder: (BuildContext context) => AlertDialog(
title: Text('Time to switch to the new app'),
content: SingleChildScrollView(child: Text(message)),
actions: [
Copy link
Member

Choose a reason for hiding this comment

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

nit: I like the structure I found for the welcome dialog (0e34d96 / #1590):

  • turn this builder callback into the build method of a widget;
  • then the logic above the showDialog call (well, except for a couple of locals that are part of the build logic) becomes a static method on that widget — so e.g. BetaCompleteDialog.show().

In particular that gives the build callback a bit more room to breathe, and separates it from the one-time logic that decides when and whether to show it.

Comment on lines 162 to 168
TextButton(
onPressed: () {
Navigator.pop(context);
PlatformActions.launchUrl(context,
Uri.parse('https://github.com/zulip/zulip-flutter/releases/latest'));
},
child: _dialogActionText('Download official APKs (less common)')),
Copy link
Member

Choose a reason for hiding this comment

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

nit: These three very similar items can be made easier to read with a helper method; e.g.,

Suggested change
TextButton(
onPressed: () {
Navigator.pop(context);
PlatformActions.launchUrl(context,
Uri.parse('https://github.com/zulip/zulip-flutter/releases/latest'));
},
child: _dialogActionText('Download official APKs (less common)')),
_linkButton(
url: 'https://github.com/zulip/zulip-flutter/releases/latest',
label: 'Download official APKs (less common)'),


final zulipLocalizations = ZulipLocalizations.of(context);

final message = 'Since Zulip’s new Flutter app has launched, this beta app is no longer maintained. We strongly recommend uninstalling it and switching to the main Zulip application to get the latest features and bug fixes. Thank you for being a beta tester!';
Copy link
Member

Choose a reason for hiding this comment

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

nit: wrap for readability

Suggested change
final message = 'Since Zulip’s new Flutter app has launched, this beta app is no longer maintained. We strongly recommend uninstalling it and switching to the main Zulip application to get the latest features and bug fixes. Thank you for being a beta tester!';
final message = 'Since Zulip’s new Flutter app has launched,'
' this beta app is no longer maintained.'
' We strongly recommend uninstalling it and switching to the'
' main Zulip application to get the latest features and bug fixes.'
' Thank you for being a beta tester!';

(handy tip in Android Studio / IntelliJ: with cursor in the middle of a string literal, hit enter — that will insert the close-quote and open-quote to split the string, too)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for that tip, it worked great!


final zulipLocalizations = ZulipLocalizations.of(context);

final message = 'Since Zulip’s new Flutter app has launched, this beta app is no longer maintained. We strongly recommend uninstalling it and switching to the main Zulip application to get the latest features and bug fixes. Thank you for being a beta tester!';
Copy link
Member

Choose a reason for hiding this comment

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

For the text here, how about:

Thanks for being a beta tester of the new Zulip app! This app became the main Zulip mobile app in June 2025, and this beta version is no longer maintained. We recommend uninstalling this beta after switching to the main Zulip app, in order to get the latest features and bug fixes.

@@ -112,3 +116,75 @@ DialogStatus<bool> showSuggestedActionDialog({
]));
return DialogStatus(future);
}

bool debugDisableBetaCompleteDialog = false;
void resetDebugDisableBetaCompleteDialog() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should also start with "debug", as it's meant to be called only in debug mode

Comment on lines 80 to 81
debugDisableBetaCompleteDialog = true;
addTearDown(resetDebugDisableBetaCompleteDialog);
Copy link
Member

Choose a reason for hiding this comment

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

nit: alternatively, could dispense with the separate "reset" method:

Suggested change
debugDisableBetaCompleteDialog = true;
addTearDown(resetDebugDisableBetaCompleteDialog);
debugDisableBetaCompleteDialog = true;
addTearDown(() => debugDisableBetaCompleteDialog = false);

Comment on lines 77 to 78
debugDisableBetaCompleteDialog = true;
addTearDown(resetDebugDisableBetaCompleteDialog);
Copy link
Member

Choose a reason for hiding this comment

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

nit: are these needed? I don't see ZulipApp used in this file

@chrisbobbe chrisbobbe force-pushed the pr-beta-complete-dialog branch from 8685f9a to bbf6413 Compare August 16, 2025 00:31
@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed, and here's a screenshot with the new text:

image

This is a cherry-pick of d6affc9
from the main branch.
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! Looks good.

One nit; and I'm also pushing a commit to try to get CI working:
0190b1f ci: Fetch 3000 commits from upstream, rather than 1000

Comment on lines 154 to 155
Widget _linkButton(BuildContext context, {required String url, required String label}) =>
TextButton(
Copy link
Member

Choose a reason for hiding this comment

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

nit: use block body for a multi-line definition like this

@gnprice gnprice force-pushed the pr-beta-complete-dialog branch from bbf6413 to 6ead9e8 Compare August 16, 2025 00:52
@chrisbobbe chrisbobbe force-pushed the pr-beta-complete-dialog branch from 6ead9e8 to 8986236 Compare August 16, 2025 00:58
@chrisbobbe
Copy link
Collaborator Author

Thanks! Fixed that nit.

@gnprice
Copy link
Member

gnprice commented Aug 16, 2025

CI failed with one lint issue and 5 test failures.

At least some of those are due to using the latest upstream Flutter. So let's switch ci.yml in this branch to instead pin the same Flutter version that's in pubspec.yaml.

@chrisbobbe chrisbobbe force-pushed the pr-beta-complete-dialog branch 4 times, most recently from 2ba4e8d to 70eb175 Compare August 16, 2025 02:02
To avoid one lint issue and 5 test failures that would happen if
using the latest main.
@chrisbobbe chrisbobbe force-pushed the pr-beta-complete-dialog branch from 70eb175 to fd5138b Compare August 16, 2025 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants