Modify

Opened 7 years ago

Last modified 7 years ago

#7317 reopened defect

Cycle list is different for middle-click and Alt+click

Reported by: joshdoe Owned by: team
Priority: normal Milestone:
Component: Core Version: latest
Keywords: cycle select list Cc: cmuelle8

Description

Middle-clicking cycles through all nearby nodes and ways, but Alt+click only cycles through objects of the type already selected. The former is implemented in MapStatus.java, and uses getAllNearest() from NavigatableComponent.java. The latter is implemented in SelectAction.java and uses getNearestNodes() if a node is already selected, or getNearestWays() if a way is already selected.

This inconsistency doesn't make sense to me. I made the trivial modification so Alt+click uses getAllNearest(), and it seems to work as I'd expect.

Any particular reason it was implemented this way?

Attachments (1)

use_getAllNearest.patch (990 bytes) - added by joshdoe 7 years ago.
use all nearest objects when Alt+clicking

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by joshdoe

Attachment: use_getAllNearest.patch added

use all nearest objects when Alt+clicking

comment:1 Changed 7 years ago by joshdoe

Cc: cmuelle8 added
Summary: Cycle list is different for middle-click and Alt+click[patch] Cycle list is different for middle-click and Alt+click

I'm pretty sure because this patch is so trivial I must be missing something important. Maybe cmuelle8 can shed some light since he worked on the cycling code?

comment:2 in reply to:  1 ; Changed 7 years ago by skyper

Replying to joshdoe:

I'm pretty sure because this patch is so trivial I must be missing something important. Maybe cmuelle8 can shed some light since he worked on the cycling code?

Why do we need two different ways for the same action ?

Please, be careful with ALT + Mouse as it is already used by many window-manager in Linux !

comment:3 in reply to:  2 ; Changed 7 years ago by joshdoe

Replying to skyper:

Replying to joshdoe:

I'm pretty sure because this patch is so trivial I must be missing something important. Maybe cmuelle8 can shed some light since he worked on the cycling code?

Why do we need two different ways for the same action ?

Middle-mouse button is not always available or easy to use (smaller laptops, netbooks), and honestly the pop-up just gets in the way most of the time.

Please, be careful with ALT + Mouse as it is already used by many window-manager in Linux !

This behavior has already been implemented for over a year (see #5457) and there are alternatives as I just happened to mention here.

comment:4 in reply to:  3 Changed 7 years ago by cmuelle8

Replying to joshdoe:

This behavior has already been implemented for over a year (see #5457) and there are alternatives as I just happened to mention here.

thx for the clarity in this comment - it's a fairly good summary on primitive cycling..

I've never looked at the code that builds the list when middle click is used, but it should not be so difficult to use same code paths for list building, I guess. imho those lists do not have to be identical from a user perspective. however, if that simplifies the code it wouldn't hurt either.

greetings

comment:5 Changed 7 years ago by akks

By the way, why did not anyone apply such useful patch? Are there any introduced problems?

Last edited 7 years ago by akks (previous) (diff)

comment:6 in reply to:  5 Changed 7 years ago by stoecker

Replying to akks:

By the way, why did not anyone apply such useful patch? Are there any introduced problems?

Nobody had time for a review. Apply it, when you reviewed it.

comment:7 Changed 7 years ago by akks

It is all ok with this patch, it works as desired.

The only question before commiting - why does not we add relations to getAllNearest() (to the end of list)?
Users then can have fast and easy accesible tool to select relations. Can it lead to some ergonomic or performance problems? :)

Last edited 7 years ago by akks (previous) (diff)

comment:8 in reply to:  7 Changed 7 years ago by joshdoe

Replying to akks:

It is all ok with this patch, it works as desired.

The only question before commiting - why does not we add relations to getAllNearest() (to the end of list)?
Users then can have fast and easy accesible tool to select relations. Can it lead to some ergonomic or performance problems? :)

See #7314 which does exactly what you mentiom. I've been using these two patches for the past month and have found them very useful. Thanks for taking the time to look at them.

comment:9 Changed 7 years ago by akks

Oh, I had some feeling of Deja-Vu writing last comment :) Sorry. These shortcut issues are bad for my memory)))

I will apply both patches then.

comment:10 Changed 7 years ago by akks

Resolution: fixed
Status: newclosed

In 5016/josm:

Patch by JoshDoe [Faster relation selection with Alt+click and middle-click], fix #7314, #7317

comment:11 Changed 7 years ago by cmuelle8

Resolution: fixed
Status: closedreopened
Version: latest

The patch in #7314 is fine, I think. From a user perpective #7317 sucks though - it significantly changes user handling.

I did some real live mapping with 5016/josm and it is cumbersome to click through all the objects [rels/ways/nodes] as opposed to the list that is most probable.

E.g. if I want to build a nodes selection, I usually select a node, then use modifier Strg and click on the next node, not necessarily being in the same cycle group, of course. Since 5016/josm this is impossible, I have to cycle through ALL objects near the node starting from the second node I select.

In essence this means, I cannot select e.g. 5 random nodes anymore by simply clicking on them, for each node but the first I have to cycle through ALL objects nearby and 'wait' until the right node shows up, then move on.

This introduced behavior seems dumb, since the editor can (and did) do better - if I click on or really close to a node, the probability is very high, that I indeed want to cycle a node list (if present at all, usually just one node if uncluttered) OR a way list.

The argument that is given to cycleSetup has an important information which is, if the user clicked 'more' on a node or more close to a way that is connected to a node. Depending on this, node or way lists have been cycled in the past. This was very usable, since those lists can grow very long in dense areas and doing a preselection of what list to be build based on the input to cycleSetup() automagically split this to do the right thing (tm).

While I love #7314, I cannot really live with #7317. My suggestion would be the following:

  • build a getAllNearestRelations() method in NavigatableComponent.java separately
  • revert the patch in SelectAction.java and append the output of getAllNearestRelations() to
    • either the node list
    • or the way list
  • .. depending on which one is built in cycleSetup()

Appending only the relevant relations leads to the following possibilities for including relations in the cycle list:

  • cycle nodes + all relations as given by the combined relation list
  • cycle ways + all relations as given by the combined relation list
  • cycle nodes + those nearby relations that have at least one of the nodes on the node list as member
  • cycle ways + those nearby relations that have at least one of the way on the way list as member

Greetings,
Christian

Last edited 7 years ago by cmuelle8 (previous) (diff)

comment:12 in reply to:  11 Changed 7 years ago by joshdoe

Replying to cmuelle8:

While I love #7314, I cannot really live with #7317. My suggestion would be the following:

  • build a getAllNearestRelations() method in NavigatableComponent.java separately
  • revert the patch in SelectAction.java and append the output of getAllNearestRelations() to
    • either the node list
    • or the way list
  • .. depending on which one is built in cycleSetup()

I haven't found this functionality inconvenient, but I can understand in certain use cases it could be frustrating. I think your suggestion is a fine one, though it still means you'll have to cycle through relations when you just want nodes. Perhaps this should be an advanced preference? There could be three levels, only nodes OR ways, (nodes OR ways) AND relations, and then all objects. Question is what the default should be. I'd tend to go with the middle one, which is your suggestion.

comment:13 Changed 7 years ago by skyper

Summary: [patch] Cycle list is different for middle-click and Alt+clickCycle list is different for middle-click and Alt+click

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to joshdoe
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.