-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Cursor movement inconsistent around collapsed inclusiveLeft/Right #3658
Comments
This may not be consistent in terms of symmetry between these cases, but it does seem to be consistent in terms of actual behavior. Since you're setting Why are you including your sentinels in the actual text at all? Wouldn't it make more sense to keep them out of the document, and track them in your own data structure? |
I'd agree that the approach you describe is probably better, but I'm mostly trying to maintain an extant library. It'd probably be very difficult to rewrite this bit at the moment. Firepad is trying to have invisible sentinels at the start of each line. Using |
Could you say more about these sentinels? Why aren't they implicit in the fact that the line starts there? |
Well, in Firepad, these sentinels store additional rich-text formatting information. I don't quite understand the entire system back to front, but basically Firepad is using an OT system wherein metadata like that has to be part of the document in order to be versioned appropriately by OT. You can read part of the implementation that deals with sentinels here: https://github.com/firebase/firepad/blob/master/lib/rich-text-codemirror.js. I'm mostly trying to follow along and diagnose cursor movement bugs in this part of the code. To me it seems that it is not entirely clear what the difference between |
If you have a line
Does that help? |
The code is now clearer, and will prefer the nearer end of the atomic range as long as that means there is still progress (i.e. the cursor does not get stuck). Issue #3658
Looking through the (terribly dense) code for this, I realized that it was actually workable to change the behavior so that, when coming from one side into an atomic range, if there is a valid position that is at the side that we are coming from, but is not equal to our old position, we choose that. I've implemented that in attached patch. This should address your issue, I believe. |
@marijnh Thank you very much again for the patch, it almost addresses all of our issues (I think). Here's a nagging case where it doesn't quite work:
At this point you would expect the cursor to move to the second line, but instead moves to the end of the first (after the I dug into your refactor and I suspect (through my admittedly limited understanding) that the issue is related to this line: https://github.com/codemirror/CodeMirror/blob/710f43a1c55ffa4102ef4c3fce46329ad5643da7/lib/codemirror.js#L2253. The way I read that line seems to say to me "If we are moving rightwards, and approach a span with My logging seems to indicate that |
Which is kind of arbitrary but seems to give less surprising results. Issue #3658
Hm. That is, again, the correct outcome of the algorithm, but indeed surprising. So I've changed the algorithm again, and now it only uses the 'near' side of an atomic marker when it is on the same line as the origin goal pos. That's somewhat arbitrary, but fixes this specific case. |
Thank you very much for the followup patch. It seems to address our issues, which I very much appreciate. |
We are in the process of updating Brackets to codemirror 5.11 and found that this change affected some of our unit tests. The issues are around the specific check Here is a lil history if you folks wanna take a quick peek. To see the issue: 1 go to https://codemirror.net/index.html
3 Select anything spanning multiple lines. I expect (and the unit tests as well) that all three of the following selections would generate the same result... But the first selection has unexpected results.
4 The selection issue can be revealed by clearing the ranges after trying each selection in the previous step.
5 Checkout the selected range.
Please let me know if I can help clarify follow the steps or understand more of the issue. Thanks a million!! |
Description
When using collapsed ranges with
inclusiveLeft
, CodeMirror has trouble moving the cursor around the ranges, but seems to handleinclusiveRight
naturally enough.Reproduction
Go to https://codemirror.net
Open a console and run
Place your cursor anywhere in the document and hit up or down arrow keys.
The cursor will move one line downward on down, but two lines upwards on up.
Now try again with
inclusiveRight
:Cursor movement should work as expected, one line per press
Related
Problem seems very similar to the one mentioned in a previous issue I opened, #2635, and is probably the root cause of FirebaseExtended/firepad#82.
Firepad uses collapsed line sentinels at the start of every line to mark metadata, but struggles with blank lines where the only character on a line is a collapsed range with
inclusiveLeft
.The text was updated successfully, but these errors were encountered: