Modify

Opened 6 years ago

Closed 5 years ago

#7846 closed enhancement (fixed)

Harmonize Relation popup menus

Reported by: simon04 Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description

Currently, we have (at least) 3 different relation popup menus:

  1. Properties toggle dialog
    • Select relation
    • Select in relation list
    • Select members
    • Download incomplete members
    • ----
    • Go to OSM wiki for tag help
  2. Selection toggle dialog
    • Zoom to selection
    • Zoom to selected elements
    • ----
    • Select in relation list
    • Call relation editor
    • ----
    • Download incomplete members
  3. Relation toggle dialog
    • Download members
    • Download incomplete members
    • ----
    • Select members
    • Select members (add)
    • Select relation
    • Select relation (add)
    • ----
    • Add selection to relation

For a more intuitive UI, I am strongly in favour of a single, harmonized popup menu for all 3 toggle dialogs. To achieve this, we may need to disable (grey out) some entries for some dialogs.

Furthermore, the double click behaviour differs as well:

  • Properties toggle dialog: open relation editor
  • Selection/Relation toggle dialog: select

Attachments (1)

7846_refactor.patch (38.7 KB) - added by Don-vip 5 years ago.
Refactorization needed for xml-imagery-bounds and tag2link plugins

Download all attachments as: .zip

Change History (33)

comment:1 Changed 6 years ago by simon04

Btw, the corresponding parts in the source code are (r5324):

  • SelectionListDialog L210
  • PropertiesDialog L721
  • RelationListDialog L846

comment:2 Changed 5 years ago by akks

I have started to prepare this change.
Many action were copy-pasted or very similar in these dialogs.
I tried to decouple these actions from toggle dialogs and place them into separate package, reducing number of list selection listeners.

comment:3 Changed 5 years ago by akks

In 5793/josm:

Big refactoring of relation context menu actions, removing duplicate and similar code. Any behavior change is a bug!
see #7846: Harmonize Relation popup menus and actions,
Please check relation-actions in Selection List, Relation List and Properties dialogs.

comment:4 Changed 5 years ago by akks

Please fix the commit or post here in case of any related issues or revert it (if the chosen class structure is bad enough).

comment:5 Changed 5 years ago by Don-vip

In 5794/josm:

see #7846 - fix copyright + javadoc + potential NPEs

comment:6 Changed 5 years ago by Don-vip

The chosen class structure is very good. Thanks for this code refactorization :) I have applied some code cleanup in r5794 :)

comment:7 Changed 5 years ago by akks

In 5799/josm:

Membership tabled in properties toggle dialog supports multiselect (and multiple membership deletion)
Property toggle dialog refactoring - methods splitting and reordering
see #7846: more RelationListDialog refactoring, all other relation-related actions separated from dialogs, @Override, JavaDocs

comment:8 in reply to:  6 Changed 5 years ago by akks

Replying to Don-vip:

The chosen class structure is very good. Thanks for this code refactorization :) I have applied some code cleanup in r5794 :)

Thank you :) There are some more changes to check.
In PropertiesDialog only real code change for now is adding multiselect behavior to membership table.

I am also planning to add multi-copy/paste in propertiesTable (for pasting with Ctrl-Shift-V) later.

Modification of context menus and double-click should be now possible and can be discussed.

P.S. That is all for now from me because of night time and possible clean-up commit with number 5800 :-)

comment:9 Changed 5 years ago by akks

In 5800/josm:

Properties toggle dialog: allow to select and copy/delete multiple keys/values, see #7846
fix #6917, fix #7895 - select multiple "member of" (fixed one non-working action in previous revision)

comment:10 Changed 5 years ago by Don-vip

r5793 broke imagery-xml-bounds plugin, I'm working on a fix.

comment:11 in reply to:  10 Changed 5 years ago by akks

Replying to Don-vip:

r5793 broke imagery-xml-bounds plugin, I'm working on a fix.

I am sorry. I have looked at plugin code before committing, noticed calls like relationListDialog.addPopupMenuAction(relationListAction) but did not notice that many of these actions implement RelationRelated and SelectionChangeListener...

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

comment:12 Changed 5 years ago by akks

To fix ShowBoundsListAction (ListSelectionListener method are not called), most likely we should return ListPopupMenu into RelationListDialog in core.

