Handle the case when a code block has no grandchildren #919
Conversation
|
#917 is stacked on top of this. |
| @@ -344,6 +344,14 @@ class ContentExample { | |||
| ]), | |||
| ]); | |||
|
|
|||
| static final codeBlockWithEmptyBody = ContentExample( | |||
There was a problem hiding this comment.
Let's have a comment that makes it clear we don't expect this in messages from current servers. The link to the ancient message you put in the PR description is helpful; we could include that here too.
| final grandchild = child.nodes[child.nodes.length - 1]; | ||
| final grandchild = child.nodes.last; |
There was a problem hiding this comment.
Is this a pure refactor, or does this change behavior somehow? I noticed there's a line final first = child.nodes[0]; above this; should that also be changed to say child.nodes.first?
There was a problem hiding this comment.
This refactoring is purely for readability. Let's get an nfc commit for both.
|
Thanks! I happened to check in on this PR and noticed that the current revision (ending in fb5ea67) responds to my review, so I'm comfortable marking this as ready for Greg's review. |
chrisbobbe
added
integration review
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Thanks @PIG208 for the fix, and @chrisbobbe for the reviews!
Generally this looks good. Two small comments below.
lib/model/content.dart
Outdated
| if (divElement.nodes.length != 1) return null; | ||
| final child = divElement.nodes[0]; | ||
| final child = divElement.nodes.first; |
There was a problem hiding this comment.
If we use a fancy getter here, it should be single — that way we correctly express the expectation that this is the only node (not the first of potentially several nodes).
| if (child is! dom.Element) return null; | ||
| if (child.localName != 'pre') return null; | ||
|
|
||
| if (child.nodes.length > 2) return null; | ||
| if (child.nodes.length > 2 || child.nodes.isEmpty) return null; |
There was a problem hiding this comment.
nit: include the key "why" information in the commit message, rather than rely on the PR description. We aim for the Git history to be able to serve as our primary record of what we did.
In particular key points include
- this was a crash bug;
- the affected messages in public chat.zulip.org history ranged from message ID 5020 to 29855, all in 2016.
Details that can be read off from the commit's added test case don't need to be repeated in the commit message, though.
PIG208
force-pushed
the
content
branch
2 times, most recently
from
72ad468
to
6cf84df
Compare
|
Thanks for the review. Updated the PR! |
This improves readability, especially in the .last case. Signed-off-by: Zixuan James Li <[email protected]>
A code block containing no grandchildren (i.e. a <div class="codehilite"> element containing an empty <pre> block) appears in some very old messages. The parser was crashing on them. The affect messages in public https://chat.zulip.org history ranged from message ID 5020 to 29855, all in 2016. Signed-off-by: Zixuan James Li <[email protected]>
|
Thanks! Looks good; merging, with a tweak to clarify the commit message a bit: |
For some very old messages (e.g.: this), a code block might contain no grandchildren, or equivalently, the
<pre></pre>block within the<div class="codehilite"></div>element is empty.While these messages might be present on CZO only due to some ancient server bugs, the client should not crash when it does encounter them.