CTRL-Drag only work when node already moved

[stoecker]

When in select mode, then dragging a node, pressing CTRL and dropping it on another node effectively replaces the original one with the target node.

For no good reason this only works when CTRL is pressed after moving the node started (and I believe also before being to close to the target node). Especially disturbing when this method is used to connect free nodes to ways by using virtual nodes as source.

This should be fixed by allowing to press CTRL also before the source node has been selected and moved.

[akks]

Yes, holding CTRL is rather unpredicatble...

• Nothing selected, drag anything => ignored, but "Move 0 points" command
• Nothing selected, drag virtual node => it works OK and merges nodes
• Nodes selected, drag starting from empty place => deselect nodes in rectangle
• Nodes selected, drag starting from any line on screen (!) => all nodes are moved and merged altogether (!) with node under cursor
• While dragging with CTRL if mouse pass near any virtual node, thar virtual node begins to move instead of starting nodes (very confusing!)

And SelectAction became a real afraid-to-touch monster with 1000-line code, even more terrifying than DrawAction was :)

[stoecker]

Problem is that CTRL has two different modes. First is the deselect mode and second is the drag and join mode.

Hmm, 1 and 5 are clearly bugs, but I could not reproduce point 5.

I think my initial issue could be fixed by allowing drag also when the point was not selected before drag when CTRL is pressed. Essentially like it does for virtual nodes ATM :-)

Point 4 should probably be fixed. Drag and join should allow a single node only. Multiple nodes very likely is an error always. Probably pop-up a warning in this case.

[akks]

I tried to rearrange methods and parameters in SelectAction.java to make it more understandable. The behaviour is still unchanged, I hope. I am not sure if (and when) it should be commited... (I simply could not do anything meaningful with original version).

[stoecker]

Without a diff I cannot really check, but I gave you SVN access as I trust your judgment. So if you think it improves situation - do a checkin. If it breaks then I will rant a bit with you and then we all fix the problems :-)

JOSM-Latest is very stable usually, but I have no problem with some instability from time to time, especially short after a tested release when there is enough time to fix bugs.

[akks]

Diff is erally messy because of fuctions reordering... I tried working with this version and found no differences with original one (checked selecting, rotating, moving, virtual nodes moving and node merging), so I'll commit.

Maybe this version of code will be little bit more coder-friendly :)

[akks]

In 5370/josm:

see #5341: faster nodes moving (by JB) beginUpdate/endUpdate) + big refactoring of SelectAction.java
behavior of Select tool should NOT change (just more structured code), see #7888

[akks]

I placed maximum amount of non-significant changes in single commit. Patch is rather big and hard to read. So anyone can locate problems caused by refactoring only (I hope there will be no such problems).

Detailed description of refactoring:

• rearragnge methouds: group mouse events and reapint-related events
• moved some methods and fields to VirtualManager and CycleManager subclasses
• introduced methods updateCommandWhileDragging, confirmOrUndoMovement, processVirtualNodeMovements to shorten long methods code
• replaced MouseEvent with Point in non-event-handler method parameters when possible - for future snapping and so on
• (most dangerous change) removed few calls to updateKeyModifiers(e) when it was already called from event handler methods (to localize modificator detecting logic)
• applied fix by JB and stoecker from #5341 - introduce beginUpdate/endUpdate while dragging (movement, rotation, scaling) to get rid of multiple NodeChanged events

[Don-vip]

Thanks for these changes :) I agree this class was frightening enough to look somewhere else :D

[akks]

In 5377/josm:

patch by JB: Moving 400 ways takes more than 20 seconds, see #7888 (moving with arrows)

[akks]

(mistake in commit comment, was for ticket #5377)

Now everyone can try to improve the behavior of SelectAction :)

[mdk]

Point 5 (While dragging with CTRL if mouse pass near any virtual node, that virtual node begins to move instead of starting nodes (very confusing!) ) is not easy to reproduce. Just to be shure a "virtual node" is the cross between two "real" nodes?

If so, here is a better way to reproduce:

• select a real node
• move the node over a virtual node
• press CTRL

Result: the selected real node will be dropped and the virtual node is now moved instead.

Because it is enough to be "near" a virtual node when you press CTRL, this happend some time in the heat of editing.

[akks]

Yes, that is how it works. On my Windows XP and Windows 7 it is enough to press CTRL after dragging started, then dragged node is changed every time mouse cursor passes near virtual node (red cross), reproducion is very easy, see the video: ​http://screencast.com/t/GKFJ3KoU

[mdk]

This may be the final clue: I work both on Windows and Ubuntu.

On Windows it is enough to press CTRL anywere, as akks wrote and I reported in #7082 which was closed by joshdoe (not reproducable). Did joshdoe use Windows or Linux?

On Linux the CTRL key must be pressed near the virtual node.

[akks]

So you was the first one who noticed it! #7082 should be reopened then.

