Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In OpAddAnnotation support annotated ranges with 0 length #879

Merged
merged 1 commit into from
Mar 26, 2015

Conversation

kossebau
Copy link
Contributor

ODF has the concept of annotations with and without a range (either there is an <annotation-end> element or not). One could argue that an annotation with a range 0 (thus no content between start tag and end tag) has the same semantics as an annotation without an end tag.

Just, as a matter of fact currently the editing ops make a difference between both. And currently it is also not supported to change an annotation to be either range-less or ranged. So when removing all content between start and end tag, an annotation still stays ranged.
Ideally that would get a proper solution one day...

In the meantime, while pondering with OT for the annotation ops, I found it would be needed to have the AddAnnotation op to also support annotated ranges of length 0, and not just to assume a parameter of length=0 means unranged annotation.

So it makes the op match the full set of possibilities with ranged annotations. And it helps with OT for annotation ops :)

Non-backward compatible spec change (due to change of meaning of length=0).

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2344/

@@ -6,6 +6,14 @@

* Fix breaking all empty annotations on merging the paragraph they are contained in with the one before ([#877](https://github.com/kogmbh/WebODF/pull/877)))

### Improvements

* In OpAddAnnotation support annotated ranges with 0 length (WARNING: NON-BACKWARD COMPATIBLE spec change) ([#879](https://github.com/kogmbh/WebODF/pull/879)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usual terminology for this is a "breaking change" (in case you weren't aware)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never heard before, thanks for the hint. Suspect that is MS lingo, so that could be why ;)
Still, seems it is quite in use and also a short term, so happy to use it.
Hm, wondering if that should be a separate section even, and not just a inline comment. Guess it should.

@peitschie
Copy link
Contributor

A simple enough change... makes good sense to me. Please :shipit: or 🚀 if you prefer!

@kossebau kossebau force-pushed the addAnnotationWith0Length branch from bed99be to 1d24d46 Compare March 26, 2015 12:42
Before 0 length was assumed to be an annotation without a range
@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2345/

@kossebau
Copy link
Contributor Author

🍪s for the review, @peitschie Hm, out of rockets, so taking the 🚢

kossebau pushed a commit that referenced this pull request Mar 26, 2015
In OpAddAnnotation support annotated ranges with 0 length
@kossebau kossebau merged commit d91660b into webodf:master Mar 26, 2015
@kossebau kossebau deleted the addAnnotationWith0Length branch March 26, 2015 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants