Made it so that cursor clicks will not truncate the redo chain #10

aaxu · 2017-06-26T17:42:38Z

Currently this applies to cursor clicks and making selections with the mouse (all redoAddChanges with the tag 'm').

dschuyler · 2017-06-26T18:09:46Z

I'll want to look this over more thoroughly, but I wanted to mention right off that this looks like you hit on something important.

I've taken some steps to separate pen Row/Col from the cursor Row/Col to avoid a similar issue when moving the cursor (like with the cursor keys). Does this approach handle non-mouse cursor moves as well?

It looks like separating pen and cursor vs keeping a separate chain are two different approaches to the same issue.

aaxu · 2017-06-26T18:37:31Z

Ah yes I forgot about using the arrow keys, but I just did a quick test and since all functions and commands that move the cursor (without moving lines) have the tag 'm', moving the cursor with the arrow keys, page up, page down, home, and end will not truncate the redo chain. This also includes shift + arrow key to highlight characters.

…x/mouseTruncatesRedoChain

dschuyler · 2017-06-27T17:16:50Z

On undo/redo in general there are a few things that would be cool to eventually have:

allow multiple views (windows) into the same text buffer (each with their own cursor position)
- tie the cursor moves to the window (view) rather than the text buffer
- maybe the temp chain would be on the view?
- open question: should each view have its own redo chain?
  - I'm currently thinking that each text buffer would have a single redo chain (regardless of the number of views) but each view would have its own cursor and scroll position.
have branching undo/redo so that the chain is (nearly) never truncated
- new branches would be added, so the 'chain' would turn into a tree
saved (to disk) undo/redo
- i.e. reopen file and still be able to undo

Right now, there's a bug that moving the cursor after undoing will truncate the redo chain (which this PR addresses) and there's no current way to open two views.

What do you think of trying to use the view/window for the cursor position rather than using a temp chain?

aaxu · 2017-06-27T17:30:41Z

Sorry I'm not really understanding what your questions really mean.

What would be different if we tied the temp chain on the view?
If we were to make the redo chain into a tree, how the user decide what to redo or undo when there are many children?
Is there one view for each new path?
What would tying the redo chain to the window rather than the text buffer do? Do you mean that instead of a single text buffer, we split it up into multiple windows each with text buffers and each text buffer has it's own redo chain?

dschuyler · 2017-06-28T02:49:50Z

Thanks for bearing with me.

I mean that I'm thinking of two views (InputWindow) looking at the same TextBuffer object. I've uploaded some changes that may help illustrate what I mean in splitting the window.

After pulling in the new code, uncomment this line:
#'split': self.splitCommand, # Experimental wip.
then use ctrl+e, type split and press return. That will split the screen into two windows looking at the same TextBuffer.

Now, that works rather badly at the moment. The line numbers don't render well, and mouse events are confused. But it may be a good demo of what I'm thinking of. The dream is that the cursor, scroll position and selection can be different the two different windows.

I'm also willing to reconsider whether this is a good idea or not.

An alternative approach is to allow the scroll position to differ, but only track a single cursor position and selection. Hmm, this may be a better direction overall.

dschuyler · 2017-06-28T02:51:14Z

