From ed81d056d89410073b0ea782e981af368d502484 Mon Sep 17 00:00:00 2001 From: Vatsal Date: Wed, 27 Mar 2024 19:01:59 +0530 Subject: [PATCH 1/3] launch_url [nfc]: Move launchUrl logic to a new file - Move the existing launchUrl logic from content.dart to a new file named launch_url.dart. - Split function into pieces to handle realm-based and non-realm URLs. - Refactor error handling into a private function. --- lib/widgets/content.dart | 57 ++-------------------------------- lib/widgets/launch_url.dart | 62 +++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 54 deletions(-) create mode 100644 lib/widgets/launch_url.dart diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index acdc31cd67..6a76adfba7 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -1,7 +1,5 @@ -import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; -import 'package:flutter/services.dart'; import 'package:html/dom.dart' as dom; import 'package:intl/intl.dart'; import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; @@ -9,14 +7,11 @@ import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import '../api/core.dart'; import '../api/model/model.dart'; import '../model/avatar_url.dart'; -import '../model/binding.dart'; import '../model/content.dart'; -import '../model/internal_link.dart'; import 'code_block.dart'; -import 'dialog.dart'; import 'icons.dart'; +import 'launch_url.dart'; import 'lightbox.dart'; -import 'message_list.dart'; import 'store.dart'; import 'text.dart'; @@ -437,7 +432,7 @@ class MessageEmbedVideo extends StatelessWidget { final previewImageSrcUrl = store.tryResolveUrl(node.previewImageSrcUrl); return MessageMediaContainer( - onTap: () => _launchUrl(context, node.hrefUrl), + onTap: () => launchUrlWithRealm(context, node.hrefUrl), child: Stack( alignment: Alignment.center, children: [ @@ -609,7 +604,7 @@ class _BlockInlineContainerState extends State<_BlockInlineContainer> { void _prepareRecognizers() { _recognizers.addEntries(widget.links.map((node) => MapEntry(node, - TapGestureRecognizer()..onTap = () => _launchUrl(context, node.url)))); + TapGestureRecognizer()..onTap = () => launchUrlWithRealm(context, node.url)))); } void _disposeRecognizers() { @@ -1001,52 +996,6 @@ class GlobalTime extends StatelessWidget { } } -void _launchUrl(BuildContext context, String urlString) async { - Future showError(BuildContext context, String? message) { - return showErrorDialog(context: context, - title: 'Unable to open link', - message: [ - 'Link could not be opened: $urlString', - if (message != null) message, - ].join("\n\n")); - } - - final store = PerAccountStoreWidget.of(context); - final url = store.tryResolveUrl(urlString); - if (url == null) { // TODO(log) - await showError(context, null); - return; - } - - final internalNarrow = parseInternalLink(url, store); - if (internalNarrow != null) { - Navigator.push(context, - MessageListPage.buildRoute(context: context, - narrow: internalNarrow)); - return; - } - - bool launched = false; - String? errorMessage; - try { - launched = await ZulipBinding.instance.launchUrl(url, - mode: switch (defaultTargetPlatform) { - // On iOS we prefer LaunchMode.externalApplication because (for - // HTTP URLs) LaunchMode.platformDefault uses SFSafariViewController, - // which gives an awkward UX as described here: - // https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser/near/1169118 - TargetPlatform.iOS => UrlLaunchMode.externalApplication, - _ => UrlLaunchMode.platformDefault, - }); - } on PlatformException catch (e) { - errorMessage = e.message; - } - if (!launched) { // TODO(log) - if (!context.mounted) return; - await showError(context, errorMessage); - } -} - /// Like [Image.network], but includes [authHeader] if [src] is on-realm. /// /// Use this to present image content in the ambient realm: avatars, images in diff --git a/lib/widgets/launch_url.dart b/lib/widgets/launch_url.dart new file mode 100644 index 0000000000..6c7ba36e91 --- /dev/null +++ b/lib/widgets/launch_url.dart @@ -0,0 +1,62 @@ +import 'package:flutter/foundation.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; + +import '../model/binding.dart'; +import '../model/internal_link.dart'; +import 'dialog.dart'; +import 'message_list.dart'; +import 'store.dart'; + +/// Handles showing an error dialog with a customizable message. +Future _showError(BuildContext context, String? message, String urlString) { + return showErrorDialog( + context: context, + title: 'Unable to open link', + message: [ + 'Link could not be opened: $urlString', + if (message != null) message, + ].join("\n\n")); +} + +/// Launches a URL without considering a realm base URL. +void launchUrlWithoutRealm(BuildContext context, Uri url) async { + bool launched = false; + String? errorMessage; + try { + launched = await ZulipBinding.instance.launchUrl(url, + mode: switch (defaultTargetPlatform) { + // On iOS we prefer LaunchMode.externalApplication because (for + // HTTP URLs) LaunchMode.platformDefault uses SFSafariViewController, + // which gives an awkward UX as described here: + // https://chat.zulip.org/#narrow/stream/48-mobile/topic/in-app.20browser/near/1169118 + TargetPlatform.iOS => UrlLaunchMode.externalApplication, + _ => UrlLaunchMode.platformDefault, + }); + } on PlatformException catch (e) { + errorMessage = e.message; + } + if (!launched) { + if (!context.mounted) return; + await _showError(context, errorMessage, url.toString()); + } +} + +/// Launches a URL considering a realm base URL. +void launchUrlWithRealm(BuildContext context, String urlString) async { + final store = PerAccountStoreWidget.of(context); + final url = store.tryResolveUrl(urlString); + if (url == null) { // TODO(log) + await _showError(context, null, urlString); + return; + } + + final internalNarrow = parseInternalLink(url, store); + if (internalNarrow != null) { + Navigator.push(context, + MessageListPage.buildRoute(context: context, narrow: internalNarrow)); + return; + } + + launchUrlWithoutRealm(context, url); +} From 5a0a1785bf2a5f2ebdd51d79632ad8419e830d3e Mon Sep 17 00:00:00 2001 From: Vatsal Date: Fri, 29 Mar 2024 12:38:02 +0530 Subject: [PATCH 2/3] launch_url: Add i18n for launchUrl error messages --- assets/l10n/app_en.arb | 11 +++++++++++ lib/widgets/launch_url.dart | 6 ++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 626f72e683..31980b931a 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -308,6 +308,17 @@ "@loginServerUrlInputLabel": { "description": "Input label in login page for Zulip server URL entry." }, + "errorUnableToOpenLinkTitle": "Unable to open link", + "@errorUnableToOpenLinkTitle": { + "description": "Error title when a link fails to open." + }, + "errorUnableToOpenLinkMessage": "Link could not be opened: {url}", + "@errorUnableToOpenLinkMessage": { + "description": "Error message when a link fails to open.", + "placeholders": { + "url": {"type": "String", "example": "http://example.com/"} + } + }, "loginHidePassword": "Hide password", "@loginHidePassword": { "description": "Icon label for button to hide password in input form." diff --git a/lib/widgets/launch_url.dart b/lib/widgets/launch_url.dart index 6c7ba36e91..3d3e900c7c 100644 --- a/lib/widgets/launch_url.dart +++ b/lib/widgets/launch_url.dart @@ -1,6 +1,7 @@ import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; +import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import '../model/binding.dart'; import '../model/internal_link.dart'; @@ -10,11 +11,12 @@ import 'store.dart'; /// Handles showing an error dialog with a customizable message. Future _showError(BuildContext context, String? message, String urlString) { + final zulipLocalizations = ZulipLocalizations.of(context); return showErrorDialog( context: context, - title: 'Unable to open link', + title: zulipLocalizations.errorUnableToOpenLinkTitle, message: [ - 'Link could not be opened: $urlString', + zulipLocalizations.errorUnableToOpenLinkMessage(urlString), if (message != null) message, ].join("\n\n")); } From 560a4e08ee87d8011adcbcecc8afd4a21aadc656 Mon Sep 17 00:00:00 2001 From: Vatsal Date: Tue, 2 Apr 2024 17:09:02 +0530 Subject: [PATCH 3/3] login: Link to doc for what "server URL" is and how to find it Fixes: #109 --- assets/l10n/app_en.arb | 4 ++++ lib/widgets/login.dart | 17 +++++++++++++-- test/widgets/login_test.dart | 42 +++++++++++++++++++++++++++++++++++- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 31980b931a..7a62a4d92f 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -308,6 +308,10 @@ "@loginServerUrlInputLabel": { "description": "Input label in login page for Zulip server URL entry." }, + "serverUrlDocLinkLabel": "What's this?", + "@serverUrlDocLinkLabel": { + "description": "Link to doc to help users understand what a server URL is and how to find theirs." + }, "errorUnableToOpenLinkTitle": "Unable to open link", "@errorUnableToOpenLinkTitle": { "description": "Error title when a link fails to open." diff --git a/lib/widgets/login.dart b/lib/widgets/login.dart index 3f018ff9d9..a309a31af9 100644 --- a/lib/widgets/login.dart +++ b/lib/widgets/login.dart @@ -15,6 +15,7 @@ import '../model/store.dart'; import 'app.dart'; import 'dialog.dart'; import 'input.dart'; +import 'launch_url.dart'; import 'page.dart'; import 'store.dart'; import 'text.dart'; @@ -108,6 +109,9 @@ class ServerUrlTextEditingController extends TextEditingController { class AddAccountPage extends StatefulWidget { const AddAccountPage({super.key}); + static final Uri serverUrlHelpUrl = + Uri.parse('https://zulip.com/help/logging-in#find-the-zulip-log-in-url'); + static Route buildRoute() { return _LoginSequenceRoute(page: const AddAccountPage()); } @@ -213,7 +217,6 @@ class _AddAccountPageState extends State { child: ConstrainedBox( constraints: const BoxConstraints(maxWidth: 400), child: Column(mainAxisAlignment: MainAxisAlignment.center, children: [ - // TODO(#109) Link to doc about what a "server URL" is and how to find it // TODO(#111) Perhaps give tappable realm URL suggestions based on text typed so far TextField( controller: _controller, @@ -229,7 +232,17 @@ class _AddAccountPageState extends State { decoration: InputDecoration( labelText: zulipLocalizations.loginServerUrlInputLabel, errorText: errorText, - helperText: kLayoutPinningHelperText, + helper: GestureDetector( + onTap: () { + launchUrlWithoutRealm(context, AddAccountPage.serverUrlHelpUrl); + }, + child: Text( + // Restate Material default + // (`_InputDecoratorDefaultsM3.helperText` upstream)… + style: Theme.of(context).textTheme.bodySmall! + // …but use blue because this is a link. + .apply(color: const HSLColor.fromAHSL(1, 200, 1, 0.4).toColor()), + zulipLocalizations.serverUrlDocLinkLabel)), hintText: 'your-org.zulipchat.com')), const SizedBox(height: 8), ElevatedButton( diff --git a/test/widgets/login_test.dart b/test/widgets/login_test.dart index 9fc655d28b..bd9ebfb2df 100644 --- a/test/widgets/login_test.dart +++ b/test/widgets/login_test.dart @@ -1,8 +1,11 @@ import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; +import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; +import 'package:url_launcher/url_launcher.dart'; + import 'package:zulip/api/model/web_auth.dart'; import 'package:zulip/api/route/account.dart'; import 'package:zulip/api/route/realm.dart'; @@ -61,7 +64,44 @@ void main() { expectErrorFromText('email@example.com', ServerUrlValidationError.noUseEmail); }); - // TODO test AddAccountPage + group('AddAccountPage server URL helper text', () { + Future prepareAddAccountPage(WidgetTester tester) async { + await tester.pumpWidget(const MaterialApp( + localizationsDelegates: ZulipLocalizations.localizationsDelegates, + supportedLocales: ZulipLocalizations.supportedLocales, + home: AddAccountPage(), + )); + } + + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + + Future findHelperText(WidgetTester tester) async { + return find.text(zulipLocalizations.serverUrlDocLinkLabel); + } + + testWidgets('launches URL when helper text is tapped', (tester) async { + await prepareAddAccountPage(tester); + await tester.tap(await findHelperText(tester)); + + check(testBinding.takeLaunchUrlCalls()).single.equals(( + url: AddAccountPage.serverUrlHelpUrl, + mode: LaunchMode.platformDefault, + )); + }); + + testWidgets('shows error dialog when URL fails to open', (tester) async { + await prepareAddAccountPage(tester); + testBinding.launchUrlResult = false; + await tester.tap(await findHelperText(tester)); + await tester.pump(); + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: zulipLocalizations.errorUnableToOpenLinkTitle, + expectedMessage: zulipLocalizations.errorUnableToOpenLinkMessage( + AddAccountPage.serverUrlHelpUrl.toString(), + )))); + }); + }); group('LoginPage', () { late FakeApiConnection connection;