[mdk]

An other difference, which can cause the problem, is the used JVM. On Windows I use Oracle JDK, on Ubuntu OpenJDK.

But I think if we solve the problem pressing CTRL near a virtual node in Linux (or OpenJDK), we will also solve the problem in Windows (or Oracje JDK).

[akks]

I am trying to understand all SelectAction code, especially modificator-related on my Windows.

I have some questions, can someone who wrote the code (or know it well) help?

• In cyclePrims method we have following code:

   if (prims.size() > 1) {
         ///...
         if (ctrl) {
            // (1) large block of code
         }
    }
  

​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L900

Block (1) is executed only when user pressed mouse with Alt (elsewhere cycleList is one-element collection), then release Alt and Ctrl is pressed before mouse release (pressing Ctrl-Alt leads to scaling mode).

In most cases of user behavior, block (1) is not called at all.

What is the logic and purpose of such strange action?

• We have two "dangerous" lines in code that change modificator flags:

  alt = alt || Main.pref.getBoolean("selectaction.cycles.multiple.matches", false);
  ...
   shift |= getCurrentDataSet().getSelected().containsAll(prims);
  

​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L845
​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L774

(after next call of updateKeyModfiers alt and shift will be rewritten anyway, but before that happen, changed variable is used)
Is there any purpose or we can just add some temporary variable instead of alt?

[stoecker]

Sounds buggy to me. Maybe we should first collect what this action does and then see whats wrong.

These are the ones I know:

• SHIFT: Allow adding elements to selection.
• CTRL: Allow toggling of elements
• CTRL+Area: Deselect elements
• CTRL+DropOnPoint: Join points
• CTRT+SHIFT: Rotate object
• CTRL+ALT: Scale object
• ALT: Cycle objects under cursor
• ALT+Area: Select full ways partially in the area

[bastiK]

Replying to akks:

I am trying to understand all SelectAction code, especially modificator-related on my Windows.

Some parts of the JOSM code I try not to look at for too long, because it makes my eyes bleed. SelectAction is one of them. But you are trying to improve the situation, so I'll try to help the best I can.

I have some questions, can someone who wrote the code (or know it well) help?

• In cyclePrims method we have following code:

   if (prims.size() > 1) {
         ///...
         if (ctrl) {
            // (1) large block of code
         }
    }
  

​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L900

Block (1) is executed only when user pressed mouse with Alt (elsewhere cycleList is one-element collection), then release Alt and Ctrl is pressed before mouse release (pressing Ctrl-Alt leads to scaling mode).

In most cases of user behavior, block (1) is not called at all.

What is the logic and purpose of such strange action?

I think it is due to the history of the code: The alt cycle behaviour was added by Christian Müller in Sep.-Oct. 2010. Scaling was implemented 2 Months later ([3702]) by Olivier Croquette. Probably the second author didn't bother to rework that part of the code.

• We have two "dangerous" lines in code that change modificator flags:

  alt = alt || Main.pref.getBoolean("selectaction.cycles.multiple.matches", false);
  

Christian suggested, that "false" should be replaced by "true" after a testing period, but it did not happen so far.

 shift |= getCurrentDataSet().getSelected().containsAll(prims);

I would try to understand the purpose of this line and change it to a local variable unless there's a real semantic difference.

[cmuelle8]

Replying to bastiK:

I think it is due to the history of the code: The alt cycle behaviour was added by Christian Müller in Sep.-Oct. 2010. Scaling was implemented 2 Months later ([3702]) by Olivier Croquette. Probably the second author didn't bother to rework that part of the code.

+1

The code block executed when CTRL is pressed is easily understood if you pivot yourself into the user perspective.

• imagine there is a selection consisting of two elements spread across the screen
  • this essentially means they belong to two different cycle groups
  • the following cases arise with a potential next click of the user into the MapView
    • wants to keep the selection (CTRL pressed) ..
      • .. and clicks near a selected primitive (need to cycle there)
        • ​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L930
      • .. and clicks on a selection-free space (need to remember cycle start there)
        • ​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L940
    • wants to deselect the previous selection (CTRL unpressed)
      • ​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L945

So in a condensed form: the code works to give the user the chance modifying your selection of a cycle group (including its deselection), while preserving the selection in areas that are not affiliated with the current cycle group clicked.

alt = alt || Main.pref.getBoolean("selectaction.cycles.multiple.matches", false);

Christian suggested, that "false" should be replaced by "true" after a testing period, but it did not happen so far.

Users that frequently use cycle groups are advised to just set selectaction.cycles.multiple.matches to true in their preferences. This way ALT key is freed for other purposes like scaling etc. I still think it is unobstrusive enough to make it the default behavior.

If it is made default for new installations, users can still opt to continue using old ALT-behavior by adjusting selectaction.cycles.multiple.matches to false,

shift |= getCurrentDataSet().getSelected().containsAll(prims);

