[Patch] Deprecate long JosmAction constructors, use setters instead

[simon04]

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

[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.

[michael2402]

ide-before

[michael2402]

ide-after

[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:

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.

[GerdP]

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

[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.

[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.

[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.

[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).

[taylor.smock]

Stupid question on my end: is this a WONTFIX? If so, can we close the ticket as such? (Sorry simon04/michael2402)