Migrate from GitHub to SourceForge with this tool. Check out all of SourceForge's recent improvements.
Close

#1361 Wrong caret position after undo

Bug
closed-fixed
Scintilla (796)
3
2012-07-14
2012-05-03
No

In certain situations, caret will not be positioned properly after undo.

Example:
1. Write 'aaa'
2. Delete just inserted text, one letter at a time starting from the first 'a'
3. Undo
4. Main caret is now wrongly placed after the first 'a' letter, instead at the end

Discussion

1 2 > >> (Page 1 of 2)
  • Neil Hodgson

    Neil Hodgson - 2012-05-03

    Editor is notified about each undo step (character) separately. Something could be done with SC_MULTISTEPUNDOREDO and some remembered state but that would add complexity. Another approach would be to convert a sequence of automatically coalesced actions into a single action inside UndoHistory.

    Its unlikely I'll work on this in the near future.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-03
    • assigned_to: nobody --> nyamatongwe
    • priority: 5 --> 3
    • status: open --> open-accepted
     
  • Marko Njezic

    Marko Njezic - 2012-05-04

    I've attached a patch that fixes this particular situation with caret positioning when undoing.

    Redo is not affected as reversed character insertion is not automatically coalesced.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-06

    This may cause problems for existing applications which count undos and redos to be able to match the current state with other features they are driving like change bars.

     
  • Marko Njezic

    Marko Njezic - 2012-05-06

    I strongly disagree with that. All the steps remain completely unchanged and so are the positions and lengths reported by them in notifications. Therefore there will be absolutely no change to the applications that count undo and redo. Only the caret position returned at the completion of Undo is adjusted in case of a coalesced deletion. If there is an odd chance that there is an application that does something with the end caret position (which would be pointless in case of an explicitly grouped undo operations anyway, where caret can move quite a lot), such application can be adjusted by using the same logic like in the patch. Take one more look at my patch, as you apparently did not understand it properly at the first look.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-07

    Yes, I misread the code.

     
  • Marko Njezic

    Marko Njezic - 2012-05-23

    I'm pinging this item, since my patch has not been committed yet.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-23

    The patch is not minimal. I'll refine it but its low priority.

     
  • Marko Njezic

    Marko Njezic - 2012-05-24

    I don't see what needs refining as the patch is already very compact and contains only modifications related to undo. Just compare it with this modification for example: http://scintilla.hg.sourceforge.net/hgweb/scintilla/scintilla/rev/f8359cac73fb Two completely separate features committed at the same time.

    Anyway, to speed up inclusion and save you from spending time "refining" my patch, I have made a split version of the patch so that each piece is even smaller. New files are attached.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-25

    Fix with less lines

     
  • Marko Njezic

    Marko Njezic - 2012-05-25

    I don't understand what is the point of you attaching "undo-minimal.patch" now? It is *exactly* the same as scintilla-fixes_4174.patch that I have already attached.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-25

    We should always strive to improve so arguments of the form "change X did something poorly so that something should be performed poorly in the future" are unreasonable and I will not consider them. Your amended patches don't simplify anything since the unnecessary changes are a pre-requisite to the fix.

    The original patch is 91 lines long excluding header. A patch that discards the code 'improvements' is 43 lines. See undo-minimal.patch. I don't much like peeking at the next action either - the next loop will see that anyway. Now we have something that can be more easily be examined. One change in behaviour is that its not just the simple case described that causes coalescing but also some combinations: undoing <Del><Del><BS><BS> moves the caret to the end of the deletion but undoing <BS><BS><Del><Del> leaves the caret half-way. This wasn't mentioned in the patch or this tracker so perhaps its unintentional. I'd expect any intentional change to coalesce beyond the simple case to cover both sequences.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-26

    Added another patch which coalesces undo for various sequences of <BS> and <Del>. It may also include a deletion after a cursor move if that sequence was inside Begin/EndUndoAction but the consequences should still be OK.

     
  • Marko Njezic

    Marko Njezic - 2012-05-26

    > The original patch is 91 lines long excluding header. A patch that discards
    > the code 'improvements' is 43 lines.

    Those code changes that you ironically referred to as improvements written in quotes were just a first attempt at loop code simplification. Not to mention that performing the same check for a condition that doesn't change inside a loop is a very bad coding practice, but it doesn't affect the performance here, so that was not a motive for a change. There is quite a lot of code duplication inside the loop, which should be removed as that will simplify the loop considerably and make it easier to understand by someone else. Your typical response here would probably be comparison of source code with butter, but don't bother.

    > I don't much
    > like peeking at the next action either - the next loop will see that
    > anyway.

    The reason why I wrote it like that is because otherwise in order to implement this properly I would have to keep previous state and then reset it at appropriate places. Resetting previous state would add more conditional checks to already unnecessary complicated loop code and I did not want to spend time on that.

    > One
    > change in behaviour is that its not just the simple case described that
    > causes coalescing but also some combinations: undoing <Del><Del><BS><BS>
    > moves the caret to the end of the deletion but undoing <BS><BS><Del><Del>
    > leaves the caret half-way. This wasn't mentioned in the patch or this
    > tracker so perhaps its unintentional.

    There was nothing unintentional in my patch. My original report was about coalescing <DEL> operations and that's what I did. The difference that you pointed out stem from the behavior of backspace operation. Positioning after repeated <BS> operations was already correct thanks to the order of deletions. Ideally undo should return correct positions after all automatically coalesced operations mimicking coalescing performed by UndoHistory, but handling the second case always correctly will be a little tricky in case that there are interleaved container actions at the breaking point between <BS> and <DEL>.

    > I'd expect any intentional change to
    > coalesce beyond the simple case to cover both sequences.

    I'd expect that selections were also restored by Scintilla's undo implementation, but we don't live in a perfect world.

    > Added another patch which coalesces undo for various sequences of <BS> and
    > <Del>. It may also include a deletion after a cursor move if that sequence
    > was inside Begin/EndUndoAction but the consequences should still be OK.

    Your new patch is a typical example of a quick hack done without thinking, or should I say a "refinement".

    - You did not take grouped actions into account at all. You are not resetting last coalescing position properly, which prevents coalescing from working properly in certain situations. You are also wrongly stopping all coalescing operations after encountering the first insert action.
    - You did not take into account interleaved container actions inside a larger grouped undo operation, which can break coalescing depending on their coalesce parameter.
    - As stated you are aware of this, but I'm pointing it out because the consequences are not "OK". Unintentionally including deletion after cursor move in grouped undo can lead to confusion. In order to be consistent and avoid any confusions, coalescing performed by Document::Undo() should mimic automatic coalescing done by UndoHistory. No more and no less, until storing selections inside undo history is one day implemented.

    None of these issues exist in my patch, which mimics UndoHistory coalescing more closely.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-26

    If you want to improve code quality, use separate tracker issues. Mixing code improvements with bug fixes will result in patch rejection in the future.

    I can not think of an example which the most recent patch fails to coalesce in a reasonable manner.

     
  • Marko Njezic

    Marko Njezic - 2012-05-27

    You can't think of an example, because you're focusing only on the most simple scenario, when user presses one of the DEL or BS keys. You should be rather looking at UndoHistory (like I did) and seeing when and how it performs coalescing in order for the fix to be complete and handle all possible cases, which will avoid any confusion due to mixed behavior.

    First case which you did not take into consideration can be reproduced like this: begin grouped undo action, perform several delete operations at the same place (similar to pressing DEL key), move to some other non adjacent position and once again perform several delete operations, end grouped undo. Your patch will not properly coalesce the first deletion operation.

    Second case is pretty obvious: in a larger grouped undo, your patch will not stop coalescing when encountering an interleaved container action with mayCoalesce set to false.

     
  • Neil Hodgson

    Neil Hodgson - 2012-05-28

    If the goal of the patch was to match the coalescing performed by UndoHistory then it should be coalescing deletions that touch either forward or backward together as UndoHistory does. That would allow <BS><BS><Del><Del> to work more naturally.

     
  • Marko Njezic

    Marko Njezic - 2012-05-28

    I agree with that. I also explained previously why I did not handle that case:

    "Ideally undo should return correct positions after all
    automatically coalesced operations mimicking coalescing performed by
    UndoHistory, but handling the second case always correctly will be a little
    tricky in case that there are interleaved container actions at the breaking
    point between <BS> and <DEL>."

    However, like everything else, it can be done. The resulting code would either end up being too complex, or would require more code refactoring (reduced amount of conditional checks will make it easier to isolate every place where coalescing should reset).

     
  • Marko Njezic

    Marko Njezic - 2012-05-28

    Patch that handles both <BS> and <DEL> coalescing

     
  • Marko Njezic

    Marko Njezic - 2012-05-28

    I have attached a patch (scintilla-fixes_4187.patch) that handles both <BS> and <DEL> coalescing, thus exactly mirroring automatic coalescing performed by UndoHistory.

    The patch doesn't perform any code refactoring, only minor cleanup that I have left is removal of unused cellPosition variable (it is actually a leftover from the time when both substance and style were stored in one buffer and value divided by two was stored in that variable). I guess that you won't have any issues with this patch.

    I have also created a patch that completely refactors both undo and redo code. Performing cleanup and removing all duplicated conditional checks brought down the number of branches inside loop from current ten, down to six. However, I'll leave these modifications in my local repository for the time being.

     
  • Neil Hodgson

    Neil Hodgson - 2012-06-04

    Multiple assignments per line is not in keeping with existing Scintilla code.

     
1 2 > >> (Page 1 of 2)

Log in to post a comment.