content: Implement syntax hightlighting for code blocks #242

rajveermalviya · 2023-07-25T11:25:47Z

Context

Implements the syntax highlighting for the code blocks by adapting the css styles from the zulip web frontend to flutter's TextStyle.

Implementation details

While parsing the HTML nodes we store the text along with the className in CodeSpanNode.
Both of them are strings, but className can probably be changed to dart enums with their conversion functions (string <-> enum), so that app doesn't unnecessarily store the short strings hogging the memory.

In the widgets we do pattern matching over the className to match the specific TextStyle. Then we use it to generate the TextSpans for the RichText widget.

There is also an assumption that a span node will only have a single class associated with it, please let me know if it's an invalid assumption & I will update the code accordingly.

Notes

There are some styles that are repeating in the css classes which could be consolidated and referenced multiple times. But for now they are all defined separately for easier review.
The .err class in web frontend uses border, it cannot be mapped directly in TextSpans so it's TODO for now. (I read the comment documenting the trouble for _buildInlineCode).
- Should the .err class follow _buildInlineCode by using a suitable background color? Currently it just doesn't style it.

Testing

Didn't add any new tests, but updated the previous ones.

Fixes: #191

rajveermalviya · 2023-07-25T11:36:07Z

Preview of how it looks:

Flutter	Web

gnprice

Very interesting! Thanks @rajveermalviya for the contribution.

I haven't done a full code review, but some high-level thoughts:

Didn't add any new tests, but updated the previous ones.

Cool, that's fine for a draft for early feedback. We'll want some tests for this functionality before we merge it, though.

