Test content parsing; use Diagnosticable; write docs for ContentNode etc. #205
Conversation
lib/model/content.dart
Outdated
| // We don't actually seem to need this information. | ||
| // We don't actually seem to need this information in code. Instead, | ||
| // the inner text already shows how to communicate it to the user. | ||
| // (E.g., silent mentions' text lacks a leading "@".) | ||
| // final UserMentionType mentionType; | ||
| // final bool isSilent; |
There was a problem hiding this comment.
Interesting! The added explanation does help. 🙂
I wonder, though: does the inner text really make these two fields unnecessary? It seems convenient to have this information in this nice, structured way.
- How can we distinguish a user mention from a group mention at least as efficiently as this? The inner text will just be the name of a user group or a user, like "mobile" or "Chris Bobbe", right? So we'll have to look in our data structures for the answer, and to do that efficiently we'd at least have to maintain dictionaries keyed by those names.
- I think I've discovered a case where the inner text of a silent mention could start with "@": consider a silent mention of a user group that starts with "@".
(Hmm, and might we need to recognize wildcard mentions too, like @all?)
There was a problem hiding this comment.
Definitely if/when we end up adding code that cares whether it's silent, or to a group, etc., we'll want to record that on the node, based on the classes the HTML element has. I wouldn't want to go inspect the text for an "@" to try to infer that (except as part of some regrettably-necessary workaround).
In particular I think we'll need to distinguish user vs. group when we go to do #157, the message-list redesign. In the pre-2023 design, though, both kinds of mentions are styled the same way, with the same gray-to-white gradient.
- I think I've discovered a case where the inner text of a silent mention could start with "@": consider a silent mention of a user group that starts with "@".
Looks confusing! We should maybe not allow user-group names (or user names, for that matter) to start with an "@".
There was a problem hiding this comment.
Looks confusing! We should maybe not allow user-group names (or user names, for that matter) to start with an "@".
test/model/content_checks.dart
Outdated
| extension InlineContainerNodeChecks on Subject<InlineContainerNode> { | ||
| Subject<List<InlineContentNode>> get nodes => has((n) => n.nodes, 'nodes'); | ||
| } | ||
|
|
||
| // TODO write similar extensions for the rest of the content node classes |
There was a problem hiding this comment.
content test: Extend equalsNode to cover all node types
Is this TODO still needed at this commit?
There was a problem hiding this comment.
Thanks, yeah — looks like it's not.
There are some node classes with fields that aren't covered here — but those are classes with structural ==, and it's not clear we'll ever want Subject getters for them.
I wasn't sure what this comment meant when I came across it recently, even though I wrote it myself. Today I was writing tests for this code and saw what the data looks like, and the meaning became clear. Expand the comment so that it's hopefully clear to other readers without having to see the data.
This abstraction doesn't pull a lot of weight yet, but it will when we start tracking LinkNode descendants, for zulip#71.
As their new common superclass BlockInlineContainerNode shows, these two have a lot in common.
|
Thanks for the review! Merged, with those tweaks. |
This lays all the foundations we need in order to write tests for our content-parsing code, and then does so for all the existing code.
We'll want such tests for #71, which will add a new form of complication to that code. We'll also want it as we implement more features in the content parsing in general.
To get helpful debugging output when tests fail, we use the Diagnosticable class from Flutter to produce readable printouts of the content nodes:
https://api.flutter.dev/flutter/foundation/Diagnosticable-mixin.html
https://api.flutter.dev/flutter/foundation/Diagnosticable/debugFillProperties.html