Fix OpMergeParagraph merging paragraphs with empty annotations #877
Conversation
| * @return {?Node} parent of targetNode | ||
| */ | ||
| function removeUnwantedNodes(targetNode, shouldRemove) { | ||
| function removeUnwantedNodes(targetNode, shouldRemove, shouldSkip) { |
There was a problem hiding this comment.
The introduction of the shouldSkip makes this look more and more like it should follow the existing TreeWalker pattern of using return values rather than boolean filters (e.g., return NodeFilter.REJECT = delete, NodeFilter.SKIP = skip, NodeFilter.ACCEPT = keep).
The point of ambiguity with the interface here is what happens if shouldSkip & shouldDelete are both true?
You could easily fix that in the API documentation, however.
There was a problem hiding this comment.
Hm, I see your point. Indeed would make this method more flexibel if the callee has control about how shouldSkip & shouldDelete relate to each other. Will give it a try.
|
Other than that small thing, the change looks fine to me. The build-bot hasn't picked this up? |
|
test this please |
|
Build failed. |
|
test this please |
1 similar comment
|
test this please |
|
Build succeeded. |
|
Buildbot needed to be fed with enough working memory again... @peitschie So how do you like the other variant you proposed, now that you can see it in code? Myself like it slightly better than the first version, for the reason you gave. |
|
Yep... I like this a lot better too! Thanks for your rework. 👍 Please send this 🚢 off into the ocean that is webodf master. |
kossebau
force-pushed
the
fixOpMergeParagraphAndEmptyAnnotations
branch
from
86a9e0c
to
45726eb
Compare
|
Build succeeded. |
|
Thanks for your good suggestion! |
…tations Fix OpMergeParagraph merging paragraphs with empty annotations
Before it removed the
<text:p/>of empty annotations. Not nice.This fix makes sure embedded roots are not cleaned.