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)
Change History (17)
Changed 8 months ago by
Attachment: | 19296.patch added |
---|
comment:1 Changed 8 months ago by
Owner: | changed from team to GerdP |
---|---|
Status: | new → assigned |
comment:2 Changed 8 months ago by
I don't think you need to provide such patches for review. It seems obvious. Simply fix it as you find it.
comment:4 follow-up: 6 Changed 8 months ago by
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
?
comment:6 Changed 8 months ago by
Replying to GerdP:
We have a lot of actions which only need the
ActiveLayerChangeAdapter
but not theSelectionChangeAdapter
.
IIGTR actions should overwrite methodlistenToSelectionChange()
to return false when they only need theActiveLayerChangeAdapter
?
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:8 Changed 8 months ago by
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
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:11 Changed 8 months ago by
@Simon: Thanks for the hint, didn't see that difference in the two constructors.
comment:13 Changed 8 months ago by
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
Milestone: | 20.05 → Longterm |
---|---|
Type: | enhancement → task |
work in progress patch, probably more actions can be changed