(Some of the existing functionality around it doesn't have tests, because a lot of the content parsing was written when this was still an experimental prototype. But now that we're committed to building the Flutter app, we're generally writing tests for each new feature as we add it, and gradually going back and adding tests for existing features.)

There is also an assumption that a span node will only have a single class associated with it, please let me know if it's an invalid assumption & I will update the code accordingly.

That assumption sounds good, if it's what you're seeing in test messages.

What we should then do is just make sure that if the assumption is ever violated, the parser produces an UnimplementedNode — so probably an UnimplementedBlockContentNode instead of the CodeBlockNode. See #190 for background on our plans there.

Both of them are strings, but className can probably be changed to dart enums with their conversion functions (string <-> enum), so that app doesn't unnecessarily store the short strings hogging the memory.

Yeah, an enum sounds good. That will also let the pattern-match in the widgets code be exhaustive.

Should the .err class follow _buildInlineCode by using a suitable background color? Currently it just doesn't style it.

Yeah, probably the best option is to follow what we do there. Should definitely get a comment calling out that that's what's happening.

gnprice · 2023-07-25T22:28:25Z

lib/widgets/content.dart

+// .gd { color: hsl(0deg 100% 31%); }
+final _kCodeBlockStyleGd = TextStyle(color: const HSLColor.fromAHSL(1, 0, 1, 0.31).toColor(), fontStyle: FontStyle.italic);


These appear not to match.

gnprice · 2023-07-26T06:56:07Z

lib/widgets/content.dart

+// .il { color: hsl(0deg 0% 40%); }
+final _kCodeBlockStyleIl = TextStyle(color: const HSLColor.fromAHSL(1, 0, 0, 0.40).toColor());
+
+class _CodeBlockHighlightedBuilder {


This happily doesn't need a builder class, because it doesn't have any state. (The reason we have _InlineContentBuilder is in order to track _recognizer and _recognizerStack as they change over the course of the recursion.)

Instead these methods can just go on the widget class that needs them. Or as top-level functions if they end up needing to be shared by two widget classes.

gnprice · 2023-07-26T06:57:22Z

lib/widgets/content.dart

+// .hll { background-color: hsl(60deg 100% 90%); }
+final _kCodeBlockStyleHll = TextStyle(backgroundColor: const HSLColor.fromAHSL(1, 60, 1, 0.9).toColor());


BTW I appreciate how the CSS is interleaved here as comments — it helps a lot in tracing the relationship of this code to the web app's styles.

gnprice · 2023-07-26T07:07:16Z

lib/widgets/content.dart

+  if (node.spans.isEmpty) {
+    return const CodeBlockBasic(text: '');
+  } else if (node.spans.length == 1 && node.spans.first.className == ''){
+    return CodeBlockBasic(text: node.spans.first.text);
+  } else {
+    return CodeBlockHighlighted(node: node);


Can we unify the different cases here, and have just one CodeBlock widget that behaves like this CodeBlockHighlighted?

Or perhaps with a _CodeBlockContainer widget if that helps with the code organization — that one is fine, but what I really mean here is that ideally we'd have one CodeBlock instead of _buildCodeBlock, CodeBlockBasic, and CodeBlockHighlighted.

I know this resembles _buildBlockInlineContainer and its helpers. Those set a bit of a bad example, I think — I feel like they ended up somewhat messy, but I was paranoid about performance there. Specifically, every paragraph (and some other things too) in every message is going to go through that code path, and most of them don't contain any links (which are the thing that makes _BlockInlineContainer complicated and stateful), and I was worried about making every paragraph into a StatefulWidget when most of them don't need any state.

Here, code blocks aren't nearly so ubiquitous as paragraphs are, so I don't feel such a need to be paranoid; and in any case it's not clear that what CodeBlockHighlighted is doing (for a given number of spans) will be any more expensive than what CodeBlockBasic is doing. So we can cheerfully keep things simple.

ack, unified them into single CodeBlock widget

gnprice · 2023-07-26T07:14:43Z

lib/model/content.dart

-class CodeBlockNode extends BlockContentNode {
-  // TODO(#191) represent the code-highlighting style spans in CodeBlockNode
-  const CodeBlockNode({super.debugHtmlNode, required this.text});
+class CodeSpanNode extends BlockContentNode {


Suggested change

class CodeSpanNode extends BlockContentNode {

class CodeBlockSpan extends InlineContentNode {

(Or perhaps CodeBlockSpanNode.)

Just "code span" sounds like a description of what InlineCodeNode is. This node is specifically for a span found inside a code block.

This is a type of inline content node, not a block content node. See the dartdoc comments on InlineContentNode and BlockContentNode.

(Or maybe the base class should even be just ContentNode. While it's true that this is an inline content node, it's not a node we expect to find in any of the containers that have a generic list of InlineContentNodes, i.e. any of the subclasses of BlockInlineContainerNode or InlineContainerNode. So inheriting directly from ContentNode would express that for the benefit of code consuming those container nodes.)

Also let's put this after CodeBlockNode, rather than before, to match the organization of the other classes in this file.

ack, renamed CodeSpanNode to CodeBlockSpanNode

rajveermalviya · 2023-07-26T14:04:59Z

Thanks for the review @gnprice, I have rebased the branch with suggested changes.

rajveermalviya · 2023-07-26T14:16:16Z

Some new things to note:

We don't have a style for all of the token types from pygments library, and zulip also doesn't style all of them. So how we currently handle it is: parser parses all of the token types possible, in ui-view/page/widget/drawing (idk what really to call this phase) we pattern match to style only some of the tokens (this list is same as web).
Also one weird thing to note, the .hll class is not in pygments' list of token types and moreover zulip's backend doesn't seem to generate html using it either. For now, I haven't included it.

gnprice · 2023-07-26T20:30:39Z

Also one weird thing to note, the .hll class is not in pygments' list of token types

Huh yeah, odd.

Searching the pygments repo, the hll class does appear. In particular it's emitted here:
https://github.com/pygments/pygments/blob/d0acfff1121f9ee3696b01a9077ebe9990216634/pygments/formatters/html.py#L915-L933
and elsewhere there's code to emit a CSS rule applying to it. It looks like the feature it applies to is "highlighted lines".

And then grepping the zulip repo for "hl_lines", which is the pygments option that activates that feature, it does look like we offer it. I think it can be exercised with a test message like… aha, yeah, like this:

```
::markdown hl_lines="2 4"
# he
## llo
### world
```

which will give a yellow highlight to the lines # he and ### world.

Kind of odd there with that :: syntax. I'm not sure we really intend to support that. But it's there, and it means there are existing messages that use it.

gnprice · 2023-07-26T20:32:30Z

We don't have a style for all of the token types from pygments library, and zulip also doesn't style all of them. So how we currently handle it is: parser parses all of the token types possible, in ui-view/page/widget/drawing (idk what really to call this phase) we pattern match to style only some of the tokens (this list is same as web).

Sounds good.

I'd call that phase "the widgets layer", or "at build time".

(Sometimes I also say "rendering", vs. "parsing" for what's in lib/model/content.dart. But probably I should avoid that term, as it has its own meaning in a Flutter context and so is potentially confusing.)

rajveermalviya · 2023-07-27T01:50:08Z

Searching the pygments repo, the hll class does appear. In particular it's emitted here:

Thanks for finding it @gnprice

I'm not sure we really intend to support that. But it's there, and it means there are existing messages that use it.

Added the token as highlightedLines and the corresponding styles for it.

gnprice

Thanks for the revisions! Sorry for the bit of delay in replying.

Generally this looks great. The one substantial other piece we'll want before we can merge this is some tests, as mentioned at #242 (review) . If you'd like to discuss ideas for how to write good tests for this functionality, please mention it in the #mobile-team stream on chat.zulip.org and I'll be glad to talk about that.

I've just now gone and given this a full code review, and have a variety of smaller comments below.

gnprice · 2023-08-01T20:49:43Z

lib/model/content.dart

+  /// 'w'
+  whitespace,
+  /// 'esc'
+  escape,
+  /// 'err'
+  error,


Let's move these long boring boilerplatey sections — all the places that contain a list of Pygments token types — into their own separate file. Or I guess their own two files: lib/model/code_block.dart and lib/widgets/code_block.dart.

I think that separation — boilerplate in a boilerplate file, and non-boilerplate code in a non-boilerplate file — helps keep things easy to read, because one uses very different reading styles for reading those two kinds of code.

The CodeBlockNode and CodeBlockSpanNode declarations can stay in this file, though.

Perhaps in the future if the number of files that exist to support the two content.dart files grows, we might organize them further, like lib/model/content/code_block.dart and so on. But let's leave aside that extra nesting for now.

(Just to be clear in case "boring" or "boilerplatey" sound like they're bad things — in some contexts they could be, but boring and boilerplatey are exactly the right approach for some things, and this is an excellent example. I'm quite happy to see this boring boilerplate for translating these CSS classes to enum values and then to text styles.)

gnprice · 2023-08-01T20:56:45Z

lib/model/content.dart

+//
+// Note: If you update this list make sure to update the permalink
+//       and the `tryFromString` function below.
+enum CodeBlockSpanToken {


A value of this enum isn't really a token — it identifies a type of token. So it'd be better to make the last word of this name be "type" rather than "token".

Conversely, I think we need only one of the words "span" or "token" in this name. Each of these spans corresponds to a token Pyjamas found… hmm or I guess perhaps a run of consecutive tokens of the same type. Given that latter point, plus that we sometimes have a CodeBlockSpanNode which doesn't correspond to anything from Pyjamas — what we parse from a code block with no span element, which is what the user gets if they enter a plain code block with no language — I think "span" is the right version of the concept to mention in this name and we can skip "token". So:

Suggested change

enum CodeBlockSpanToken {

enum CodeBlockSpanType {

gnprice · 2023-08-01T22:06:20Z

lib/model/content.dart

+ @override
+  List<DiagnosticsNode> debugDescribeChildren() {


nit:

Suggested change

@override

List<DiagnosticsNode> debugDescribeChildren() {

@override

List<DiagnosticsNode> debugDescribeChildren() {

gnprice · 2023-08-01T22:08:44Z

lib/model/content.dart

+    return spans
+      .map((node) => node.toDiagnosticsNode())
+      .toList();


nit:

Suggested change

return spans

.map((node) => node.toDiagnosticsNode())

.toList();

return spans.map((node) => node.toDiagnosticsNode()).toList();

That makes a bit more compact so it takes up less vertical space, letting the reader see more of the node classes above and below it on their screen at once. It also makes it more visibly parallel to the similar debugDescribeChildren implementations in neighboring classes.

gnprice · 2023-08-01T22:21:21Z

lib/model/content.dart

+  /// 'kc'
+  keywordConstant,


I appreciate having docs on these. Let's go a bit further and bring them up to our standard docs style, as complete sentences:

Suggested change

/// 'kc'

keywordConstant,

/// A code-block span with CSS class `kc`.

keywordConstant,

(Similarly for all the others. Given how successfully you've produced all the other boilerplate sections of this PR, I imagine you already know how to use find-and-replace or multiple-cursors or similar tools to efficiently make this change to all the enum values at once. For anything in that vein where you do have questions, though, I'd be happy to discuss in #mobile-team on chat.zulip.org.)

For docs style in general, we largely follow the same style as Flutter itself, so Flutter's style guide is a good reference:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-correct-grammar
(We don't do the really high-effort parts of Flutter's docs style, though — sample code and images — because that level of effort on API docs makes sense only when lots of developers are going to consume it.)

gnprice · 2023-08-01T23:02:49Z

lib/widgets/content.dart

+
+  Widget _buildText() {
+    return switch (node.spans) {
+      []  => Text('', style: _kCodeBlockStyle),


Similarly to the other comment: can we just delete this case and let the default case handle it?

gnprice · 2023-08-01T23:08:26Z

lib/widgets/content.dart

      decoration: BoxDecoration(
        color: Colors.white,
        border: Border.all(
          width: 1,
-          color: const HSLColor.fromAHSL(0.15, 0, 0, 0).toColor())),
+          color: const HSLColor.fromAHSL(0.15, 0, 0, 0).toColor()),
+        borderRadius: BorderRadius.circular(4)),


This is a good change, thanks. (I think this border radius must be one of the changes that happened in the recent web redesign; see #157.)

Let's put it in a separate commit, though, so it can get its own commit message. See the Zulip style guide for commits in a branch:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html

ack, moved to a separate commit

gnprice · 2023-08-01T23:09:49Z

lib/widgets/content.dart

+        child: Padding(
+          padding: const EdgeInsets.fromLTRB(7, 5, 7, 3),


What's the reason for moving this padding here instead of the Container? On a quick experiment it seems like I get the same output with or without this change.

(Probably it should also be its own commit, or else in the same commit with the borderRadius if the reason for it is related to that change.)

Here's a comparison:
(note the padding on ScrollView adds the spacing on all sides between the actual scrollable view and the code block borders)

Before After

Got it, thanks. Definitely looks like a good change.

Here's a version for the commit message:

content: Move CodeBlock padding to inside scroll view This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: https://github.com/zulip/zulip-flutter/pull/242#discussion_r1285096855

gnprice · 2023-08-01T23:19:20Z

lib/widgets/content.dart

+  InlineSpan _buildNode(CodeBlockSpanNode node) {
+    final style = switch (node.tokenType) {
+      CodeBlockSpanToken.text => null, // styles not applied for Text token
+      CodeBlockSpanToken.highlightedLines => _kCodeBlockStyleHll,


Hmm, so, here's how this looks for my highlighted-lines test message:

But here's how it looks on web:

Note that on web, the text colors and bold are still there — there's just also the background color for the line highlight.

It looks like in order to handle this feature, we'll need some more complication in the parser and the data structure. Here's how the HTML comes out (see test/model/content_test.dart for tips on conveniently getting the server's HTML):

<span class="hll"><span class="gh"># he</span> </span><span class="gu">## llo</span>

Note the nesting, with a span.hll around another span.

Given that it'd require making our implementation more complicated, and that it's a feature Zulip doesn't support very nicely in the first place (#242 (comment)) and it's not clear we actually intend to support, I'd be happy to just not support it, at least for the present. In that case we can have the parser just produce UnimplementedBlockContentNode when it encounters one of these.

Let's do include a test case involving this feature, though. That'll help validate that we don't do anything wild when we run into it.

Updated the implementation to generically handle one level of nested span nodes in parsing the code block spans. For now, this is sufficient for span.hll nodes.

Also added a unit test to handle "highlighted lines" tokens.

gnprice · 2023-08-01T23:28:38Z

lib/widgets/content.dart

+// .o { color: hsl(332deg 70% 38%); }
+final _kCodeBlockStyleO = TextStyle(color: const HSLColor.fromAHSL(1, 332, 0.7, 0.38).toColor());
+// .cm { color: hsl(180deg 33% 37%); font-style: italic; }
+final _kCodeBlockStyleCm = TextStyle(color: const HSLColor.fromAHSL(1, 180, 0.33, 0.37).toColor(), fontStyle: FontStyle.italic);


(BTW this way of formatting these is great — I appreciate that it's so compact. No need to turn the comments into doc comments or whatever, which would inevitably make them less compact.)

As I write that, though, I realize there's one tweak which would I think improve the formatting further: add a blank line to separate each definition from the next one's comment. That just makes the grouping more unambiguous so it's quicker for the eye to go from a comment to the corresponding code and back.

It does make things a bit less compact, but so it goes.

That's also an advantage of moving the boilerplate to its own file — if it stayed here in this file, then making this section 50% longer would be a real cost of its own to the readability of the rest of the file, making it more of a trade-off.

rajveermalviya · 2023-08-05T19:01:54Z

Sorry for delayed revision @gnprice, I have updated & rebased with the suggested changes.

gnprice

Thanks for the revision! Generally these changes look good, except I'd like to do without the complexity brought by supporting hll and nested spans. Details below.

gnprice · 2023-08-10T01:53:09Z

lib/model/content.dart

+        if (type == CodeBlockSpanType.unknown) {
+          return (null, false);
+        }
+        return (CodeBlockSpanNode(text: text,type: type), true);


nit:

Suggested change

return (CodeBlockSpanNode(text: text,type: type), true);

return (CodeBlockSpanNode(text: text, type: type), true);

gnprice · 2023-08-10T01:57:19Z

lib/model/content.dart

+          // Further implementation is based on an assumption that there will
+          // be only a single class associated with the span element.
+          && classes.length == 1:
+        final type = CodeBlockSpanTypeExt.fromClassName(className);


Hmm, awkward that this ends up needing to mention the name we come up with for the extension.

Seems like we can't put a static method on an enum, which is too bad.

Probably cleanest to just make it a top-level function of its own, like codeBlockSpanTypeFromClassName. Long name, but not longer than CodeBlockSpanTypeExt.fromClassName :-)

gnprice · 2023-08-10T02:01:09Z

test/widgets/content_test.dart

@@ -161,4 +161,33 @@ void main() {
      check(tester.takeException()).isA<AssertionError>();
    });
  });
+
+  group("CodeBlock", () {


Let's squash the new tests into the same commit that introduces the code they're testing.

ack, squashed

gnprice · 2023-08-10T02:06:26Z

lib/widgets/content.dart

+        child: Padding(
+          padding: const EdgeInsets.fromLTRB(7, 5, 7, 3),


Got it, thanks. Definitely looks like a good change.

Here's a version for the commit message:

content: Move CodeBlock padding to inside scroll view This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: https://github.com/zulip/zulip-flutter/pull/242#discussion_r1285096855

gnprice · 2023-08-10T02:08:56Z

lib/model/content.dart

+      if (child is dom.Element && child.nodes.isNotEmpty) {
+         if (child.nodes.length == 1 && child.nodes.first is dom.Text) {
+          // Do nothing, fallthrough to the switch statement below.


nit:

Suggested change

if (child is dom.Element && child.nodes.isNotEmpty) {

if (child.nodes.length == 1 && child.nodes.first is dom.Text) {

// Do nothing, fallthrough to the switch statement below.

if (child is dom.Element && child.nodes.isNotEmpty) {

if (child.nodes.length == 1 && child.nodes.first is dom.Text) {

// Do nothing, fallthrough to the switch statement below.

gnprice · 2023-08-10T04:02:12Z

test/widgets/content_test.dart

@@ -161,4 +161,33 @@ void main() {
      check(tester.takeException()).isA<AssertionError>();
    });
  });
+
+  group("CodeBlock", () {


nit: move this above the two existing groups

That way we keep the tests in the same order as the code they're testing, at least at a coarse level of granularity: CodeBlock/CodeBlockNode, then LinkNode which is handled by InlineContent, then RealmContentNetworkImage.

(That isn't always possible because a test doesn't always correspond neatly to a particular area of code. But in this case they do nicely correspond, so we can.)

gnprice · 2023-08-10T04:03:28Z

test/widgets/content_test.dart

+      expect(find.text('class A {}'), findsOneWidget);
+    });
+
+    testWidgets('without syntax highlighting', (WidgetTester tester) async {


nit: put "without" case before "with", because it's simpler

gnprice · 2023-08-10T04:09:53Z

test/widgets/content_test.dart

+          '\n</code></pre></div>'
+      );


nit: when an item in a comma-separated list (not only List, but any syntactic list of things) is followed by a newline, always use a comma:

Suggested change

'\n</code></pre></div>'

);

'\n</code></pre></div>',

);

One has to do that anyway if it's any list item other than the last; so this trailing comma makes the syntax more symmetric, which is helpful for editing and for keeping diffs less noisy.

The other option here is to follow it directly with the close-paren instead of a newline:

'\n</code></pre></div>');

gnprice · 2023-08-10T04:16:09Z

test/widgets/content_test.dart

+          '\n</code></pre></div>'
+      );
+
+      expect(find.text('class A {}'), findsOneWidget);


We're writing our tests with package:checks and its check function, rather than expect. See our README:
https://github.com/zulip/zulip-flutter/blob/3cba0582fc68d875872b0b0394d75eedef109bbb/README.md#writing-tests

For this particular use case, we don't have a great idiom yet; that's #232. You could use the idiom mentioned there, or this would also be OK:

Suggested change

expect(find.text('class A {}'), findsOneWidget);

expect(find.text('class A {}'), findsOneWidget); // TODO(#232): use package:checks

i.e., leave a comment that marks clearly that this is something we want to fix (which also helps avoid people taking it as an example for other situations) and that makes it easy to find this when we fix #232 and make a sweep of all the affected spots.

ack, now uses tester.widget()

gnprice · 2023-08-10T04:25:06Z

test/widgets/content_test.dart

+      await tester.pumpWidget(MaterialApp(home: BlockContentList(nodes: parseContent(html).nodes)));
+      await tester.pump();
+      await tester.pump();


These latter pumps aren't needed:

Suggested change

await tester.pumpWidget(MaterialApp(home: BlockContentList(nodes: parseContent(html).nodes)));

await tester.pump();

await tester.pump();

await tester.pumpWidget(MaterialApp(home: BlockContentList(nodes: parseContent(html).nodes)));

In the other prepareContent, they're needed because GlobalStoreWidget and PerAccountStoreWidget each involve an async step. (To read from the database and fetch from the server, respectively. Actually we go to some trouble to make PerAccountStoreWidget after the data for a given account is first loaded — see _PerAccountStoreWidgetState.didChangeDependencies and the test "PerAccountStoreWidget immediate data after first loaded" in test/widgets/store_test.dart — but on first load, both those async steps are pretty fundamentally needed.)

But here, there's no async load involved, so it all builds on the first frame. You can verify that by just deleting these and seeing that the tests still pass, thanks to those find.text expectations which ensure the tests wouldn't spuriously pass if the widgets we want to test weren't yet built.

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

rajveermalviya · 2023-08-10T10:16:12Z

Thanks for the review @gnprice, I have rebased and updated the changes based on the suggestions.

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

rajveermalviya · 2023-08-10T13:42:22Z

(Just rebased to main)

gnprice

Thanks for the revision! This is getting close; various small comments below.

gnprice · 2023-08-11T03:05:00Z

lib/model/content.dart

+          when localName == 'span'
+            // We assume that server will always provide us with a span element
+            // that has at most one class associated with the span element.
+            && classes.length <= 1:


Can this be == 1 instead? That is, is there some known case where we get a span at this spot that has no classes?

If so, that'd nicely tighten the logic further.

Skimming through pygments html formatter: It doesn't seem to emit a span element if css_class is empty. So, it's safe to say this will never happen; Removing that case.

gnprice · 2023-08-11T03:07:38Z

lib/model/content.dart

+      final CodeBlockSpanNode span;
+


nit:

Suggested change

final CodeBlockSpanNode span;

final CodeBlockSpanNode span;

That way this variable is declared immediately before the switch that initializes it. It's naturally closely tied to that, so it's helpful to have it visually adjacent.

gnprice · 2023-08-11T03:08:33Z

lib/model/content.dart

+          final CodeBlockSpanType type;
+          if (classes.isEmpty) {
+            type = CodeBlockSpanType.text;
+          } else {
+            type = codeBlockSpanTypeFromClassName(classes.first);
+          }


nit: this one is probably simpler in ?: form

(though it'll go away if we can tighten the logic above to require classes.length == 1)

gnprice · 2023-08-11T03:09:53Z

lib/model/content.dart

+            // We assume that server will always provide us with a span element
+            // that has at most one class associated with the span element.


nit: can cut the comment — the code just around it expresses the same content

(These lines of code don't exactly directly say the "we assume" angle of it. But that's expressed by the fact that if these conditions fail, we return an UnimplementedNode.)

gnprice · 2023-08-11T03:11:30Z

lib/model/content.dart

+      }
+      if (span.text.isNotEmpty) {


nit:

Suggested change

}

if (span.text.isNotEmpty) {

}

if (span.text.isNotEmpty) {

The switch above is a big multi-stanza piece of code, and this next bit is separate from that, so I think it's helpful to separate it as a new stanza.

gnprice · 2023-08-11T19:40:16Z

test/model/content_test.dart

+  testParse('parse code blocks, unknown span type',
+    // A code block span with unknown className.


Suggested change

testParse('parse code blocks, unknown span type',

// A code block span with unknown className.

testParse('parse code blocks, unknown span type',

// (no markdown; this test is for future Pygments versions adding new token types)

In particular that makes it explicit that this test intentionally doesn't have Markdown (that that's not just a mistake), and explains why.

gnprice · 2023-08-11T19:44:49Z

test/model/content_test.dart

+    '<div class="codehilite"><pre>'
+        '<code>::markdown hl_lines="2 4"'
+        '<span class="hll"><span class="gh"># he</span>'
+        '</span><span class="gu">## llo</span>'
+        '<span class="hll"><span class="gu">### world</span>'
+        '</span></code></pre></div>', const [


Great. Yeah, the browser inspector is very helpful for things like understanding what the web app's CSS does with the HTML, but it doesn't show quite the same HTML as we get from the server.

In some cases that's because the web app's JS code makes changes to the HTML:
https://chat.zulip.org/#narrow/stream/378-api-design/topic/Video.20markdown.20support.20.2320449/near/1620082
so those can make things arbitrarily different. And then I think some of these other differences may be the browser's doing.

gnprice · 2023-08-11T19:49:22Z

test/model/content_test.dart

+    ]);
+
+  testParse('parse code blocks, with syntax highlighting and highlighted lines',
+    // "```\n::markdown hl_lines="2 4"\n# he\n## llo\n### world\n```"


Still not valid syntax :-) (continuing from #242 (comment) )

Try uncommenting the line, and you'll see parse errors. That's because there are double-quotes " inside the intended string, which interfere with the double-quotes used to enclose it.

I think the simplest fix is to enclose with single-quotes ' instead, as in my previous suggestion. Triple quotes would also work:

Suggested change

// "```\n::markdown hl_lines="2 4"\n# he\n## llo\n### world\n```"

// """```\n::markdown hl_lines="2 4"\n# he\n## llo\n### world\n```"""

In principle another solution would be to escape the internal quotes, like \". But I think that's less preferable because it makes it a bit harder to read — it takes a moment to work out that the backslashes aren't ultimately there in the string being represented.

Ah of course, sorry I should've checked.

gnprice · 2023-08-11T19:50:07Z

lib/model/content.dart

+          // Parse one level of nested code block span nodes, for example for
+          // `span.hll` node.


gnprice · 2023-08-11T19:52:45Z

lib/widgets/content.dart

+  }
+
+  InlineSpan _buildNode(CodeBlockSpanNode node) {
+    return TextSpan(text: node.text, style: codeBlockTextStyle(node.type));


Let's also squash the commit that adds the styles:
fb44680 content: Add styles for each token type

into the commit that adds the parsing:
739faee content: Add CodeBlockSpanNode to parse span types

The parsing is only useful because of the styles, so I think it makes more sense to read that way.

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

rajveermalviya · 2023-08-12T05:57:21Z

Thanks for the review @gnprice, updated and rebased!

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

gnprice

Thanks! Just one ongoing thread now, and a couple of small other comments.

gnprice · 2023-08-14T22:50:49Z

lib/model/content.dart

+      if (span.text.isNotEmpty) {
+        // We don't want to emit unnecessary CodeBlockSpanNode with empty text
+        // which results in a TextSpan with empty text which results in
+        // nothing displayed in the end anyway.


Hmm, I see. Yeah, it's that trailing newline — so it's not that we get an empty node in the HTML, but there's that one case where we make the text empty.

In the existing code in main, that just results in calling buffer.write with an empty-string argument. Which is fine; it basically does nothing, and it's unlikely that adding our own conditional there would be any kind of optimization.

I agree it's good to have an optimization like this now. To really have its full effect, it should avoid not just adding the CodeBlockSpanNode to the list, but allocating the CodeBlockSpanNode in the first place when we're not going to need it.

In previous revisions this if check was inside the dom.Text() case but I moved it out when adding the final CodeBlockSpanNode span; because dart doesn't like that we don't assign it in the else case.

The analyzer will be completely happy with not assigning span as long as that control-flow path can't ever end up trying to read the uninitialized span. So for example one can use continue, like this:

if (text.isEmpty) { continue; } span = CodeBlockSpanNode(text: text, type: CodeBlockSpanType.text);

I think it's fine to not have anything for the dom.Element case. We're not altering the text of those, so we wouldn't get an empty span there unless the server actually emits one in the HTML.

gnprice · 2023-08-14T22:53:13Z

lib/model/content.dart

+      if (span.text.isNotEmpty) {
+        // We don't want to emit unnecessary CodeBlockSpanNode with empty text
+        // which results in a TextSpan with empty text which results in
+        // nothing displayed in the end anyway.


I'm also curious now to see the behavior on newlines that we don't strip out — that is, newlines on other than the last line. Let's add a small test case where the code in the code block is a couple of lines long, and has syntax highlighting.

gnprice · 2023-08-14T22:59:20Z

lib/model/content.dart

+        case dom.Element(:final text, :final classes, :final localName)
+          when localName == 'span'
+            && classes.length == 1:
+          final CodeBlockSpanType type = codeBlockSpanTypeFromClassName(classes.first);


Suggested change

case dom.Element(:final text, :final classes, :final localName)

when localName == 'span'

&& classes.length == 1:

final CodeBlockSpanType type = codeBlockSpanTypeFromClassName(classes.first);

case dom.Element(localName: 'span', :final text, :final classes)

when classes.length == 1:

final CodeBlockSpanType type = codeBlockSpanTypeFromClassName(classes.first);

The interesting part of this is moving the localName check inside the pattern.

Also it gets confusing for the when to be at the same indentation level as the body — particularly once the when itself is on the line right before the body — so we can indent it a second level. (Similar to what we do with multiline if conditions, like in parseInlineContent above.)

gnprice · 2023-08-14T23:01:43Z

lib/model/code_block.dart

+    'gt' => CodeBlockSpanType.genericTraceback,
+    _ => CodeBlockSpanType.unknown,
+  };
+}


nit: last line is missing its newline

(git diff or git log -p will highlight this for you:

+ }; +} \ No newline at end of file

)

gnprice · 2023-08-14T23:09:05Z

lib/model/content.dart

+            case CodeBlockSpanType.unknown:
+              return UnimplementedBlockContentNode(htmlNode: divElement);


This unimplemented case is an unusual one in that (a) we totally expect it to come up from time to time, as Pygments adds new token types and the server takes Pygments upgrades; (b) there's a natural sensible way to render it for the user when we do.

So let's add a comment to remind ourselves to handle this case specifically, as part of #194:

Suggested change

case CodeBlockSpanType.unknown:

return UnimplementedBlockContentNode(htmlNode: divElement);

case CodeBlockSpanType.unknown:

// TODO(#194): Show these as un-syntax-highlighted code, in production.

return UnimplementedBlockContentNode(htmlNode: divElement);

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

rajveermalviya · 2023-08-15T02:58:33Z

Thanks for the review @gnprice, rebased with the suggested changes.

gnprice

Thanks @rajveermalviya for all your work on this!

This all looks great, with one small nit. I'm going to fix that and merge.

gnprice · 2023-08-17T03:35:02Z

test/model/content_test.dart

+      ]),
+    ]);
+
+  testParse('parse code blocks, multiline, with sytax highlighting',


nit:

Suggested change

testParse('parse code blocks, multiline, with sytax highlighting',

testParse('parse code blocks, multiline, with syntax highlighting',

- Implement parsing of all the pygments token types that can be emitted by pygments as span classes. - Add styles adapted from web frontend's css for the code block spans. Fixes: zulip#191

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

gnprice reviewed Jul 26, 2023

View reviewed changes

rajveermalviya force-pushed the syntax-highlighting branch from 9d53eff to 1713963 Compare July 26, 2023 13:53

rajveermalviya force-pushed the syntax-highlighting branch from 1713963 to 6caa441 Compare July 27, 2023 01:47

rajveermalviya force-pushed the syntax-highlighting branch from 6caa441 to 4c6e7d1 Compare July 27, 2023 04:17

rajveermalviya requested a review from gnprice July 27, 2023 04:57

gnprice reviewed Aug 1, 2023

View reviewed changes

rajveermalviya force-pushed the syntax-highlighting branch 2 times, most recently from d81888d to 9eaa9d7 Compare August 5, 2023 17:37

rajveermalviya force-pushed the syntax-highlighting branch from ddb8302 to 48110bb Compare August 8, 2023 18:20

rajveermalviya requested a review from gnprice August 8, 2023 18:22

gnprice reviewed Aug 10, 2023

View reviewed changes

rajveermalviya force-pushed the syntax-highlighting branch from 48110bb to 54b3614 Compare August 10, 2023 09:29

rajveermalviya force-pushed the syntax-highlighting branch from 54b3614 to 2726f47 Compare August 10, 2023 09:56

rajveermalviya requested a review from gnprice August 10, 2023 10:16

rajveermalviya force-pushed the syntax-highlighting branch from 2726f47 to 8d2f70d Compare August 10, 2023 13:41

gnprice reviewed Aug 11, 2023

View reviewed changes

rajveermalviya force-pushed the syntax-highlighting branch from 8d2f70d to 52be6ad Compare August 12, 2023 05:44

rajveermalviya force-pushed the syntax-highlighting branch from 52be6ad to 6ebca09 Compare August 12, 2023 06:08

rajveermalviya requested a review from gnprice August 12, 2023 06:08

gnprice reviewed Aug 14, 2023

View reviewed changes

gnprice mentioned this pull request Aug 14, 2023

Revise "unimplemented content" UX for general release #194

Open

rajveermalviya force-pushed the syntax-highlighting branch from 6ebca09 to 265d288 Compare August 15, 2023 02:55

rajveermalviya requested a review from gnprice August 15, 2023 23:59

gnprice approved these changes Aug 17, 2023

View reviewed changes

rajveermalviya added 3 commits August 16, 2023 20:39

content: Implement syntax highlighting in CodeBlock

331dd79

- Implement parsing of all the pygments token types that can be emitted by pygments as span classes. - Add styles adapted from web frontend's css for the code block spans. Fixes: zulip#191

content: Match CodeBlock border-radius with web

863adbc

content: Move CodeBlock padding to inside scroll view

5e6a0ec

This way, when the text is wide and needs to scroll, the padding scrolls with the text, rather than reducing the size of the viewport. See screenshots: zulip#242 (comment)

gnprice force-pushed the syntax-highlighting branch from 265d288 to 5e6a0ec Compare August 17, 2023 03:40

gnprice merged commit 5e6a0ec into zulip:main Aug 17, 2023

rajveermalviya deleted the syntax-highlighting branch August 17, 2023 05:21

		// .gd { color: hsl(0deg 100% 31%); }
		final _kCodeBlockStyleGd = TextStyle(color: const HSLColor.fromAHSL(1, 0, 1, 0.31).toColor(), fontStyle: FontStyle.italic);

		// .hll { background-color: hsl(60deg 100% 90%); }
		final _kCodeBlockStyleHll = TextStyle(backgroundColor: const HSLColor.fromAHSL(1, 60, 1, 0.9).toColor());

	class CodeSpanNode extends BlockContentNode {
	class CodeBlockSpan extends InlineContentNode {

		child: Padding(
		padding: const EdgeInsets.fromLTRB(7, 5, 7, 3),

	return (CodeBlockSpanNode(text: text,type: type), true);
	return (CodeBlockSpanNode(text: text, type: type), true);

	expect(find.text('class A {}'), findsOneWidget);
	expect(find.text('class A {}'), findsOneWidget); // TODO(#232): use package:checks

		// We assume that server will always provide us with a span element
		// that has at most one class associated with the span element.

		testParse('parse code blocks, unknown span type',
		// A code block span with unknown className.

	// "```\n::markdown hl_lines="2 4"\n# he\n## llo\n### world\n```"
	// """```\n::markdown hl_lines="2 4"\n# he\n## llo\n### world\n```"""

		// Parse one level of nested code block span nodes, for example for
		// `span.hll` node.

		case CodeBlockSpanType.unknown:
		return UnimplementedBlockContentNode(htmlNode: divElement);

	testParse('parse code blocks, multiline, with sytax highlighting',
	testParse('parse code blocks, multiline, with syntax highlighting',

content: Implement syntax hightlighting for code blocks #242

content: Implement syntax hightlighting for code blocks #242

Conversation

rajveermalviya commented Jul 25, 2023 • edited Loading

Context

Implementation details

Notes

Testing

rajveermalviya commented Jul 25, 2023

gnprice 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

rajveermalviya commented Jul 26, 2023

rajveermalviya commented Jul 26, 2023 • edited Loading

gnprice commented Jul 26, 2023 • edited Loading

gnprice commented Jul 26, 2023

rajveermalviya commented Jul 27, 2023

gnprice 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

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 Aug 5, 2023

gnprice 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

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 Aug 10, 2023

rajveermalviya commented Aug 10, 2023

gnprice 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

rajveermalviya commented Jul 25, 2023 •

edited

Loading

rajveermalviya commented Jul 26, 2023 •

edited

Loading

gnprice commented Jul 26, 2023 •

edited

Loading