Modify

Opened 8 months ago

Last modified 5 weeks ago

#19296 assigned task

Actions should avoid to install listeners which are not needed

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone: Longterm
Component: Core Version:
Keywords: Cc:

Description

A lot of actions use a constructor which calls JosmAction.installAdapters() by default. Some of these actions don't care about layer changes or selection changes and thus should not install the listeners.

Attachments (1)

19296.patch (5.1 KB) - added by GerdP 8 months ago.
work in progress patch, probably more actions can be changed

Download all attachments as: .zip

Change History (17)

Changed 8 months ago by GerdP

Attachment: 19296.patch added

work in progress patch, probably more actions can be changed

comment:1 Changed 8 months ago by GerdP

Owner: changed from team to GerdP
Status: newassigned

comment:2 Changed 8 months ago by stoecker

I don't think you need to provide such patches for review. It seems obvious. Simply fix it as you find it.

comment:3 Changed 8 months ago by GerdP

In 16505/josm:

see #19296: Actions should avoid to install listeners which are not needed
tbc

comment:4 Changed 8 months ago by GerdP

We have a lot of actions which only need the ActiveLayerChangeAdapter but not the SelectionChangeAdapter.
IIGTR actions should overwrite method listenToSelectionChange() to return false when they only need the ActiveLayerChangeAdapter?

Last edited 8 months ago by GerdP (previous) (diff)

comment:5 Changed 8 months ago by GerdP

In 16509/josm:

see #19296: Actions should avoid to install listeners which are not needed

  • either don't call installAdapters() or overwrite listenToSelectionChange()
  • partly reverts previous changes so that installAdapters() is not overwritten

One has to be careful because installAdapters() also calls initEnabledState()

comment:6 in reply to:  4 Changed 8 months ago by stoecker

Replying to GerdP:

We have a lot of actions which only need the ActiveLayerChangeAdapter but not the SelectionChangeAdapter.
IIGTR actions should overwrite method listenToSelectionChange() to return false when they only need the ActiveLayerChangeAdapter?

That sounds like a lot of code. I'd assume some way telling this via the constructor would be better, like changing bool to an enum which can transport multiple states?

comment:7 Changed 8 months ago by stoecker

Looking at the patch it's not so much code :-) Maybe that's ok...

comment:8 Changed 8 months ago by GerdP

And it's difficult to change something because some plugins use the existing methods. I've added some more in [o35472:35475]

comment:9 Changed 8 months ago by simon04

Milestone: 20.05

When starting JOSM, I get the following messages:

2020-05-29 20:14:47.470 INFO: Toolbar action without name: org.openstreetmap.josm.actions.ChangesetManagerToggleAction
2020-05-29 20:14:47.804 INFO: Toolbar action without name: org.openstreetmap.josm.actions.ChangesetManagerToggleAction

comment:10 Changed 8 months ago by GerdP

In 16522/josm:

see #19296: fix Toolbar action without name: org.openstreetmap.josm.actions.ChangesetManagerToggleAction
regression of r16509

comment:11 Changed 8 months ago by GerdP

@Simon: Thanks for the hint, didn't see that difference in the two constructors.

comment:12 Changed 8 months ago by simon04

@GerdP, is anything left to be done?

comment:13 Changed 8 months ago by GerdP

I thought of this as a task. I've changed almost all core actions, but only a few plugins.
I kept Undo/Redo action as is because they might be needed for #17196.

comment:14 Changed 8 months ago by GerdP

Milestone: 20.05Longterm
Type: enhancementtask

comment:15 Changed 5 weeks ago by GerdP

In 17414/josm:

see #19296: Actions should avoid to install listeners which are not needed

  • both Note actions don't use SelectionChangeListener or LayerChangeListener

comment:16 Changed 5 weeks ago by GerdP

In 17419/josm:

see #19296: Actions should avoid to install listeners which are not needed

  • DownloadAlongAction doesn't use the listeners but is created for each right click on a GPX layer, stores a ref to GpxData and blocks GC

Modify Ticket

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