Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/389 ModelRange#getTransformedByMove and other #392

Merged
merged 18 commits into from
May 6, 2016
Merged

T/389 ModelRange#getTransformedByMove and other #392

merged 18 commits into from
May 6, 2016

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented May 4, 2016

This PR combines changes from ckeditor/ckeditor5#3692 which are required by undo manager. The biggest change is introducing engine.treeModel.Range#getTransformedByMove method.

@scofalik
Copy link
Contributor Author

scofalik commented May 4, 2016

Follow up #393

@@ -307,16 +311,15 @@ export default class Document {
* * 'removeRootAttribute' when attribute for root is removed,
* * 'changeRootAttribute' when attribute for root changes.
*
* Change event is fired after the change is done. This means that any ranges or positions passed in
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was not true - data.range is indeed after the change, but data.position is before the change. I added additional note in params comments.

@Reinmar
Copy link
Member

Reinmar commented May 5, 2016

Important reminder – please remember to rebase your branch onto the latest master before putting it on review.

}

return transformed;
}

/**
* Returns all deltas from history, starting from given history point (if passed).
*
* @param {Number} from History point.
Copy link
Member

Choose a reason for hiding this comment

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

Missing @returns. Can't it be a generator, btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@scofalik
Copy link
Contributor Author

scofalik commented May 6, 2016

Important reminder – please remember to rebase your branch onto the latest master before putting it on review.

Is there really a difference? I always rebase with master (git rebase master, I might forgot once) and there were never any problems with my PRs (I mean they were but because of different stuff). And when I tried the other method I got some problems. I have experience using my workflow so if there isn't big difference I'd like to kindly ask if I can keep doing as I was always doing?

I also find git history much more messy when doing stuff your way.

@Reinmar
Copy link
Member

Reinmar commented May 6, 2016

@scofalik: I was asking exactly for rebasing :D. We only need to merge once an issue is already on master.

Anyway, it's not a big issue – it just may delay or confuse the reviewer. We had some issues because of that in the past. Especially, when more things depend on each other, being really up to date with your changes is important to ensure that tests pass.

@scofalik
Copy link
Contributor Author

scofalik commented May 6, 2016

Sure, sure. I understand the point of being up-to-date with master :), I just thought you are talking about different command.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants