Modify

Opened 9 months ago

Last modified 3 months ago

#19327 assigned enhancement

[Patch] Deprecate long JosmAction constructors, use setters instead

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

Description

The various constructors with many parameters are hard to use and distinguish. Let's favour a setter-based style instead.

Attachments (3)

19327.patch (171.3 KB) - added by simon04 9 months ago.
Bildschirmfoto von 2020-12-12 23-58-55.png (11.8 KB) - added by michael2402 3 months ago.
ide-before
Bildschirmfoto von 2020-12-13 00-02-22.png (11.4 KB) - added by michael2402 3 months ago.
ide-after

Download all attachments as: .zip

Change History (11)

Changed 9 months ago by simon04

Attachment: 19327.patch added

comment:1 Changed 8 months ago by stoecker

Actually I find that a lot less readable. Much more code where easily some of the components may be forgotten. And, depending on the optimizer, it is also a bit slower when executed.

I'm against this change.

comment:2 Changed 8 months ago by simon04

Milestone: 20.06

Changed 3 months ago by michael2402

ide-before

Changed 3 months ago by michael2402

ide-after

comment:3 Changed 3 months ago by michael2402

That saves so much nulls when creating acitons and makes them much more readable, especially in code reviews where you don't have IDE support (my GSoC experience again …)

And in the IDE, I still find it more readable since it forces devs to use a new line for each property and removes all those unused properties.

Before:
ide-before

After:
ide-after

Just a short review (if you have the patch somewhere on github, I can send you the notes there, should be easier to find the lines):

    protected final void setIcon(String iconName) {
Should not be final (you may never know …)
Should call setIcon((IconProvider) null)

    protected final void setIcon(ImageProvider icon) {
Sould reset the icon if called with icon==null or if resource not found

And we should get rid of the true/false constructor, too.

setHelpId() is inconsistent wit the no-args constructor there. And you can call installAdapters() yourself instead of using the constructor parameter.

Last edited 3 months ago by michael2402 (previous) (diff)

comment:4 Changed 3 months ago by GerdP

Well, in most cases you don't want to pass null arguments. See also #19296.

comment:5 in reply to:  4 Changed 3 months ago by michael2402

Replying to GerdP:

Well, in most cases you don't want to pass null arguments. See also #19296.

For Icons? Well, you want to replace them sometimes without creating a new action object.

And as for plugins: The JosmAction class has some neat features, like setting an Icon for JOSM. But in that case, I do not need many more features. Especially the boolean tags are very confusing.

comment:6 Changed 3 months ago by GerdP

Well, I think we should at least deprecate the constructor that you used in your example as it installs the listeners and callers might not be aware of that even though the javadoc mentions it.

comment:7 Changed 3 months ago by stoecker

I'm totally against changing one of the central APIs of JOSM without any functional improvements only to make the code follow a personal programming style opinion. We'd rework major code parts and invalidate nearly all plugins for no real gain.

If it does not improve the functionality (e.g. comment:6) that's a WONTFIX.

comment:8 in reply to:  7 Changed 3 months ago by michael2402

Replying to stoecker:

If it does not improve the functionality (e.g. comment:6) that's a WONTFIX.

But this is just the reason why fixing this is important: That action is used wrong in many Plugins I see. Constructors are mixed depending on author preference and without knowing what each constructor does.

We can keep the old API for some time (the change won't break anything there, except for the normal default constructor, which would then not register listeners - but in the cases I saw, those listeners were not used).

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain simon04.
as The resolution will be set.
to The owner will be changed from simon04 to the specified user.
The owner will change to simon04
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.