Clickable hyperlinks for MessageBubbles #235

Merged
sarah merged 9 commits from NimaBoscarino/cwtch-ui:nima/clickable-links into trunk 2021-11-17 02:09:06 +00:00
Contributor

Addresses #169. Clickable links are added only to MessageBubble, and are locked behind an experiment gate.

Note that the PR is currently riddled with TODOs! Specific questions are annotated directly in the code view here on Git, but the main issues are:

  • The z-index of the SnackBar is below the modal, and I can't quite figure out how to get it to display above it!
  • Still need to test on Android emulator
  • General styling in the modal
  • onOpen breaks the selectable text

A demo of the clickable links

Addresses https://git.openprivacy.ca/cwtch.im/cwtch-ui/issues/169. Clickable links are added only to `MessageBubble`, and are locked behind an experiment gate. Note that the PR is currently riddled with TODOs! Specific questions are annotated directly in the code view here on Git, but the main issues are: * The z-index of the SnackBar is below the modal, and I can't quite figure out how to get it to display above it! * Still need to test on Android emulator * General styling in the modal * `onOpen` breaks the selectable text ![A demo of the clickable links](https://i.imgur.com/zSSQqPw.gif)
NimaBoscarino added 2 commits 2021-11-06 06:26:20 +00:00
ec1dd05ba1 WIP: add experimental clickable links with dialog
(copy / open). Bug remaining for selectable text
continuous-integration/drone/pr Build is failing Details
b8d50f234a
Merge branch 'trunk' into nima/clickable-links
NimaBoscarino reviewed 2021-11-06 06:27:10 +00:00
@ -30,6 +30,11 @@
<action android:name="android.intent.action.MAIN"/>
<category android:name="android.intent.category.LAUNCHER"/>
</intent-filter>
<intent-filter>
Author
Contributor

I still need to test this on my Android emulator this weekend (currently only working in the macOS app), so I'm not yet sure if this needs to be like this.

I still need to test this on my Android emulator this weekend (currently only working in the macOS app), so I'm not yet sure if this needs to be like this.
NimaBoscarino reviewed 2021-11-06 06:27:53 +00:00
@ -230,0 +241,4 @@
},
activeTrackColor: settings.theme.defaultButtonActiveColor(),
inactiveTrackColor: settings.theme.defaultButtonDisabledColor(),
secondary: Icon(Icons.link, color: settings.current().mainTextColor()),
Author
Contributor

Let me know if there's a preferred icon to use!

Let me know if there's a preferred icon to use!
Owner

Our staff designer @marcia will create an icon for this :)

Our staff designer @marcia will create an icon for this :)
NimaBoscarino reviewed 2021-11-06 06:29:09 +00:00
@ -58,0 +68,4 @@
text: widget.content + '\u202F',
// TODO: onOpen breaks the "selectable" functionality. Maybe something to do with gesture handler?
options: LinkifyOptions(humanize: false),
linkifiers: [UrlLinkifier()], // TODO: double-check on this (only web links to avoid Android messiness)
Author
Contributor

This restricts Linkify to only format URLs. Does this sound okay for now?

This restricts Linkify to only format URLs. Does this sound okay for now?
Owner

Yeah this sounds good.

Yeah this sounds good.
NimaBoscarino reviewed 2021-11-06 06:37:08 +00:00
@ -58,0 +66,4 @@
} else {
wdgMessage = SelectableLinkify(
text: widget.content + '\u202F',
// TODO: onOpen breaks the "selectable" functionality. Maybe something to do with gesture handler?
Author
Contributor

For some reason, once I add the onOpen handler the text selection breaks (e.g. https://i.imgur.com/SrtOLIG.mp4). I don't know enough about Flutter/Dart to figure it out easily, do you know what the case might be? I tried to recreate it in a fresh sandbox but had no luck, so I'm wondering if maybe the TapGestureRecognizer in Linkify conflicts with the MouseRegion in messagerow.dart?

For some reason, once I add the `onOpen` handler the text selection breaks (e.g. https://i.imgur.com/SrtOLIG.mp4). I don't know enough about Flutter/Dart to figure it out easily, do you know what the case might be? I tried to recreate it in a fresh sandbox but had no luck, so I'm wondering if maybe the `TapGestureRecognizer` in [Linkify](https://github.com/Cretezy/flutter_linkify/blob/1363236817d2c703770fdc7e02ccb4a3754b8ef8/lib/flutter_linkify.dart#L344) conflicts with the `MouseRegion` in `messagerow.dart`?
Owner

We've definitely had a few issues with widgets interacting in these kinds of ways. I will take a look into this early next week.

We've definitely had a few issues with widgets interacting in these kinds of ways. I will take a look into this early next week.
Owner

I could not replicate the lack of text selection on Linux under the following Flutter Build

Flutter 2.6.0-12.0.pre.637 • channel master • https://github.com/flutter/flutter.git
Framework • revision d6dd2fa78d (15 minutes ago) • 2021-11-08 11:23:34 -0800
Engine • revision 469d6f1a09
Tools • Dart 2.15.0 (build 2.15.0-285.0.dev)

As such I wonder if this is platform specific (or perhaps fixed in a newer version of flutter).

I could not replicate the lack of text selection on Linux under the following Flutter Build Flutter 2.6.0-12.0.pre.637 • channel master • https://github.com/flutter/flutter.git Framework • revision d6dd2fa78d (15 minutes ago) • 2021-11-08 11:23:34 -0800 Engine • revision 469d6f1a09 Tools • Dart 2.15.0 (build 2.15.0-285.0.dev) As such I wonder if this is platform specific (or perhaps fixed in a newer version of flutter).
Author
Contributor

I tried it with

Flutter 2.6.0-12.0.pre.641 • channel master • https://github.com/flutter/flutter.git
Framework • revision f4f23ecb59 (46 minutes ago) • 2021-11-08 12:27:14 -0800
Engine • revision 469d6f1a09
Tools • Dart 2.15.0 (build 2.15.0-285.0.dev)

and still ran into the issue, so it looks like it might be as macOS-specific issue!

I tried it with ``` Flutter 2.6.0-12.0.pre.641 • channel master • https://github.com/flutter/flutter.git Framework • revision f4f23ecb59 (46 minutes ago) • 2021-11-08 12:27:14 -0800 Engine • revision 469d6f1a09 Tools • Dart 2.15.0 (build 2.15.0-285.0.dev) ``` and still ran into the issue, so it looks like it might be as macOS-specific issue!
NimaBoscarino reviewed 2021-11-06 06:38:36 +00:00
@ -93,0 +123,4 @@
builder: (BuildContext bcontext) {
return Container(
// TODO: Ask re: hard-coded height
height: 200, // bespoke value courtesy of the [TextField] docs
Author
Contributor

I stole this modal from elsewhere in the code. Since the height has been hard-coded to 200 in several places, should I extract the modal out to be its own widget?

I stole this modal from elsewhere in the code. Since the height has been hard-coded to 200 in several places, should I extract the modal out to be its own widget?
Owner

I think having this widget defined here right now is fine. We have a separate task planned to go through and check widgets like (with hard coded heights etc.)

I think having this widget defined here right now is fine. We have a separate task planned to go through and check widgets like (with hard coded heights etc.)
NimaBoscarino reviewed 2021-11-06 06:39:21 +00:00
@ -93,0 +135,4 @@
"Opening this link will launch an application outside of Cwtch and may reveal metadata or otherwise compromise the security of Cwtch. Only open links from people you trust. Are you sure you want to continue?"
),
// TODO: Ask about styling preferences (should this be a reusable "inline-button"?)
Flex(direction: Axis.horizontal, mainAxisAlignment: MainAxisAlignment.center, children: <Widget>[
Author
Contributor

I'm quite new to styling and layout in Flutter. Does this look okay, or are there cleaner/other ways preferred?

I'm quite new to styling and layout in Flutter. Does this look okay, or are there cleaner/other ways preferred?
Owner

This looks fine :)

This looks fine :)
NimaBoscarino reviewed 2021-11-06 06:41:18 +00:00
@ -93,0 +144,4 @@
Clipboard.setData(new ClipboardData(text: link.url));
// TODO: Ask about desired SnackBar + modal behaviour
final snackBar = SnackBar(
Author
Contributor

I saw that in other places in Cwtch there's a snackbar displayed for the "Copy to clipboard" action, so I did the same here. It currently renders under the modal though, and I'm not sure how to make it display above. Any thoughts/suggestions for that?

I saw that in other places in Cwtch there's a snackbar displayed for the "Copy to clipboard" action, so I did the same here. It currently renders under the modal though, and I'm not sure how to make it display above. Any thoughts/suggestions for that?
Owner

You will probably need to cancel/destroy the modal prior to launching the snackBar.

You will probably need to cancel/destroy the modal prior to launching the snackBar.
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch-ui/326
Owner

@NimaBoscarino This is looking really good :)

I will take a closer look at this early next week and probably get some more comments back to you on Monday. And will also get Marcia to start thinking about the icon for the experiment switch.

I will also make a start at creating the translatable strings.

FYI: When you want to get down to some fuzz testing, I upgraded FuzzBot(4y2hxlxqzautabituedksnh2ulcgm2coqbure6wvfpg4gi2ci25ta5ad) yesterday to be on the most recent version of Cwtch so it should be more available and faster to respond.

Definitely interested to see how blns integrates with this new feature :)

Thanks again!

@NimaBoscarino This is looking really good :) I will take a closer look at this early next week and probably get some more comments back to you on Monday. And will also get Marcia to start thinking about the icon for the experiment switch. I will also make a start at creating the translatable strings. FYI: When you want to get down to some fuzz testing, I upgraded FuzzBot(`4y2hxlxqzautabituedksnh2ulcgm2coqbure6wvfpg4gi2ci25ta5ad`) yesterday to be on the most recent version of Cwtch so it should be more available and faster to respond. Definitely interested to see how `blns` integrates with this new feature :) Thanks again!
NimaBoscarino added 1 commit 2021-11-07 02:11:58 +00:00
continuous-integration/drone/pr Build is running Details
35dcc24e66
Remove resolved TODO statements, and destroy modal
after copying link
NimaBoscarino added 1 commit 2021-11-07 07:56:57 +00:00
continuous-integration/drone/pr Build is pending Details
a58e09dec5
android config for url_launcher
Author
Contributor

I tested it out with blns and I didn't catch anything that seemed strange! Just a few things like the pictures attached. Links are only detected if there's an http:// or https:// to start it.

I also managed to set it up to test it out on my Android emulator and it looks like links can open up properly now!

I tested it out with `blns` and I didn't catch anything that seemed strange! Just a few things like the pictures attached. Links are only detected if there's an `http://` or `https://` to start it. I also managed to set it up to test it out on my Android emulator and it looks like links can open up properly now!
Owner

I got this running this morning :) It works very well! And I think this is almost ready to be pulled into nightly.

Few small things:

- The color of the links needs to be set. Ideally they should either be set in mkThemeData (referencing .current().mainTextColor() or use Provider.of<Settings>(context).current().mainTextColor()

  • I could not replcicate the issue with text selection on Linux under the most recent Flutter nightly. @dan should probably test this PR on Windows / Mac to see if they can replicate.

Other Notes:

  • I asked Marcia this morning to work on an Icon. However I would be comfortable merging this into nightly without the Icon being finalized.
  • Same with translations, I will create the strings today, but I can replace them in a follow up PR.
    - This change may require an Flutter upgrade to our build server - @dan (url_launcher requires at least 2.4, and we may still be on the earlier dev release`
I got this running this morning :) It works very well! And I think this is almost ready to be pulled into nightly. Few small things: ~~- The color of the links needs to be set. Ideally they should either be set in `mkThemeData` (referencing `.current().mainTextColor()` or use `Provider.of<Settings>(context).current().mainTextColor()`~~ - I could not replcicate the issue with text selection on Linux under the most recent Flutter nightly. @dan should probably test this PR on Windows / Mac to see if they can replicate. Other Notes: - I asked Marcia this morning to work on an Icon. However I would be comfortable merging this into nightly without the Icon being finalized. - Same with translations, I will create the strings today, but I can replace them in a follow up PR. ~~- This change may require an Flutter upgrade to our build server - @dan (url_launcher requires at least 2.4, and we may still be on the earlier `dev` release`~~
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch-ui/327
NimaBoscarino added 1 commit 2021-11-08 20:58:14 +00:00
continuous-integration/drone/pr Build is pending Details
1c03fdf1db
Add link styling, using main text colour
sarah added this to the Cwtch Beta 1.5 project 2021-11-08 21:01:50 +00:00
sarah requested review from dan 2021-11-08 21:02:00 +00:00
sarah added the
cwtch-beta-1.5
enhancement
labels 2021-11-08 21:02:15 +00:00
NimaBoscarino changed title from WIP: Clickable hyperlinks for MessageBubbles to Clickable hyperlinks for MessageBubbles 2021-11-08 21:19:30 +00:00
sarah approved these changes 2021-11-08 21:58:09 +00:00
sarah added 1 commit 2021-11-10 18:03:38 +00:00
continuous-integration/drone/pr Build is pending Details
d553d6a474
Merge branch 'trunk' into nima/clickable-links
sarah added 1 commit 2021-11-10 18:13:16 +00:00
continuous-integration/drone/pr Build is pending Details
b9549e6885
Merge branch 'trunk' into nima/clickable-links
Owner

@NimaBoscarino - Thank you so much for your work on this. I've been using this experiment for a few days now and it has made a significant improvement to my workflow.

How would you like to credited in the changelog of the next cwtch release? Additionally: is there any profile/website you would like linked to the credit?

@NimaBoscarino - Thank you so much for your work on this. I've been using this experiment for a few days now and it has made a significant improvement to my workflow. How would you like to credited in the changelog of the next cwtch release? Additionally: is there any profile/website you would like linked to the credit?
Author
Contributor

@sarah Woo I'm so glad to hear that! And thank you so much for all the help with this issue! You can just credit me as Nima Boscarino, and my site is https://www.n11o.com/

@sarah Woo I'm so glad to hear that! And thank you so much for all the help with this issue! You can just credit me as Nima Boscarino, and my site is https://www.n11o.com/
sarah added 1 commit 2021-11-11 00:22:54 +00:00
continuous-integration/drone/pr Build is running Details
4dea1e1dd4
Merge branch 'trunk' into nima/clickable-links
dan approved these changes 2021-11-16 23:11:12 +00:00
dan left a comment
Owner

Looks good to me. We've used this to determine we need/are moving our build infra to flutter stable 2.5.3, (from dev 2.5.0-6pre) so I first need to upgrade all the build envs before this can be built and merged

Looks good to me. We've used this to determine we need/are moving our build infra to flutter stable 2.5.3, (from dev 2.5.0-6pre) so I first need to upgrade all the build envs before this can be built and merged
@ -228,2 +228,4 @@
secondary: Icon(Icons.attach_file, color: settings.current().mainTextColor()),
),
SwitchListTile(
title: Text("Enable Clickable Links", style: TextStyle(color: settings.current().mainTextColor())),
Owner

I beleive Sarah you will translate these in a follow up PR?

I beleive Sarah you will translate these in a follow up PR?
Owner

Yup.

Yup.
dan added 1 commit 2021-11-17 01:27:21 +00:00
continuous-integration/drone/pr Build is running Details
40c27c3f30
Merge branch 'trunk' into nima/clickable-links
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch-ui/346
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch-ui/340
Owner

Merging this. Build bot needs some updates to allow building from different forks. But we've now built this for all platforms.

Merging this. Build bot needs some updates to allow building from different forks. But we've now built this for all platforms.
sarah merged commit 2c1347e50e into trunk 2021-11-17 02:09:06 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cwtch.im/cwtch-ui#235
No description provided.