content: Handle link previews #1049

rajveermalviya · 2024-11-06T21:00:23Z

Screenshots

Web	Flutter

Web (Small)	Flutter (Small)

chrisbobbe

Thanks! Looks like this needs a rebase, so I'll do a more thorough review after that. But here are some comments from a quick skim (in particular I haven't read the parsing code or checked the UI code against web).

chrisbobbe · 2024-12-10T20:17:03Z

lib/widgets/content.dart

+      padding: const EdgeInsets.symmetric(horizontal: 5),
+      child: InsetShadowBox(
+        bottom: 8,
+        color: messageListTheme.streamMessageBgDefault,


This looks like it'll be wrong for DMs, and (in future) for messages where we highlight the background because of @-mentions in the message (#647).

chrisbobbe · 2024-12-10T20:20:29Z

lib/widgets/content.dart

+                  child: Text(node.title!,
+                    style: TextStyle(
+                      fontSize: 1.2 * kBaseFontSize,
+                      color: const HSLColor.fromAHSL(1, 200, 1, 0.4).toColor()))),


I see a hard-coded color; does it follow web? It needs either a variable in ContentTheme or this comment:

// Web has the same color in light and dark mode.

(Same for any other hard-coded colors.)

Please also post screenshots in light mode; I see screenshots for dark mode already.

Updated the screenshots.

chrisbobbe · 2024-12-10T20:22:31Z

lib/widgets/content.dart