(I'm intentionally leaving the branching undo and saving undo for a later (or separate) chat. I think I was confusing this chat with mixing too many topics).

aaxu · 2017-06-28T05:08:19Z

I don't really know how I feel about this feature, maybe because I've never really used it before. It doesn't seem particularly useful if it's just the same document. What I usually do is I keep my cursor on some spot, scroll up and look at whatever I need to reference, and then typing will snap my view back to my cursor. I can see this somewhat useful to cross reference different files. Maybe it'll be a good feature to implement later on, although maybe it might be better if we split the screen vertically instead of horizontally? I think we should put this off though until the project is more developed first and we can finally deploy it 👍 , though that's up to you.

dschuyler · 2017-06-28T17:29:53Z

Yeah, that's reasonable. Let's move forward with this PR then since it's solving a real and current issue :)

dschuyler · 2017-06-28T17:31:22Z

app/text_buffer.py

@@ -49,6 +49,9 @@ def __init__(self):
    self.parserTime = .0
    self.relativePath = ''
    self.redoChain = []
+    self.tempChain = []  # Used to store cursor view actions without trimming redoChain


Does the tempChain every get more than one element in it?
i.e. would it make sense to make this a tempChange or something rather than a tempChain

dschuyler · 2017-06-28T17:33:56Z

app/text_buffer.py

+      if self.redoIndex < self.savedAtRedoIndex:
+        self.savedAtRedoIndex = -1
+      self.redoChain = self.redoChain[:self.redoIndex]
+      if len(self.tempChain) and self.redoChain[-1][0] == 'm':


Try:

make an edit

undo the edit (so the redoChain is empty)

move cursor (so tempChain is not empty)

press backspace (or something to get here)
problem: exception on an index error on the redoChain.

Sorry I don't think I'm doing the steps right. Can you give a more detailed example to reproduce this?

dschuyler · 2017-06-28T17:36:18Z

app/text_buffer.py

+      else:
+        self.redoChain.extend(self.tempChain)
+        self.redoIndex += len(self.tempChain)
+      self.dirtyChain = False


I may be missing something, but can we get by without the self.dirtyChain flag?

Ah yes, I changed it so now it's a local variable. I needed it to tell if there was a new trivial change in the redoAddChange function.

dschuyler · 2017-06-28T17:54:08Z

Fixes #16

…mouseTruncatesRedoChain

…x/mouseTruncatesRedoChain

dschuyler

Please try:

make a change in a document (e.g. type 'a')
undo the change (ctrl+z)
move the cursor
make a new change (e.g. type 'b')
problem: IndexError in redoAddChange

dschuyler · 2017-06-29T03:08:12Z

app/text_buffer.py

    if self.debugRedo:
      app.log.info('--- redoIndex', self.redoIndex)
      for i,c in enumerate(self.redoChain):
        app.log.info('%2d:'%i, repr(c))
+      app.log.info('tempChange', repr(self.tempChange))
+
+  def undoDirty(self):


What do you think of naming this undoMove and passing in the tempChange. That would make it more symmetric with redoMove(). It could then be used in the normal undo move, again symmetric with redoMove().

Yes, I think I can do that. Will have to remember to manually change tempChange to None after this call though. I'll fix this soon.

dschuyler · 2017-06-29T03:12:35Z

app/text_buffer.py

        return
-      #app.log.info('opti', change)
-    self.redoChain.append(change)
+      self.redoDirty = True


Is redoDirty True if-and-only-if tempChange is not None?
If so, can we remove the flag?

No. So the reason I have this flag is that when we perform a mouse action, the program is designed so that it calls redoAddChange and then a redo(). I edited redoAddChange so that it'll add the change to tempChange instead and then the next redo will make this change visible. However, if the user then performs a CTRL + Y and performs another redo, we want it to revert the changes caused by tempChange and perform a redo on the regular chain. However, at this time, tempChange still has a change assigned to it, so I needed the flag to determine whether the change is active or not.

Maybe I'll rename it to activeTempChange if it makes it more clear

…oOp is removed

and changed some comments.

Made it so that cursor clicks will not truncate the redo chain

982582a

aaxu mentioned this pull request Jun 26, 2017

Redos not saved #11

Open

aaxu and others added 2 commits June 26, 2017 11:42

Merge branch 'master' of https://github.com/google/ci_edit into bugFi…

3d28172

…x/mouseTruncatesRedoChain

Minor changes to comments.

34d377c

dschuyler requested changes Jun 28, 2017

View reviewed changes

aaxu and others added 5 commits June 28, 2017 11:39

Addressing some reviews and removed dirtyChain flag

de4dcf6

Merge branch 'master' of https://github.com/aaxu/ci_edit into bugFix/…

78e5ccc

…mouseTruncatesRedoChain

Merged with master and new updates

39c717a

Merge branch 'master' of https://github.com/google/ci_edit into bugFi…

cbda788

…x/mouseTruncatesRedoChain

minor comment changes

a3573ff

dschuyler requested changes Jun 29, 2017

View reviewed changes

aaxu and others added 6 commits June 29, 2017 00:22

Fixed some requests to changes. Need to address new bug caused when N…

faa0c3e

…oOp is removed

Fixed merge changes and addressed some reviews

b1d0226

Fixed crashing. Need to test for some intended behaviors

eb7fc7a

Fixed bug

aa7ad36

added return rather than indenting scope of redo

c867123

and changed some comments.

comment changes

e416c6d

dschuyler merged commit eff2c8f into google:master Jun 29, 2017

aaxu deleted the bugFix/mouseTruncatesRedoChain branch June 29, 2017 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made it so that cursor clicks will not truncate the redo chain #10

Made it so that cursor clicks will not truncate the redo chain #10

aaxu commented Jun 26, 2017

dschuyler commented Jun 26, 2017

aaxu commented Jun 26, 2017 •

edited

Loading

dschuyler commented Jun 27, 2017

aaxu commented Jun 27, 2017

dschuyler commented Jun 28, 2017

dschuyler commented Jun 28, 2017

aaxu commented Jun 28, 2017

dschuyler commented Jun 28, 2017

dschuyler Jun 28, 2017

dschuyler Jun 28, 2017

aaxu Jun 28, 2017

dschuyler Jun 28, 2017

aaxu Jun 28, 2017

dschuyler commented Jun 28, 2017

dschuyler left a comment

dschuyler Jun 29, 2017

aaxu Jun 29, 2017

dschuyler Jun 29, 2017

aaxu Jun 29, 2017

aaxu Jun 29, 2017

Made it so that cursor clicks will not truncate the redo chain #10

Made it so that cursor clicks will not truncate the redo chain #10

Conversation

aaxu commented Jun 26, 2017

dschuyler commented Jun 26, 2017

aaxu commented Jun 26, 2017 • edited Loading

dschuyler commented Jun 27, 2017

aaxu commented Jun 27, 2017

dschuyler commented Jun 28, 2017

dschuyler commented Jun 28, 2017

aaxu commented Jun 28, 2017

dschuyler commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuyler commented Jun 28, 2017

dschuyler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaxu commented Jun 26, 2017 •

edited

Loading