Before r5793:
957 class RelationDialogPopupMenu extends ListPopupMenu {
958
959 public RelationDialogPopupMenu(JList list) {
960 super(list);
961
Since r5793:

783 class RelationDialogPopupMenu extends JPopupMenu {

and valueChanged() of additional actions is not called automatically...

Replacing RelationRelated action with AbstractRelationAction is simple, but it is not called automatically - maybe we need finder of AbstractRelationAction items in PropertiesDialog instead of long list.
Now: http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java#L396
Was with RelationRelated search: http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java?rev=5789#L398

I am not touching the code, maybe you'll find more bugs...

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

comment:13 Changed 5 years ago by Don-vip

I have started a significant refactorization, I'll attach a patch for review, probably tonight.

comment:14 Changed 5 years ago by akks

OK.

Changed 5 years ago by Don-vip

Attachment: 7846_refactor.patch added

Refactorization needed for xml-imagery-bounds and tag2link plugins

comment:15 Changed 5 years ago by Don-vip

Summary: Harmonize Relation popup menus[Patch] Harmonize Relation popup menus

I plan to merge this patch tomorrow :)

comment:16 Changed 5 years ago by akks

Wow, the unified context menus!
I have looked at the code (had no time to apply). It looks clear, better than mine :)

It should work as needed, but our NodeAction, WayAction and RelationAction are little bit hard to reuse in other parts of JOSM. If we want to make it universal, maybe all action could be merged together ( OsmPrimitive? )...

There are also a lot of action-creating methods in DataActionPopupMenuHandler. Maybe this can be done automatically too (some arrays maybe..)?
The base class DataActionPopupMenuHandler can be reused too then.

There are more actions left in dialogs that we did not touch because they are not relation-related but node-way-or "primitive-list"-related.

However, these ideas may be non-realizable.

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

comment:17 in reply to:  16 ; Changed 5 years ago by Don-vip

Replying to akks:

It should work as needed, but our NodeAction, WayAction and RelationAction are little bit hard to reuse in other parts of JOSM. If we want to make it universal, maybe all action could be merged together ( OsmPrimitive? )...

There are more actions left in dialogs that we did not touch because they are not relation-related but node-way-or "primitive-list"-related.

I have maybe an idea to implement a better approach (keep only an OsmPrimitiveAction interface) and move collection filtering from caller to specific actions. I'll try when I will have some time to do it.

comment:18 in reply to:  17 Changed 5 years ago by akks

Replying to Don-vip:

I have maybe an idea to implement a better approach (keep only an OsmPrimitiveAction interface) and move collection filtering from caller to specific actions. I'll try when I will have some time to do it.

Yes, I thought about it too. Main difficulty is that we need the updating of the primitive list to be fast (just copy the recference). Filtering or even checking from every action when selection is changed can slow down the process and increase CPU load... When the action is performed, it is OK.

comment:19 Changed 5 years ago by stoecker

imagery-xml-bounds still does not compile.

comment:20 Changed 5 years ago by akks

@Don-vip: Have you started to edit the code? If no, I can edit the patch (joining different actions to OsmPrimitiveAction). Then you could check&fix it and apply.

I guess we need to fix plugins before tested release... If the time is short, we can apply it as is and rework later.

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

comment:21 Changed 5 years ago by Don-vip

Sorry, I should have told you I was not available this week-end. I think I may be able to finish and commit this patch tonight, before the nightly rolls out :)

comment:22 in reply to:  21 Changed 5 years ago by akks

Replying to Don-vip:

Sorry, I should have told you I was not available this week-end. I think I may be able to finish and commit this patch tonight, before the nightly rolls out :)

I guessed that it was Easter holiday in Europe :)
OK, I'll check the committed code.

comment:23 Changed 5 years ago by Don-vip

In 5821/josm:

see #7846 - Large code refactorization in management of popup menus to simplify interactions with plugins (needed at least for imagery-xml-bounds and tag2link plugins)

comment:24 Changed 5 years ago by Don-vip

Plugins fixed in [o29461].

comment:25 Changed 5 years ago by akks

I have checked the code. It looks and works correctly. Some minor optimizations and universalizations (moving Zoom and Select actions etc.) may be done after Tested is released.

comment:26 Changed 5 years ago by akks

In 5825/josm:

fix #8574: relation list dialog side buttons din not work
Сontext menu unification (see #7846): relation list and membership table of properties dialog

comment:27 Changed 5 years ago by akks

I have made the context menus in two dialogs look the same. I hope it does not break anything, but please check...
pre-latest build: https://dl.dropbox.com/u/63393258/josm-custom.jar

By the way, why do not we have auto-generated very-latest-josm.jar rebuilt after each revision for developers?

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

comment:28 Changed 5 years ago by akks

Summary: [Patch] Harmonize Relation popup menusHarmonize Relation popup menus

comment:29 Changed 5 years ago by Don-vip

Developers can update their SVN copy :)

comment:30 Changed 5 years ago by Don-vip

Can we close this ticket ?

comment:31 Changed 5 years ago by akks

If Selection toggle dialog context menu should not be changed, then the ticket can be closed.

comment:32 Changed 5 years ago by akks

Resolution: fixed
Status: newclosed

I guess it can be closed :)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.