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

Add helper text for "server URL" with link to documentation #595

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add helper text for "server URL" with link to documentation #595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed81d05
launch_url [nfc]: Move launchUrl logic to a new file
VatsalBhesaniya Mar 27, 2024
5a0a178
launch_url: Add i18n for launchUrl error messages
VatsalBhesaniya Mar 29, 2024
560a4e0
login: Link to doc for what "server URL" is and how to find it
VatsalBhesaniya Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions assets/l10n/app_en.arb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,21 @@
"@loginServerUrlInputLabel": {
"description": "Input label in login page for Zulip server URL entry."
},
"serverUrlDocLinkLabel": "What's this?",
VatsalBhesaniya marked this conversation as resolved.
Show resolved Hide resolved
"@serverUrlDocLinkLabel": {
"description": "Link to doc to help users understand what a server URL is and how to find theirs."
},
"errorUnableToOpenLinkTitle": "Unable to open link",
VatsalBhesaniya marked this conversation as resolved.
Show resolved Hide resolved
"@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/"}
Copy link
Member

Choose a reason for hiding this comment

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

nit: even in examples, let's stick to HTTPS everywhere:

Suggested change
"url": {"type": "String", "example": "http://example.com/"}
"url": {"type": "String", "example": "https://example.com/"}

}
},
"loginHidePassword": "Hide password",
"@loginHidePassword": {
"description": "Icon label for button to hide password in input form."
Expand Down
57 changes: 3 additions & 54 deletions lib/widgets/content.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,17 @@
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';

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';

Expand Down Expand Up @@ -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: [
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -1001,52 +996,6 @@ class GlobalTime extends StatelessWidget {
}
}

void _launchUrl(BuildContext context, String urlString) async {
Future<void> 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;
Copy link
Member

Choose a reason for hiding this comment

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

This // TODO(log) got dropped. Was that intended?

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
Expand Down
64 changes: 64 additions & 0 deletions lib/widgets/launch_url.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
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';
import 'dialog.dart';
import 'message_list.dart';
import 'store.dart';

/// Handles showing an error dialog with a customizable message.
Future<void> _showError(BuildContext context, String? message, String urlString) {
Comment on lines +12 to +13
Copy link
Member

Choose a reason for hiding this comment

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

This doc doesn't really add anything beyond the function's name and parameters; so better to leave it out.

See:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-useless-documentation

final zulipLocalizations = ZulipLocalizations.of(context);
return showErrorDialog(
context: context,
title: zulipLocalizations.errorUnableToOpenLinkTitle,
message: [
zulipLocalizations.errorUnableToOpenLinkMessage(urlString),
if (message != null) message,
].join("\n\n"));
}

/// Launches a URL without considering a realm base URL.
void launchUrlWithoutRealm(BuildContext context, Uri url) async {
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

From the name, I can't really tell what this does; I don't know what "without realm" means.

I think what this function is really about in contrast with launchUrlWithRealm is that it requires the URL to be already resolved to an absolute URL — it won't accept a relative link. So we can rename it accordingly.

We can also revise the doc to be more specific about what "launch" means, following the upstream launchUrl doc; and add an assert checking that expectation of an absolute URL:

Suggested change
/// Launches a URL without considering a realm base URL.
void launchUrlWithoutRealm(BuildContext context, Uri url) async {
/// Pass a URL to the underlying platform for handling.
///
/// [url] must be an absolute URL.
/// For possibly-relative URLs, consider [launchUrlWithRealm].
void launchAbsoluteUrl(BuildContext context, Uri url) async {
assert(url.hasScheme);

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 {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly I don't feel like "with realm" quite makes clear what the relationship to the realm is. I think launchUrlWithRealmBase would do it.

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);
}
17 changes: 15 additions & 2 deletions lib/widgets/login.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<void> buildRoute() {
return _LoginSequenceRoute(page: const AddAccountPage());
}
Expand Down Expand Up @@ -213,7 +217,6 @@ class _AddAccountPageState extends State<AddAccountPage> {
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,
Expand All @@ -229,7 +232,17 @@ class _AddAccountPageState extends State<AddAccountPage> {
decoration: InputDecoration(
labelText: zulipLocalizations.loginServerUrlInputLabel,
errorText: errorText,
helperText: kLayoutPinningHelperText,
helper: GestureDetector(
onTap: () {
launchUrlWithoutRealm(context, AddAccountPage.serverUrlHelpUrl);
},
child: Text(
VatsalBhesaniya marked this conversation as resolved.
Show resolved Hide resolved
// 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(
Expand Down
42 changes: 41 additions & 1 deletion test/widgets/login_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -61,7 +64,44 @@ void main() {
expectErrorFromText('[email protected]', ServerUrlValidationError.noUseEmail);
});

// TODO test AddAccountPage
Copy link
Member

Choose a reason for hiding this comment

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

There's lots more to test on this page 🙂 — so this TODO is still relevant and let's keep it.

group('AddAccountPage server URL helper text', () {
Future<void> prepareAddAccountPage(WidgetTester tester) async {
await tester.pumpWidget(const MaterialApp(
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
supportedLocales: ZulipLocalizations.supportedLocales,
home: AddAccountPage(),
));
}

final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
Copy link
Member

Choose a reason for hiding this comment

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

This GlobalLocalizations.zulipLocalizations is application state that can change. So reading it should happen within individual test cases (including helpers they call), i.e. individual testWidgets callbacks, not outside them.


Future<Finder> 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;
Copy link
Member

Choose a reason for hiding this comment

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

When a test manipulates testBinding, it needs to call addTearDown(testBinding.reset) somewhere so that the state gets reset before the next test.

(In the "error dialog if invalid link" in widgets/content_test.dart which this one is presumably based on, that gets taken care of by the prepare helper, because the latter does other things with testBinding that also need a reset.)

You can reproduce the problem with the command:

$ flutter test test/widgets/login_test.dart --test-randomize-ordering-seed=random

At your current revision, that often fails because launchUrlResult remains set to false when the other new test tries to run. After you fix the problem with a reset, that command should pass every time.

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;
Expand Down
Loading