Refactor caret and selection code #1309
Conversation
|
Note that this code is very similar and I think it could be unified. Essentially the selection change of the caret should be the same as changing an interval where |
| * if it's a removal of text, hence capping to 0).</li> | ||
| * </ul> | ||
| */ | ||
| private static int applyChange(int position, int changeStart, int changeEnd, int netLength) { |
There was a problem hiding this comment.
The following are just my preferences to making the code easier to understand. If you have strong reasons for not using them, that's fine:
- remove static: it seems unnecessary, as it's not for instance being used by a test. Also see next ....
- change return to void: position is a field so update it directly. It's confusing, for me a least, when the value is being changed here, then returned, only to update the field by the caller. The only reason it's like this at the moment is because the method is static.
- remove the position parameter: it's a field variable so doesn't need to be passed all the time. Again, the only reason it's like this is because the method is static.
- if you decide to keep the position parameter then please give it a different name to distinguish it from the field variable.
There was a problem hiding this comment.
I think both a method or a static function are good solutions here, I don't have a strong preference. I'm totally ok changing to a method (and your comments 2/3/4 makes sense with that change).
I'll do ti.
To add some context, I initially extracted this bit of code in CaretPositionChange and SelectionChange to show that it was duplicated code. Then I adapted with the last bit which cannot be done in SelectionChange:
// ...
else if(position == changeStart) {
position += Math.max(0, netLength);
}There was a problem hiding this comment.
Actually, sorry to contradict myself, I just made the change and then looked at SelectionChange where I cannot make it a method (because it applies to two different values). So I propose that we keep the static to keep evident the close correspondance between the code in these two classes, if that's ok for you.
|
I think you need to rebase or merge from master ? |
|
I already did the pull from master normally (and I see the PR states there are no conflicts) |
|
Ahh, seems my side was out of sync for some reason - strange. |
2 participants
After the merged PR #1303 and #1304 the code is merged.