Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#6953 closed defect (fixed)

[additional PATCH] Allow hiding buttons in left toolbar

Reported by: Zverikk Owned by: team
Priority: normal Milestone:
Component: Core Version: latest
Keywords: toolbar Cc:

Description

Now it is possible to hide some panel buttons, but not instruments. An experienced mapper almost always select tools not by clicking a button, but by using shortcuts, so in order to free more screen space it'd be nice to have option of hiding rarely used buttons.

Attachments (9)

toolbarContextMenu.patch (2.5 KB) - added by akks 8 years ago.
toolbarContextMenu2.patch (5.0 KB) - added by akks 8 years ago.
MapModeToolbarCustomization.patch (4.9 KB) - added by akks 8 years ago.
toolbarMapModeShowHide.patch (5.7 KB) - added by akks 8 years ago.
bigShowHideButton.patch (16.0 KB) - added by akks 8 years ago.
SidetoolbarPreferences.patch (1.4 KB) - added by akks 8 years ago.
SidetoolbarLayerChange.patch (2.7 KB) - added by akks 8 years ago.
PluginRoutingPatch.patch (6.1 KB) - added by akks 8 years ago.
SidetoolbarLayerChange2.patch (3.9 KB) - added by akks 8 years ago.

Download all attachments as: .zip

Change History (62)

Changed 8 years ago by akks

Attachment: toolbarContextMenu.patch added

comment:1 Changed 8 years ago by akks

I have made a patch to add toolbar context menu ("remove button" and "show preferences" for now).

There is unsolved problem - I do not know, how to show specific tab of preferences, not the first [without big refactoring]. There is no list of preference tabs, tabs even does not have titles to use for search~~

Please have a look at it if you have some time.

Last edited 8 years ago by akks (previous) (diff)

comment:2 Changed 8 years ago by akks

Summary: Allow hiding tool buttons from the edit toolbar[PATCH] Allow hiding tool buttons from the edit toolbar

comment:3 Changed 8 years ago by akks

Next version of the patch also includes methods for selecting specific preferences tab (small additions to PreferenceDialog and PreferenceTabbedPane). Such method is used to open toolbar preferences directly from context menu.

Last edited 8 years ago by akks (previous) (diff)

Changed 8 years ago by akks

Attachment: toolbarContextMenu2.patch added

comment:4 Changed 8 years ago by stoecker

Resolution: fixed
Status: newclosed

In [4586/josm]:

fix #6953 - patch by akks - Allow hiding tool buttons from the edit toolbar

comment:5 Changed 8 years ago by Zverikk

Resolution: fixed
Status: closedreopened

Thanks, but edit toolbar is the one to the left, not the top one.

comment:6 Changed 8 years ago by stoecker

Status: reopenednew
Summary: [PATCH] Allow hiding tool buttons from the edit toolbarAllow hiding buttons in left toolbar

comment:7 Changed 8 years ago by akks

Lower buttons (Tool windows) already can be hidden from its context menu. Only instruments remain and it is harder to add "hide", beause it does not have toobar editor!

Maybe we should simply make left toolbar floatable?

comment:8 Changed 8 years ago by stoecker

What do you mean with "floatable"?

comment:9 Changed 8 years ago by akks

toolBarActions.setFloatable(false); -> toolBarActions.setFloatable(true); in MapFrame.java and we can drag the toolbar

But some sizing and docking properties need fixing (I have no experience in complex Swing GUI).

Last edited 8 years ago by akks (previous) (diff)

comment:10 Changed 8 years ago by Zverikk

I guess making it floatable is a bad idea. Edit toolbar is ok, it's just some buttons are more useful than others. If there is a way to hide lower buttons, why not extend it to the upper ones?

comment:11 Changed 8 years ago by akks

"Extension" means ~100-strings copypasting, unfortunately :(
Current realization is for "ToggleDialog" class only, not for IconToggleButton, and there are actually TWO independent toolbars on left side - editable one and noneditable one:)

Last edited 8 years ago by akks (previous) (diff)

comment:12 Changed 8 years ago by akks

Big copypaste is finished, at last - almost 2 hours. It was good for exploring Swing...
I can not call resulting code "elegant", but it works and it is slightly simpler than for second toolbar.

Here is the patch, you can rework it if needed :)

Last edited 8 years ago by akks (previous) (diff)

comment:13 Changed 8 years ago by akks

Summary: Allow hiding buttons in left toolbar[PATCH][another] Allow hiding buttons in left toolbar

Changed 8 years ago by akks

comment:14 Changed 8 years ago by stoecker

Why otherMapMode? Other for the below buttons was used as the have no name. MapMode buttons have a name: MapMode :-).

Can't the code for these two buttons sets be unified? Must it really be copied? Especially with respect to long-standing request #59.

comment:15 in reply to:  14 Changed 8 years ago by akks

Replying to stoecker:

Why otherMapMode? Other for the below buttons was used as the have no name. MapMode buttons have a name: MapMode :-).

Can't the code for these two buttons sets be unified? Must it really be copied? Especially with respect to long-standing request #59.

"otherMapMode" is not a good name, I agree.

Unification is rather hard, I thought of it.
Problems are:

  • ToggleDialog is a class with its own functionality and additional settings. Saving preferences is implemented inside it - it is logical.
  • There is no list of MapMode buttons, except toolGroup ButtonGroup contents.
  • IconToggleButton is ndefinetly not a place to add preferences...
  • ToggleWindow toolbar is refilled with buttons every time, while I have just used setVisible() to hide buttons. - which is better?
  • Classes with menus are not easy to unify. ToggleDialog menu (OtherButtonsAction) needs access to ToggleDialog methods. We need additional adapter for IconToggleButton?

I did not notice #59. It actually contains lot of ToggleDialog refactoring and idea to split MapFrame and dialogs lificycle. Should MapMode initialization go somewhere too?

I do not know, what to do next... Maybe xeen can finish main refactoring and then I can do unified menus, if needed? Or in reverse order?

Last edited 8 years ago by akks (previous) (diff)

comment:16 Changed 8 years ago by stoecker

I applied large parts of code from #59. Please improve your patch for recent SVN.

comment:17 Changed 8 years ago by anonymous

Should I make another menu for MapModes instead of adding "ohter" button?

comment:18 Changed 8 years ago by akks

Summary: [PATCH][another] Allow hiding buttons in left toolbar[PATCH][reworked] Allow hiding buttons in left toolbar

comment:19 Changed 8 years ago by akks

!!! [currently I am reworking the patch - some variables name are not correct!!! ] !!!

Here is updated patch. It is almost the same copy-paste - style. I do not know is it suitalbe...

I moved "button_hidden" preference management to IconToggleButton (unlike ToggleDialog). I am not sure how to unify button visibilty mamagement - maybe, concentrate it in IconToggleButton? [no, ToggleDialog it seems that ToggleBDialog notify the its button is shown /hidden ] I also do not understand added "menu system" clear enough to append new menu for MapModes :)

Last edited 8 years ago by akks (previous) (diff)

comment:20 Changed 8 years ago by stoecker

After short review: Is the "final" for addmapMode() really required? It changes the signature of a Plugin-used function requiring update of all plugins.

comment:21 Changed 8 years ago by akks

!!! [currently I am reworking the patch - some variables names are not correct and leads to incorrect meaning !!! ]

final is required to show context menu... I know how to remove it, one minute please :)

Changed 8 years ago by akks

comment:22 Changed 8 years ago by akks

Here is a corrected version - "hidden" now means hidden, not "visible" and I also removed "final" from addmapMode() by introducing new final local variable bb.

comment:23 Changed 8 years ago by akks

I have a feeling that all button-related methods should be removed from ToggleDialog. All showing/hiding/setting hidden-preferences for button should go to IconToggleButton class, or, better, to HidebleButton interface.

Elsewhere we can not implement unified show-hide-button logic for MapModes and ToggleDialogs in MapFrame.

Only problem is informing ToggleDialog about its button show-hide. Some events should be added to IconToggleButton (buttonShown, buttonHidden, listened in ToggleDialog) or something else...

But this need rather big refactoring (removing 4 public methods) and can lead to plugin recompilation.

Maybe I am not right, so I'll better wait for comments before starting this patch. Maybe you can make it yourself (for more stability:) )

Last edited 8 years ago by akks (previous) (diff)

comment:24 Changed 8 years ago by akks

[remove - double post]

Last edited 8 years ago by akks (previous) (diff)

comment:24 Changed 8 years ago by akks

I have prepared bigger patch described in previous post.

It completely removes showing-hiding button logic from ToggleDialog.

Showing-Hiding is now concentrated in HideableButton interface implemented by IconToggleButton.
Visibility management and context menus are unified now for both left toolbars (addHideContextMenu method and ListAllButtonsAction class are universal).

I did not change the signatures of ToggleDialogh class (intensively used by pluguins), but removed many methods. It does not affect the plugins, as I see. None of installed plugin crashes and full plugin recompilation works fine too.

Many methods are moved to IconToggleButton, but its existing methods are unchanged.

Please, can your review (or rework this patch) when you have enough time. (I will have very busy week myself...)

Changed 8 years ago by akks

Attachment: bigShowHideButton.patch added

comment:25 Changed 8 years ago by akks

I have prepared bigger patch described in previous post.

It completely removes showing-hiding button logic from ToggleDialog.

Showing-Hiding is now concentrated in HideableButton interface implemented by IconToggleButton.
Visibility management and context menus are unified now for both left toolbars (addHideContextMenu method and ListAllButtonsAction class are universal).

I did not change the signatures of ToggleDialogh class (intensively used by pluguins), but removed many methods. It does not affect the plugins, as I see. None of installed plugin crashes and full plugin recompilation works fine too.

Many methods are moved to IconToggleButton, but its existing methods are unchanged.

Please, can your review or rework this patch when you have enough time. I will have very busy week myself, so my answers may be slow.

comment:26 Changed 8 years ago by akks

Summary: [PATCH][reworked] Allow hiding buttons in left toolbar[PATCH][BIG] Allow hiding buttons in left toolbar

comment:27 Changed 8 years ago by akks

Can you please answer something to confirm that you noticed the patches :)

I am not asking for immediate patch review, it can wait for its time.

comment:28 Changed 8 years ago by stoecker

Ping.

.oO(I've seen, but currently little free time, scheduled for weekend)

comment:29 Changed 8 years ago by akks

OK)

comment:30 Changed 8 years ago by akks

Any chance this weekend?

comment:31 Changed 8 years ago by stoecker

Resolution: fixed
Status: newclosed

In [4609/josm]:

fix #6953 - patch by akks - improve hide handling for left toolbar

comment:32 Changed 8 years ago by stoecker

Sorry for the delay and please keep nagging if this happens in future :-)

comment:33 Changed 8 years ago by akks

You did a huge amount of changes this weekend, really :) Thank you!

comment:34 Changed 8 years ago by stoecker

Resolution: fixed
Status: closedreopened
Type: enhancementdefect

The configuration entries depend on the translated texts. This is an unwanted behaviour and must be fixed.

comment:35 Changed 8 years ago by akks

Actually, there is no untranslated text associated with button actions. Plugins can also add buttons (like PicLayer) and no one can guarantee the icons or class names will be unique. Actions are created like

super(tr("Select"), "move/move", tr("Select, move, scale and rotate objects"), .... )

where "move/move" is and icon name. If it should it be used for preferences, please edit applyButtonHiddenPreferences() and setButtonHidden(boolean b) by replacing

String actionName = (String) getSafeActionValue(AbstractAction.NAME)

with something else. I have no good idea.

comment:36 Changed 8 years ago by stoecker

For menus I somehow fixed that somehow. I think I changed the methods, so that untranslated text is passed.

Version 0, edited 8 years ago by stoecker (next)

comment:37 Changed 8 years ago by akks

Unfortunately, IconToggleButton takes javax.swing.Action in its constructor. Sometimes it is JosmAction, sometimes - AbstractAction, sometimes - MapMode, etc. We can not change it without modifying ToggleDialog and plugins.

What can be unique text ID of any action?

Last edited 8 years ago by akks (previous) (diff)

comment:38 Changed 8 years ago by akks

Maybe icon name would be the best solution (like it was before patching) -but all the icons must be unique and not null.

comment:39 in reply to:  38 Changed 8 years ago by stoecker

Replying to akks:

but all the icons must be unique and not null.

This is anyway a requirement I would say.

comment:40 Changed 8 years ago by akks