+    if (isSmallWidth) {
+      return Container(
+        decoration: const BoxDecoration(border:
+          Border(left: BorderSide(color: Color(0xFFEDEDED), width: 3))),


Same comment about hard-coded colors; also, I think we more often use lowercase (so 0xffededed instead of 0xFFEDEDED); here and below.

chrisbobbe · 2024-12-10T20:32:02Z

lib/widgets/content.dart

+    final messageListTheme = MessageListTheme.of(context);
+    final isSmallWidth = MediaQuery.sizeOf(context).width <= 576;
+
+    final dataContainer = Container(


Is dataContainer the best name? I see the Container widget being used…what's the "data" and how does that widget "contain" it?

How about building this method's return value with help from a mutable Widget result variable? So here:

Widget result = Container(/* etc. */);

then below,

result = isSmallWidth ? Column(/* etc. */, children: [/* etc. */, result]) : Row(/* etc. */, children: [/* etc. */, result]);

then result = Container(decoration: /* etc. */, child: result);

and finally return result;.

rajveermalviya · 2024-12-12T17:48:02Z

Thanks for the initial comments @chrisbobbe! Pushed a new revision, PTAL.

chrisbobbe · 2024-12-23T22:59:54Z

What's a good way for me to test this; do I need to set up a dev server? 🙂 I see link previews are disabled on CZO; I've asked on CZO if there's a reason for that: https://chat.zulip.org/#narrow/channel/9-issues/topic/Link.20previews.20for.20Zulip.20URLs/near/2013846

gnprice · 2024-12-24T01:11:02Z

Dev server, or make a test realm on Zulip Cloud.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2025-01-13T21:09:30Z

lib/widgets/content.dart

+            GestureDetector(
+              onTap: () => _launchUrl(context, node.hrefUrl),
+              child: RealmContentNetworkImage(
+                Uri.parse(node.imageSrcUrl),


How about just one codepath for URL-parsing this string, instead of splitting by isSmallWidth?

Also, this will throw an error if parsing fails. Instead of doing that, let's use Uri.tryParse instead, similar to what we do in MessageImage.

chrisbobbe · 2025-01-14T00:01:06Z

lib/model/content.dart

+      if (second.nodes.length > 2) return null;
+
+      String? title, description;
+      for (final node in second.nodes) {


If there is both a title and a description, can they appear in either order? If not—if it's always the title first—how about requiring that? For the code structure, instead of a loop, maybe we could do a switch on the length of second.nodes, and in the 1 case we expect either a title or a description, and in the 2 case we expect a title first and then a description.

chrisbobbe · 2025-01-14T00:04:44Z

lib/model/content.dart

+      final first = divElement.nodes.first;
+      if (first is! dom.Element) return null;
+      if (first.localName != 'a') return null;
+      if (first.className != 'message_embed_image') return null;
+      if (first.nodes.isNotEmpty) return null;


I wonder if we can be a bit more compact in some places by using Dart Patterns; try a regex search for if.*case in this file.

chrisbobbe · 2025-01-14T20:40:20Z

lib/widgets/content.dart

+  @override
+  Widget build(BuildContext context) {
+    final messageListTheme = MessageListTheme.of(context);
+    final isSmallWidth = MediaQuery.sizeOf(context).width <= 576;


576 is from the web app, right? And some other explicit width and height values below: 500, 80, 115, etc.

We could comment on each one, saying they come from the web app. But actually, I could imagine future design work where we want to tune these numbers to be different from the web app. In that case such comments would become wrong/misleading if we forgot to update them. So maybe best not.

To memoize the fact that they match web, though (so a reader doesn't have to check each one), let's mention it in the commit message.

chrisbobbe · 2025-01-14T21:58:08Z

lib/widgets/content.dart

+
+    return Container(
+      decoration: const BoxDecoration(
+        border: Border(left: BorderSide(


BorderDirectional(start:, right?

chrisbobbe · 2025-01-14T23:00:23Z

lib/widgets/content.dart

+        // TODO(#647) use different color for highlighted messages
+        // TODO(#681) use different color for DM messages
+        color: messageListTheme.streamMessageBgDefault,


Ah right; yeah, I forgot we haven't done #681 yet.

I guess this needs one more TODO I hadn't thought of before:

// TODO(#488) use different color for non-message contexts

Probably the desired effect of that TODO will be to guide the implementation toward a color param rather than a param that's about the aspects of a Zulip message.

chrisbobbe · 2025-01-14T23:02:18Z

lib/widgets/content.dart

+              ? titleAndDescription
+              : LayoutBuilder(
+                  builder: (context, constraints) => ConstrainedBox(
+                    constraints: BoxConstraints(


There's a lot about constraints in this code: a Container.constraints, an UnconstrainedBox, a LayoutBuilder, a ConstrainedBox.constraints. I'm not really following it yet; do you think there might be a simpler way to write it?

Removed this LayoutBuilder here.

chrisbobbe · 2025-01-14T23:09:52Z

lib/widgets/content.dart

+              fit: BoxFit.cover,
+              width: 80,
+              height: 80,
+              alignment: Alignment.center)),


Isn't Alignment.center the default; can we leave out this argument?

chrisbobbe · 2025-01-14T23:19:10Z

lib/widgets/content.dart

+              : LayoutBuilder(
+                  builder: (context, constraints) => ConstrainedBox(
+                    constraints: BoxConstraints(
+                      maxWidth: constraints.maxWidth - 115),


I wonder about dropping this maxWidth - 115 detail, for a few benefits:

More of the text can show before it gets clipped

Removes the need for LayoutBuilder which isn't great for

performance

code complexity (e.g. my difficulty in a previous comment)

We allow more horizontal space for other paragraph content without issues

Could leave a code comment saying we're not following web in this way.

chrisbobbe · 2025-01-14T23:30:03Z

lib/widgets/content.dart

+        border: Border(left: BorderSide(
+          // Web has the same color in light and dark mode.
+          color: Color(0xffededed), width: 3))),
+      padding: const EdgeInsets.all(5),


Web also puts 5px bottom margin, a.k.a. --markdown-interelement-space-px, in addition to this. In zulip-flutter do we have something systematic for vertical spacing between block elements, or is each element responsible for adding its own space at the bottom and/or top?

From a quick test, looks like the spacing between different widgets is not consistent (or non-existent in some cases). On Web --markdown-interelement-space-px is calculated from line-height (which for me becomes 6.72px instead of 5px), so we'd need something similar for zulip-flutter too. However, it'll need a sweep across all the content widgets therefore making it out-of-scope for this PR.

Should 5px bottom margin as a quick fix, suffice for now?

Should 5px bottom margin as a quick fix, suffice for now?

Sure, sounds good

… delay Fixes zulip#1289.

The LinkPreview widget follows the Web styling, like having different layout for larger viewports (> 576), and any other constraints that are empirically present on Web. Fixes: zulip#1016

rajveermalviya · 2025-01-21T19:25:07Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

chrisbobbe

Looks good, thanks! Just nits below, and I think #1049 (comment) is still open. Otherwise LGTM; marking for Greg's review.

chrisbobbe · 2025-01-31T01:18:14Z

lib/widgets/content.dart

@@ -148,6 +151,7 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
  final Color colorTableCellBorder;
  final Color colorTableHeaderBackground;
  final Color colorThematicBreak;
+  final Color colorLink;


nit: keep in alphabetical order (here and elsewhere in class definition)

chrisbobbe · 2025-01-31T01:19:21Z

lib/widgets/content.dart

@@ -1030,7 +1037,7 @@ class _InlineContentBuilder {
        _pushRecognizer(recognizer);
        final result = _buildNodes(node.nodes,
          // Web has the same color in light and dark mode.


Let's remove this comment; it would be easy for it to become wrong if the light/dark variants start to be different. Also you've already left a comment with the same meaning on the dark variant.

rajveermalviya force-pushed the pr-content-link-previews branch from abdec10 to 8fba3fe Compare November 18, 2024 21:57

rajveermalviya marked this pull request as ready for review November 18, 2024 23:14

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Nov 18, 2024

chrisbobbe self-requested a review December 10, 2024 20:33

chrisbobbe self-assigned this Dec 10, 2024

chrisbobbe reviewed Dec 10, 2024

View reviewed changes

rajveermalviya force-pushed the pr-content-link-previews branch 3 times, most recently from 533fa33 to aeabbe6 Compare December 12, 2024 17:45

rajveermalviya requested a review from chrisbobbe December 12, 2024 17:48

chrisbobbe reviewed Jan 14, 2025

View reviewed changes

rajveermalviya force-pushed the pr-content-link-previews branch 2 times, most recently from 019edde to 6e37971 Compare January 21, 2025 19:19

chrisbobbe and others added 3 commits January 22, 2025 00:52

autocomplete test: Add extra pump in new test for new RawAutocomplete…

3e61f63

… delay Fixes zulip#1289.

content [nfc]: Move link color to ContentTheme.colorLink

2b7f723

content: Handle message_embed website previews

70ea927

The LinkPreview widget follows the Web styling, like having different layout for larger viewports (> 576), and any other constraints that are empirically present on Web. Fixes: zulip#1016

rajveermalviya force-pushed the pr-content-link-previews branch from 6e37971 to 70ea927 Compare January 21, 2025 19:22

rajveermalviya requested a review from chrisbobbe January 21, 2025 19:25

chrisbobbe reviewed Jan 31, 2025

View reviewed changes

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jan 31, 2025

chrisbobbe assigned gnprice and unassigned chrisbobbe Jan 31, 2025

chrisbobbe requested a review from gnprice January 31, 2025 01:21

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Jan 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

content: Handle link previews #1049

content: Handle link previews #1049

rajveermalviya commented Nov 6, 2024 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Dec 10, 2024

chrisbobbe Dec 10, 2024

rajveermalviya Dec 12, 2024

chrisbobbe Dec 10, 2024

chrisbobbe Dec 10, 2024

rajveermalviya commented Dec 12, 2024

chrisbobbe commented Dec 23, 2024

gnprice commented Dec 24, 2024

chrisbobbe left a comment

chrisbobbe Jan 13, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

rajveermalviya Jan 21, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

chrisbobbe Jan 14, 2025

rajveermalviya Jan 21, 2025

chrisbobbe Jan 31, 2025

rajveermalviya commented Jan 21, 2025

chrisbobbe left a comment

chrisbobbe Jan 31, 2025

chrisbobbe Jan 31, 2025

content: Handle link previews #1049

Are you sure you want to change the base?

content: Handle link previews #1049

Conversation

rajveermalviya commented Nov 6, 2024 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Dec 12, 2024

chrisbobbe commented Dec 23, 2024

gnprice commented Dec 24, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jan 21, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Nov 6, 2024 •

edited

Loading