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

Already on GitHub? Sign in to your account

Deleting all the content of an inline editor throws an error #1764

mcorlan opened this Issue Oct 4, 2012 · 9 comments


None yet
5 participants

mcorlan commented Oct 4, 2012

The error is: Typing in Editor should not destroy its own _visibleRange
and it is thrown from Editor.prototype._handleEditorChange()

@ghost ghost assigned RaymondLim Oct 4, 2012


njx commented Oct 10, 2012

Reviewed. Marking medium priority. It currently only happens with "delete line" (if you do a select all and delete in the inline editor it doesn't happen). This is probably because "delete line" is the only way to get rid of the final newline in the inline editor. The inline editor/doc syncing code should be robust to this (we shouldn't expect all extensions to handle this kind of special case).

@ghost ghost assigned njx Oct 22, 2012


pthiess commented Oct 22, 2012

Please add this to the bug fix story.


peterflynn commented Oct 22, 2012

I don't think there's a good way to fix up the heuristics to allow this case -- we might just have to remove the assertion.

The reason this couldn't be fixed easily is because of the ambiguity around undo changes vs. regular edits. We "lose sync" whenever there's an edit spanning the boundary because afterward, changes just outside the new boundary are ambiguous: it's unclear whether the boundary should be adjusted to include those changes or not.

If I recall, earlier our heuristic waited until we actually got an ambiguous change before "losing sync," which would allow deleting the line but not undoing it afterward. I think that was one of the main reasons we moved away from that heuristic -- it could feel scary that the entire inline editor disappears when you hit undo in those cases. (Plus the other edge cases where edits in a different editor would cause the inline to close even though the edits appeared to the user to be cleanly outside its bounds).


njx commented Oct 24, 2012

Good points. Thinking about this some more, I think we should do both: (1) remove the assertion, and (2) also fix up delete line so it doesn't actually delete the newline in this case. It's true that not everyone might think to handle this case properly, so removing (1) makes it so that an edit command that doesn't handle this properly will just cause the editor to collapse rather than throwing. But we should probably actually fix up whatever edit commands we can to have decent heuristics.

Though one could argue that if you delete the last line in an inline editor, it should just close. I'm not convinced, though. I feel like the intention would generally be to type some new code immediately afterward (similar to doing select all/delete).


peterflynn commented Oct 24, 2012

It seems like in general, editing commands that have a boundary condition on the last line in the file should be changed so they behave the same for the last line in visibleRange too. In both cases, commands that normally touch the trailing newline typically should change to touch the preceding newline instead.

I can repo this same issue with the Move Line Up command, btw.


njx commented Oct 24, 2012

Hmmm. It looks like there are a number of problems with Move Line Up/Down that are trickier to fix. I started trying to fix them as part of this bug fix, but it's a little hairier.

I'm going to go ahead and put up a pull request for this bug, and open a separate bug for Move Line Up/Down. In general, when reviewing new editor commands, we'll need to make sure the submitter has thought through the behavior in inline editors.


peterflynn commented Oct 26, 2012

@mcorlan: can you verify this is working as you expected now? Thanks!

@ghost ghost assigned mcorlan Oct 26, 2012


mcorlan commented Oct 28, 2012

@peterflynn just synched my repository and tried the command. It works. As far I can tell, in inline editors when deleting everything there is left an empty line which is fine by me. This is what I'd expect.

Awesome fix!


peterflynn commented Oct 29, 2012

Cool, thanks for verifying! Closing...

@peterflynn peterflynn closed this Oct 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment