Fix for Issue 42

[mihaisucan]

Completed work on fixing Issue 42.

Comments are welcome. Please let me know if any changes are needed. Thanks!

[fjakobs]

Wow this is massive. I don't really like the 'isMeta' flag on the change event. Why is this required? I'd prefer the 'move' function to be on the edit_session to keep the document as simple as possible. This might not be possible though if we can't find a nice solution for the 'isMeta'.

[nightwing]

two small issues

1. dragging don't work after triple click
2. dropping text on itself leaves selection and cursor misaligned

[mihaisucan]

Fabian: the isMeta flag is the least ugly solution in my opinion, at the moment. Problem was that all the change event handlers always assume that the event.data.range property exists, and that the event range refers to an insertion or removal. I do have two ranges in the move action, and I did think of using event.range instead of event.fromRange, but that would've been a "hack", a "trick" to get it work - which I also expect it breaks those event handlers (didn't even test). I could've added an if (action == 'move') return to skip the event, in every such event handler, but then it's too specific. I kind of expect in the future we will want other types of actions to be skipped - hence I decided to use the isMeta flag, to refer to actions that do not directly change the document (like insert/remove). The move action is a meta-action that summarizes the inserts and removals. It seems the undo manager does not support having "meta-actions", "groups of actions". So, this is another reason why I decided to use the new isMeta flag.

In the grand scheme of things, perhaps the solution here is not the ideal one. However, I think it's far beyond the purpose of this bug, of this patch, to make bigger improvements to the undo manager. Maybe you want to file a follow-up issue that specifically asks for improvements to the UndoManager.

nightwing: thanks for testing the patch! I'll fix the issues you found.

[mihaisucan]

nightwing: you do manage to drop text on itself? Please give me a screencast or a list of exact steps to reproduce. I cannot reproduce the bug. I actually have code already in place that does check if you are dropping the selection in the same place, or if you put the drag cursor within the selection - it cancels the drag if you do this, and nothing bad happens: selection stays the same, cursor stops moving.

[mihaisucan]

Pushed a fix for making the selection draggable after triple click. Thanks nightwing!

[nightwing]

this should clear selection and move cusor to dragCursor
otherwise cursor is displayed inside selection instead of boundary

[mihaisucan]

That's on purpose. I believe I prefer this behavior, instead of loosing the selection. The user can use this to ... cancel the drag if he decides against dragging (if he suddenly changes his mind). It's like with clicking on buttons: you hold mouse down.... move out of the button to cancel the click. Here it's the opposite.

If Fabian wants, I can do that - I can clear the selection and move cursor to dragCursor location.

[nightwing]

if mouse released when dragcursor is inside selection blinking cursor continues to remain inside selection
if needed i'll make a screencast this evening when i'll be at home.

[mihaisucan]

Ah, right. That's on purpose. I thought of that and said ... well, perhaps the user wants to cancel the drag operation but not lose the selection. Fabian, what do you think?

[fjakobs]

Hi Mihai,

I have just tested this feature and I really like it. Just some remarks:

1. I think there should be a short delay before it turns into "move" mode. If I select some text and then want to select text where the new selection starts in the old selection I don't expect to trigger a move. I would prefer if it worked like this: I select some text - I click in the selection and hold down the mouse button for a while (e.g. 500ms) - when I move the mouse now It should move the text.

2. Visual feedback would be nice. The easies way would be to set a CSS class on the editor container element and then have a rule like this:

   .ace_dragging .ace_marker-layer,
   .ace_dragging .ace_text-layer {
   cursor: move
   }

3. The problem with the undo manager is that it assumes that all deltas in one undo operation are either deletes or inserts. I have fixed the undo handler to also cope with mixed undos like the ones generated by a move operation. With this being fixed I think the "move" method should be moved into the edit_session.

[adamjimenez]

1. In that case wouldn't you just click outside the selection first? Haven't tried it - but already sounds like what I'd expect.

[fjakobs]

I don't want to break the current selection interaction.

I just checked how apple is doing it. They also use the delay to distinguish between selecting and moving text. If the selected text is not too big it might be cool to attach it semi transparent below the mouse cursor (optional).

http://www.screencast.com/users/fjakobs/folders/Jing/media/20ba9e53-a2a7-4602-8c62-53c0a0c6e5de

[adamjimenez]

there's no delay in dreamweaver (pc). but I suppose there are benefits either way.

[mihaisucan]

