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

Fire addMarker event in data pipeline if marker is at element boundary #9897

Merged
merged 8 commits into from
Jun 29, 2021

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Jun 17, 2021

Suggested merge commit message (convention)

Fix (engine): Fire addMarker event in data pipeline if collapsed marker is at element boundary. Closes #8485.

@niegowski niegowski self-requested a review June 17, 2021 08:12
@@ -250,7 +250,7 @@ export default class DataController {
// Convert markers.
// For document fragment, simply take the markers assigned to this document fragment.
// For model root, all markers in that root will be taken.
// For model element, we need to check which markers are intersecting with this element and relatively modify the markers' ranges.
// For model element, we need to check which markers are contained in this element and relatively modify the markers' ranges.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment change is not entirely true - markers that are partly in that element are also included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewritten a bit.


if ( intersection ) {
result.push( [ marker.name, intersection ] );
const isMarkerCollapsedAtElementBoundary = markerRange.isCollapsed && ( markerRange.start.isAtStart || markerRange.start.isAtEnd );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const isMarkerCollapsedAtElementBoundary = markerRange.isCollapsed && ( markerRange.start.isAtStart || markerRange.start.isAtEnd );
const isMarkerCollapsedAtElementBoundary = markerRange.isCollapsed && ( markerRange.start.isEqual( elementRange.start) || markerRange.start.isEqual( elemenetRange.end ) );

Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth adding tests for the previous implementation - with a collapsed marker that is at the start or end of some other element (not included in the checked element) and for a collapsed marker at the start or end inside some deeper nested element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

} );
} );

data.toView( root );
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's check how this looks in the view as in other tests it's done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote the tests to make them look similar to others. I didn't use the markerToHighlight conversion, because it seems that it does not allow creating a collapsed marker. Instead, I'm using now markerToData helper.

@psmyrek psmyrek requested a review from niegowski June 25, 2021 13:42
@niegowski niegowski merged commit 91d61dd into master Jun 29, 2021
@niegowski niegowski deleted the ck/8485 branch June 29, 2021 14:23
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.

addMarker event is not called in data pipeline when range is at start of the root element
2 participants