Modify

Opened 6 months ago

Closed 5 months ago

#16388 closed defect (fixed)

RelationEditor#registerRelationEditor not used?

Reported by: michael2402 Owned by: michael2402
Priority: normal Milestone: 18.07
Component: Core Version:
Keywords: Cc: stoecker, Biswesh

Description (last modified by michael2402)

When trying to register an extended relation editor for the PT assistant plugin, we noticed that the methods to register new relation editors seem not to be functional.

There are several issues with it:

  • The registerRelationEditor is not static - it probably should be
  • The type of the parameter should be Class<? extends RelationEditor>. Otherwise, you can only register exact relation editor instances.
  • We are not checking the full constructor signature of that class, we cannot do this since the second argument is contains a generic type info.

Is it used? If not, we should drop it and replace it by a more robust registration mechanism that does not rely on unchecked reflective accesses and uses a factory interface instead.

Alternative: Prevent the relation editor from being replaced and only allow hooks to hook into it's lifecycle so that e.g. buttons may be added by plugins.

@stoecker: You seem to have added the code, what was it used for?

Attachments (1)

fix-16388.patch (121.5 KB) - added by michael2402 6 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 months ago by michael2402

Description: modified (diff)

comment:2 Changed 6 months ago by stoecker

That is not from me. You need to check history better. Code was introduced by Gubaer in r1828.
changeset/1828/josm/trunk/src/org/openstreetmap/josm/gui/dialogs/relation/RelationEditor.java

Very likely the idea was to have a interface to extent the editor (i.e. adding tabs with more specific editors as far as I remember). Probably should have been used by turnrestrictions plugin, but then another way was implemented?

Updates/Improvements welcome.

comment:3 Changed 6 months ago by Don-vip

I don't find any reference in all OSM SVN plugins. So it seems to be safe.

comment:4 Changed 6 months ago by michael2402

While trying to find a nice way to fix this and allow plugins to add their own buttons, I changed the way parameters are passed to the editor actions.

Until now, all models/tabels/... were always passed as parameters explicitly and when adding the acitons to the toolbar, you needed to know what listeners to set on which models and which models to pass on.

I changed it and added a universal interface IRelationEditorActionAccess that provides access to all things the actions might be using.

Actions are now grouped into IRelationEditorActionGroups. Plugins can add their groups to the toolbars using the RelationEditorHooks class.

Changed 6 months ago by michael2402

Attachment: fix-16388.patch added

comment:5 Changed 5 months ago by michael2402

In 14027/josm:

See #16388: New mechanism for plugins to register relation editor actions.

comment:6 in reply to:  5 Changed 5 months ago by Don-vip

Replying to michael2402:

In 14027/josm:

See #16388: New mechanism for plugins to register relation editor actions.

All builds are failing now, can you please fix them?

comment:7 Changed 5 months ago by Don-vip

In 14028/josm:

see #16388 - fix compilation errors in unit tests caused by r14027 (broke all Jenkins builds)

comment:8 Changed 5 months ago by Don-vip

r14027 introduced a lot of warnings, can you please fix them?

  • 2 FindBugs warnings
  • 49 Checkstyle warnings
  • 1 PMD warning
  • 87 Javadoc warnings

comment:9 Changed 5 months ago by Don-vip

Owner: changed from team to michael2402

comment:10 Changed 5 months ago by Don-vip

Milestone: 18.07

comment:11 Changed 5 months ago by michael2402

I'll have a look at it tomorrow.

comment:12 Changed 5 months ago by michael2402

In 14029/josm:

See #16388: Checkstyle: Convert tabs to spaces.

comment:13 Changed 5 months ago by michael2402

In 14030/josm:

See #16388: Fix Checkstyle / Test issues.

comment:14 Changed 5 months ago by michael2402

In 14031/josm:

See #16388: Fix broken merge.

comment:15 Changed 5 months ago by Don-vip

Last edited 5 months ago by Don-vip (previous) (diff)

comment:16 Changed 5 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 14056/josm:

fix #16388 - fix sonar warning

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain michael2402.
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.