1. Let's think of this from the technical perspective as well. If we introduce a delay before it switches to "move" mode, how do we determine in the delay handler if we should or not switch to "move" mode? I presume the answer is "we switch to move mode only if the user did not move the mouse within the allocated 500ms" (or how many ms we choose). But then, I can't help but wonder: if the user waits 500ms with the mouse down and doesn't move the mouse for 500ms and then drags the mouse ... will he expect the selection to be dragged or a new selection to be made? You say about the margins of the selection. How about the middle of the selection? If he waits inside the selection, no matter how long, he still expects the selection to move, not a new one to be drawn. If the user holds the mouse down on the selection margins, again, he will expect the same behavior irrespective of delays. Now, depending on previous experience he might expect the selection to be dragged (since he clicked inside the selection - like I usually expect), or he expects to start a new selection (since he clicked on the margin of the selection - like you usually expect). One of the two users will get a different behavior than the one expected.

How about a technical and behavioral "compromise": if the user clicks on the margins of the selection (left or right) then we don't consider he wants a drag at all. No delays, nothing. We specifically change inSelection to be false when the mousedown event happens at the selection start/end points. Does this sound good for you? At least it does for me as a user and as the implementor, hehe. :)

If you really want the delay-based approach, I can do it. No problem - I don't mind, I am just explaining my reasoning here: I think it's too complex for what we really want.

1. Sure, I'll put cursor:move for .ace_dragging, to indicate that the user is performing a selection move operation.
2. i'll look into the undo manager changes. Can it now handle a "move" action like what I added? Thanks for the changes, btw. ;)

Also, thanks for your feedback!

[fjakobs]

I really want the delay since this is what I think feels most natural. This is how I would implement it:

• two variables control the user experience:
  • the delay (e.g. 500ms)
  • the offset (e.g. 4px)
• on click you store the mouse coordinates and a timestamp
• you have three states: "unknown", "drag" and "select"
• depending on the state onSelectionInterval will do
  • "unknown"
    if the distance between the cursor and the start position is bigger than the offset -> goto state "select"
    if the currentTime - startTime > delay -> goto state "drag"
    else reamin in state "unknown"
  • "drag"
    you behaviour to drag the text
  • "select"
    the current selection behaviour

I'd create a separate function for each state.

We will have to experiment what the best values for "delay" and "offest" are.

I have tested the undo manager changes with your brach and it worked fine even without the meta events.

[mihaisucan]

Oky, thanks for your reply. Will look into implementing that behavior then.

Great to hear you tested the undo manager changes with my branch. Thanks!

[mihaisucan]

So, I tested the undo manager changes with my local rebased branch for issue 42: things work fine. Good work dude! So, the 'move' event is no longer needed.

[mihaisucan]

"unknown" if the distance between the cursor and the start position is bigger than the offset -> goto state "select" if the currentTime - startTime > delay -> goto state "drag" else reamin in state "unknown"

The above is not very clear. The code below is what I understand from the given prose:

if (distance(startCursor, currentCursor) >  offset)
  do_select();
else if ((currentTime - startTime) > delay)
  do_drag();
else return;

What if both distance and delay conditions are satisfied? Or did I miss understood something?

[mihaisucan]

Pushed the code which implements the desired delay. I don't like it much. :D

1. now you really have to want to drag, to drag. you have to hold the mouse down for 500ms then drag.
2. you can't click inside the selection to deselect - this is possible in the master code, and in the initial proposal patch for this issue. click
3. now if you want to quickly drag the selection (by mouse hold inside the selection), where you are 100% sure the editor will pick dragging behavior ... you're still forced to wait for those 500ms - otherwise the selection changes.

Fabian: please test and let me know what changes you want.

I also added cursor: move and now it nicely indicates you're dragging the selection.

[fjakobs]

I agree that the behavior is a little awkward but you were almost there :-). I did some tweaks to your branch and I'm now fairly happy with the result. Basically you were missing the initialization of the drag selection.

https://github.com/ajaxorg/ace/tree/issue-42

If you are also happy with it as well I will merge it to master.

One thing I recognized is that while dragging I should be able to abort the drag by hitting escape. Though this will not hold me back from merging it to master. It is already in a state where I want to have it in master.

[fjakobs]

I have fixed the triple click issue now as well. And pulled the issue-42 branch to master. Let me know if you are happy with the way it works.

[mihaisucan]

Thanks dude! Glad to see this pulled into the main Ace repo.