It seems I have found correct key by debugging - it is getAction().getValue("toolbar"). The patch will appear soon...

Changed 8 years ago by akks

comment:41 Changed 8 years ago by akks

Here it is.

By the way, some plugins implements custom showing-hiding logic (PicLayer, Routing, ...), they are generally compatible with visibilty menus and so on, but sometimes too many buttons are shown. I do not think it is a serious issue (as it does not show normally, only while reconfiguring process).

comment:42 Changed 8 years ago by akks

Summary: [PATCH][BIG] Allow hiding buttons in left toolbar[additional PATCH] Allow hiding buttons in left toolbar

comment:43 in reply to:  41 Changed 8 years ago by stoecker

Replying to akks:

By the way, some plugins implements custom showing-hiding logic (PicLayer, Routing, ...), they are generally compatible with visibilty menus and so on, but sometimes too many buttons are shown. I do not think it is a serious issue (as it does not show normally, only while reconfiguring process).

Is this really necessary? Can't we change the JOSM interface in a way, that these plugins can access the required features directly from JOSM?

We don't need to keep old workarounds alive forever.

comment:44 Changed 8 years ago by akks

For now, core does not provide the functionality for dynamic showing-hiding.

Should we hide all disabled buttons (disable-enable should go to plugin logic?) or do something else?

Here is the current code from PicLayer plugin:

    /**
     * The toolbar buttons shall be active and visible only when the PicLayer is active.
     */
    @Override
    public void activeLayerChange(Layer oldLayer, Layer newLayer) {
        boolean oldPic = oldLayer instanceof PicLayerAbstract;
        boolean newPic = newLayer instanceof PicLayerAbstract;
        // actually that should be not enough - JOSM should hide all buttons that are disabled for current layer!
        if (oldPic && !newPic || oldLayer == null && !newPic) { // leave picture layer - hide all controls
            for (PicToggleButton btn : buttonList) {
                btn.writeVisible();
                btn.setVisible(false);
            }
            if (oldLayer != null)
                ((PicLayerAbstract)oldLayer).setDrawPoints(false);
        }
        if (!oldPic && newPic) { // enter picture layer - reset visibility of controls
            for (PicToggleButton btn : buttonList)
                btn.readVisible();
        }
    }

comment:45 Changed 8 years ago by stoecker

I think best would be an isValidLayer() in MapMode logic or something like this.

comment:46 Changed 8 years ago by stoecker

Hmm, are we talikng about MapMode or MenuToggleButtons? But for both a isValidLayer() can be useful.

Changed 8 years ago by akks

comment:47 Changed 8 years ago by akks

Yes, MapMode.layerIsSupported looks suitable - thank you for advice.

Please have a look at new version of patch - it hides all unsupported MapModes automatically (plugins can be easily changed to provide layerIsSupported).

This behaviour was not introduced before - try switching layers... Isn't it too radical?

comment:48 Changed 8 years ago by stoecker

Yes, it is a bit radical I think. Probably we should add two additional checks:

  • a hidden config enabling this behaviour for all mapmodes and
  • a setHideDisabled() or something like that which activates this individual for each MapMode(), so plugins can use it.

comment:49 Changed 8 years ago by akks

Here is an updated version of the patch. I noticed that no checking of layer compatibility is needed - button is disabled automatically. So the only needed functionality was to hide disabled buttons or buttons listed as 'hidden' in preferences at right moments.

Added hidden parameter is called "sidetoolbar.hideDisabledButtons" and is set to false by default.

I have also made a sample patch for Routing plugin to illustrate the work.

Changed 8 years ago by akks

Attachment: PluginRoutingPatch.patch added

Changed 8 years ago by akks

comment:50 Changed 8 years ago by stoecker

Resolution: fixed
Status: reopenedclosed

In [4669/josm]: patch by akks - fixes for left hand buttons.

comment:51 Changed 8 years ago by stoecker

See [o27288]. Do you want to update other plugins doing custom stuff as well?

comment:52 Changed 8 years ago by akks

I can do it on weekend if ou or authors will not be faster :)

PicLayer plugin is rather complex, it is better to be fixed by author.

Only ElevationProfile and Piclayer implements custom logic (to be removed), but there are also about 5 more plugin MapModes without isLayerSupported.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.