Modify

Opened 6 years ago

Closed 6 years 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 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by michael2402, 6 years ago

Description: modified (diff)

comment:2 by stoecker, 6 years ago

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 by Don-vip, 6 years ago

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

comment:4 by michael2402, 6 years ago

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.

by michael2402, 6 years ago

Attachment: fix-16388.patch added

comment:5 by michael2402, 6 years ago

In 14027/josm:

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

in reply to:  5 comment:6 by Don-vip, 6 years ago

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 by Don-vip, 6 years ago

In 14028/josm:

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

comment:8 by Don-vip, 6 years ago

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 by Don-vip, 6 years ago

Owner: changed from team to michael2402

comment:10 by Don-vip, 6 years ago

Milestone: 18.07

comment:11 by michael2402, 6 years ago

I'll have a look at it tomorrow.

comment:12 by michael2402, 6 years ago

In 14029/josm:

See #16388: Checkstyle: Convert tabs to spaces.

comment:13 by michael2402, 6 years ago

In 14030/josm:

See #16388: Fix Checkstyle / Test issues.

comment:14 by michael2402, 6 years ago

In 14031/josm:

See #16388: Fix broken merge.

comment:15 by Don-vip, 6 years ago

Version 0, edited 6 years ago by Don-vip (next)

comment:16 by Don-vip, 6 years ago

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.