Question about the history code

[ajpalkovic]

I'm not really sure where to ask this, but...

What is the purpose of the timeout code in lib/ruco/history.rb ?

For reference, these are the two methods I'm talking about:

def timeout!
  @last_merge = Time.now.to_f - @timeout
end

def merge_timeout?
  (Time.now.to_f - @last_merge) > @timeout
end

If I call those two methods almost immediately, like

...
history.timeout!
history.merge_timeout?
...

It will effectively execute this:

Time.now.to_f - (Time.now.to_f - @timeout) > @timeout

That is almost the same as writing:

Time.now.to_f - Time.now.to_f + @timeout > @timeout

This will only fail if Time.now.to_f is exactly equal in both cases, regardless of the value of timeout. Also, whether timeout is 2 or 0 or 100,000, if the second call to Time.now.to_f (which is actually executed first, so its really the first call, but just appears second), is even slightly less than the other call, then this will return true.

My guess is timeout! should just be:

@last_merge = Time.now.to_f

Then in merge_timeout, it checks if 2 seconds or whatever has elapsed.

I'm pretty sure this is why some of the specs are failing on Windows, but I have no clue why they don't also fail on Linux.

[ajpalkovic]

I dunno if I explained that well enough. The point is that the timeout code doesn't actually use the @timeout variable. So, that is probably a bug. Timeout can be set to 0 or 100,000, and the code will do the same thing.

[grosser]

the idea is that timeout! should mark it as timeouted, so @last_merge = 0 would be a simpler implementation that also gets rid of the edge-case you found. It uses the timeout variable to tell when a normal timeout occurs. e.g. user typing for to long. Ill add this simplified version (tests were still green)

[ajpalkovic]

Hmm, I don't think that's the right solution.

A call to each function would effectively look something like this:

Time.now.to_f - (Time.now.to_f - @timeout) > @timeout

If Time.now.to_f is the same, then we get something like this:

100 - (100 - 2) > 2  ->  100-98 > 2  ->  2 > 2  ->  false

But, if we change the timeout, we get the same answer:

100 - (100 - 0) > 0  ->  100-100 > 0  ->  0 > 0  ->  false

Now, if the two times are slightly different, say 100 and 100.1, we get:

100.1 - (100 -2) > 2  ->  2.1 > 2  ->  true

I think the point of what you wrote is that if the user types for half a second, then rather than create a new undo state for each keystroke, it will create one undo for the whole text that was entered for 0.5 seconds. However, if they type for 2 seconds or more, it will create one undo state for the first 2 seconds, and another for the rest.

But, I'm not sure that's what this does. I think what happens is if Time.now.to_f is the same, then it will merge the two, if it is not the same, then it creates two states.

On the rspecs, one of the tests is failing because inserting the letter a is merged with the initial state of the editor, instead of being inserted as a new state.

If you set @last_merge = 0 in timeout!, then I think it would never merge two states. The add method would always increment @position.

I would change the history file to this: https://gist.github.com/963423 I think if you clone it you can see the diffs.

This might behave the same as yours, but it passes more of the specs on my machine. The differences are:

1. anytime you undo/redo, the next set of changes are always added to a new state. You should never modify an existing undo/redo state if that makes sense.
2. Never modify the initial state of the file so it can always be undone. One of the failing specs on my machine prevented that.
3. timeout! should only be called when we want to start inserting a new state, ie, if we insert a character, but we have not inserted a character for 10 seconds, then timeout! is called to mark the start of this state.

I think this behaves better. But, I think you might want to consider a different undo system.

I've worked on e, which is like textmate, but for Windows. And, I think e has a neat undo system. Essentially, e defines two different types of undo actions. You can insert characters or you can delete characters. So, if I insert a, it creates a state, if I insert b, it then merges, so there is still only one state. If I delete a character, it creates a new state. So, the undo tree in e is essentially alertnating with insertions and deletions. You never have two 'insertion' states in a row in e. That might make more sense than a timeout system. The timeout does work, but I feel like in the real world, it might produce 'weird' behavior? Idk, I'm open to whatever. A combination of the two might make sense as well. Whenever you switch between insertion/deletion a new state is added, and after two seconds of being in the same state, a new state is added, if that makes sense?

[grosser]

when using def timeout! @last_merge = 0 end

the code would be Time.now - 0 > 100, which would always be true -> it would always create a new state after a timeout.

The separation of insert and delete sounds good, i think the timeout rule is important and already provides most of the usability since most of the time you type something, think about it(long) and then type/delete again, i almost never end up in a state where half a word was typed when I undo.

[ajpalkovic]

I tried what you suggessted, but that did not fix any of the specs for me. I changed the history code in this commit: https://github.com/ajpalkovic/ruco/commit/8916b144f0be51335dfb65086a5a63f649d10589 . I think there are times when we want to set the @last_merge to 0 in order to force any future changes to create a new state in the undo stack, but there are also times when we want to 'merge' one set of changes with the top of the undo stack, which is when we set @last_merge to Time.now. Idk, with these changes it passes all the specs on windows and linux for me now.