Modify

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21730 closed defect (fixed)

[patch] Not all JosmActions registering in toolbar

Reported by: gaben Owned by: GerdP
Priority: normal Milestone:
Component: Core Version: tested
Keywords: regression toolbar plugin Cc: GerdP

Description (last modified by gaben)

Changes made in r18321 blocks some core and custom actions, possibly added by a plugin (MainApplication.getToolbar().addCustomButton()) from registering into the toolbar settings.

See the screenshot

Attachments (3)

JOSM_toolbar_actions.png (33.8 KB ) - added by gaben 2 years ago.
21730.patch (2.8 KB ) - added by GerdP 2 years ago.
21730-2.patch (3.7 KB ) - added by GerdP 2 years ago.

Download all attachments as: .zip

Change History (24)

by gaben, 2 years ago

Attachment: JOSM_toolbar_actions.png added

comment:1 by gaben, 2 years ago

Summary: Not all JosmActions not registering in toolbarNot all JosmActions registering in toolbar

comment:2 by GerdP, 2 years ago

Cannot reproduce this with r18303, tried my normal config and also a clean one.

comment:3 by gaben, 2 years ago

Until r18320 is fine, the problem affects r18321 and later revisions.

comment:4 by GerdP, 2 years ago

Ah, yes, sorry, got the version numbers wrong.

comment:5 by GerdP, 2 years ago

Hmm, I don't understand why e.g. "Orthogonalize Shape / Undo" no longer appears and I think it was never meant to appear at that place. Instead it should appear somewehere under Tools.

comment:6 by gaben, 2 years ago

I have no idea either. Also don't know why the phone action (provided by my plugin) forego the "move up" action. I think the move actions supposed to follow each other as they created at the same time(?).

The whole thing affects me in a way, that I can't use the JOSM core toolbar settings. So either I need to code it myself or have to choose fixing the button into the toolbar. Mostly inconvenience, but bothering.

comment:7 by skyper, 2 years ago

See also #6725 and #14541.

comment:8 by skyper, 2 years ago

Keywords: toolbar added
Version: tested

comment:9 by gaben, 2 years ago

Description: modified (diff)
Keywords: plugin added

comment:10 by GerdP, 2 years ago

Last edited 2 years ago by GerdP (previous) (diff)

comment:11 by GerdP, 2 years ago

Owner: changed from team to GerdP
Status: newassigned

by GerdP, 2 years ago

Attachment: 21730.patch added

comment:12 by GerdP, 2 years ago

@gaben: Problem was this part of the code:

        for (Map.Entry<String, Action> a : regactions.entrySet()) {
            if (actions.get(a.getKey()) == null) {
                rootActionsNode.add(new DefaultMutableTreeNode(a.getValue()));
            }
        }

With my changes the actions.get(a.getKey()) == null was never true and thus only actions which appear in a menu were added. I've renamed actions to actionsInMenu to make that clearer and now the map is cleared first in loadActions().

Last edited 2 years ago by GerdP (previous) (diff)

comment:13 by GerdP, 2 years ago

Have to think again about this. The new patch probably re-introduces the memory leak...

by GerdP, 2 years ago

Attachment: 21730-2.patch added

comment:14 by GerdP, 2 years ago

2nd patch should also fix the memory leak as the map itself should be GCed now.
There's probably no need to use a ConcurrentHashMap, but my understanding of that is small.

comment:15 by gaben, 2 years ago

[patch v2]

  • Something strange going on. Now I got the cycle layer actions in the toolbar settings. That's fine, but where were they until now? They are missing completely in previous versions too. They seem to appear based on layer availability, interesting.
  • The actions class variable was moved, but javadoc remains and now it is javadoc of regactions variable. Is this intended?
  • unrelated, but the public getToolString() javadoc is missing

I can't comment on other changes, as I don't understand much of the GUI code :(

Last edited 2 years ago by gaben (previous) (diff)

comment:16 by GerdP, 2 years ago

The javadoc is OK for regactions, too.
Does the patch solve your plugin problem?

comment:17 by gaben, 2 years ago

Yes, thank you for the quick fix!

comment:18 by GerdP, 2 years ago

OK, I think I'll wait a few days before committing this. Or do you think it is worth a hotfix?

comment:19 by gaben, 2 years ago

Ah, you are kind. I'm fine with waiting, as the plugin is not released yet.

It will be my first public code repository, and I'm not sure about licensing stuff and also accidentally used my private email in commits, so I need to rewrite the whole repo history before making it public... Otherwise the plugin is usable. ​See #15250 for request.

comment:20 by gaben, 2 years ago

Summary: Not all JosmActions registering in toolbar[patch] Not all JosmActions registering in toolbar

comment:21 by GerdP, 2 years ago

Resolution: fixed
Status: assignedclosed

In 18362/josm:

fix #21730: Not all JosmActions registering in toolbar

  • separate again between all registered actions and those which appear in menus (regression from r18321)
  • replace global map actions by local one to avoid memory leak

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
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.