Please read the comment at ​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L771

selectPrims is called twice (on mouse down and on mouse release). Historically JOSM starts moving a selection regardless of how many elements it consists if you click on any of the selected elements. So my best guess is, the code artificially presses SHIFT to hold the selection for the drag movement - if none is done, the selected objects are unselected on release. I have not tried but you can check this behavior by selecting a couple of primitives and then

• click on a selected element and drag
• click on an unselected element and drag

[akks]

Thank you very much for your help! Not it is more clear.
If selectaction.cycles.multiple.matches = true, CTRL-block works perfectly. I would be also glad to see this "no-alt-cycling" as default behaviour, it is very logical.

I have removed "Alt-hack" and no bad consequences were noticed, shift need some more analysis :)

[akks]

In 5394/josm:

see #7888, smaller refactoring or SelectAction (cycling) (behavior should not change)

[akks]

I have commited some more refactoring and comment related to Cycle action (we should be able to detect wrong refactoring by this "extra" revision). Preference reading was separeated (init functions), also some loops and if blocks were rearraged for simpler understanding (I hope).

[bastiK]

Replying to akks:

If selectaction.cycles.multiple.matches = true, CTRL-block works perfectly. I would be also glad to see this "no-alt-cycling" as default behaviour, it is very logical.

Then we have 2 people in favour and no one that is against this change. Let's go ahead and wait for user reactions...

[anonymous]

So maybe you can check the code, change default value and commit...

akks. login problem

[anonymous]

So maybe you can check the code, change default value and commit...

akks. login problem

[bastiK]

Replying to anonymous:

So maybe you can check the code, change default value and commit...

akks. login problem

Basically I have no opinion on this (just counting the votes), so I won't commit the change. Please do whatever you think is best!

[akks]

In 5418/josm:

fix #7082 (?) - CTRL-drag in selection mode problem, see #7888: SelectAction and MoveCommand rework

[akks]

To eliminate bug with Ctrl-dragging near virtual nodes and improve performance, I have added skipping events with the same modifiers (windows sends key_pressed, key_pressed, ... key_pressed, key_released when holding CTRL):

​http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/actions/mapmode/SelectAction.java#L214

Serious changes was done in dragging-moving. They are not related to the bug. I have removed all "incremental" moving on MouseDrag and most of dx/dy variables, now at any time SelectAction and MoveCommand knows the starting point of drag-action and coordinates are calculated from initial position every time (not from previous moved coordinates like before).

I hope dragging behaviour did not change and new logic is easier to understand and modify (for example,add "directed moving" parallel to some line or something else later).

Other Ctrl-drag bugs reported here are still pending, their time is near. Almost all of the SelectAction code fits in my head now :)

[mdk]

JOSM 5419: On Linux the problem is not fixed (see comment in #7082)

[akks]

Replying to mdk:

JOSM 5419: On Linux the problem is not fixed (see comment in #7082)

Renamed some methods and tried to fix it by adding !dragInProgress() condition.

[mdk]

Test with '5435 SVN' on Linux: the problem described in #7082 is fixed.

I will try to test this on Windows tomorrow.

[mdk]

Replying to akks:

Serious changes was done in dragging-moving. They are not related to the bug. I have removed all "incremental" moving on MouseDrag and most of dx/dy variables, now at any time SelectAction and MoveCommand knows the starting point of drag-action and coordinates are calculated from initial position every time (not from previous moved coordinates like before).

r5442: This change produce a new problem (or make an old bug more visible): For some operations the initial point must be (re-)set to the current mouse position. To reproduce the case do the following:

1. Grab an object (e.g. a large building). Keep LMB pressed all the time. Icon is a hand (drag mode)
2. Press SHIFT and CTRL (to enter rotation mode. Icons change to rotating arrows.)
3. Rotate the object
4. Release SHIFT and CTRL, but don't move the mouse: Rotation stops and selected object is in drag mode again
5. Move the mouse a little bit: The object "jumps" a delta which includes the mouse movement during rotation.

So after releasing SHIFT and CTRL in step 4., the starting point must be set to the current mouse position.

Perhaps there are other complex workflows which requires a position reset (?)

[akks]

@mdk: Thank you very much for detailed description and locating the problem!
This is fixed, I hope :)

@stoecker
I tried to improve Ctrl-Drag: if nothing was selected, nearest element to the cursor will be dragged (but selection will be performed after dragging starts, not like for rotation and scaling).

[akks]

In 5443/josm:

see #7888: Ctrl-Drag moves the node/way under cursor; fix dragging after rotation; minor refactoring of SelectAction

[mdk]

What is about this ticket? I tested it on Linux with latest JOSM and I can't reproduce the problem. Also the problem never occurred for me for longer time. But I wasn't able to test it on Windows, because I didn't work with this OS any more...

[mdk]

Can we close this issue?

[Don-vip]

I think yes.