Modify

Opened 16 months ago

Closed 15 months ago

Last modified 15 months ago

#18638 closed enhancement (fixed)

[PATCH] There should be a method to cycle through layers

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 20.02
Component: Core Version:
Keywords: layer, cycle Cc:

Description (last modified by taylor.smock)

I originally used CTRL+[/CTRL+] for the layer switches, but I figured SHIFT+{/SHIFT+} would be better ([ -> { due to SHIFT converting [ to {).

I haven't written tests yet -- I'm doing that now.

I probably didn't pick the right class to instantiate the new class in (I'm currently using LayerListDialog, but I figured that was probably the best place to put it).

Attachments (8)

18638.patch (6.0 KB) - added by taylor.smock 16 months ago.
Initial patch (no tests)
18638.1.patch (10.5 KB) - added by taylor.smock 16 months ago.
Switch back to [ and ] from { and } along with tests
18638.review.patch (10.9 KB) - added by Don-vip 15 months ago.
codestyle fixed. Beware to please the Shortcut parser
18638_junit.jpg (25.5 KB) - added by Don-vip 15 months ago.
18638.2.patch (10.0 KB) - added by taylor.smock 15 months ago.
Split cycle up/cycle down actions into separate files, fix shortcut issues (single line), and some other pmd issues
18638.3.patch (2.1 KB) - added by taylor.smock 15 months ago.
Actually initialize the actions (in LayerListDialog)
18638.shortcuts.patch (1.8 KB) - added by GerdP 15 months ago.
18638.shortcuts_names.patch (2.8 KB) - added by taylor.smock 15 months ago.
Modify GerdP's patch to add consistency in action UI descriptions

Download all attachments as: .zip

Change History (28)

Changed 16 months ago by taylor.smock

Attachment: 18638.patch added

Initial patch (no tests)

comment:1 Changed 16 months ago by taylor.smock

Description: modified (diff)

Changed 16 months ago by taylor.smock

Attachment: 18638.1.patch added

Switch back to [ and ] from { and } along with tests

comment:2 Changed 16 months ago by taylor.smock

Description: modified (diff)

comment:3 Changed 15 months ago by taylor.smock

Ping

comment:4 Changed 15 months ago by Don-vip

Milestone: 20.02

comment:5 Changed 15 months ago by Don-vip

Milestone: 20.0220.03

Changed 15 months ago by Don-vip

Attachment: 18638.review.patch added

codestyle fixed. Beware to please the Shortcut parser

comment:6 Changed 15 months ago by Don-vip

I've modified your patch to respect checkstyle rules and make sure the Shortcut parser will work properly. But I got three unit test failures on 6:

Last edited 15 months ago by Don-vip (previous) (diff)

Changed 15 months ago by Don-vip

Attachment: 18638_junit.jpg added

Changed 15 months ago by taylor.smock

Attachment: 18638.2.patch added

Split cycle up/cycle down actions into separate files, fix shortcut issues (single line), and some other pmd issues

comment:7 in reply to:  6 Changed 15 months ago by taylor.smock

I put it in the MapWithAI plugin temporarily, and it also complained about the tests. I fixed them, but forgot to update the patch here.
It turns out that (for whatever reason), the CI environment wasn't passing the modifier keys, IIRC. I ended up splitting the actions up, instead of trying to have one class (it worked on Mac OS X, but not in the CI environment).

I forgot to bring the changes back over to this bug report. I should have, so you wouldn't have duplicated some of the work.

comment:8 Changed 15 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 15923/josm:

fix #18638 - add a method to cycle through layers (patch by taylor.smock, modified)

comment:9 Changed 15 months ago by skyper

Milestone 20.2

comment:10 Changed 15 months ago by taylor.smock

Resolution: fixed
Status: closedreopened

I forgot to add the initialization code to the patch.

Last edited 15 months ago by taylor.smock (previous) (diff)

Changed 15 months ago by taylor.smock

Attachment: 18638.3.patch added

Actually initialize the actions (in LayerListDialog)

comment:11 Changed 15 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 15929/josm:

fix #18638 - Initialize the cycle layer actions in LayerListDialog (patch by taylor.smock)

comment:12 Changed 15 months ago by Klumbumbus

Milestone: 20.0320.02

comment:13 Changed 15 months ago by Zverikk

By the way, [ and ] keys are used by the "todo" plugin to cycle through the todo list.

comment:14 Changed 15 months ago by taylor.smock

I knew that, which is why I used shift + [/shift + ] (which translates the ] to } and [ to {, which made testing interesting).

comment:15 Changed 15 months ago by Zverikk

Ah, indeed, thank you.

comment:16 Changed 15 months ago by Hb---

The names for the keyboard shortcuts and for the action seem not consistent to each other. Currently we have

Cycle layers up for the shortcut
Cycle layer up for the action and
Cycle up through layers for the hint.

Cycle layers down for the shortcut
Cycle layers for the down action and
Cycle through layers for the hint

Are different names for the shortcut and for the action necessary?

Why is plural used when only one layer is affected? This good action is not as shuffling the background layers around each other.

Suggestion:
Cycle layer up for shortcut and action
Cycle through data layers upwards for the hint.

Cycle layer down for the shortcut and action
Cycle through data layers downwards for the hint

comment:17 Changed 15 months ago by GerdP

FYI: I see this message in some unit tests, e.g. JoinAreasActionTest

2020-03-06 09:40:10.255 INFORMATION: Registered toolbar action cycle-layer overwritten: org.openstreetmap.josm.gui.dialogs.layer.CycleLayerUpAction gets org.openstreetmap.josm.gui.dialogs.layer.CycleLayerDownAction

Changed 15 months ago by GerdP

Attachment: 18638.shortcuts.patch added

comment:18 Changed 15 months ago by GerdP

Resolution: fixed
Status: closedreopened

My patch fixes only the issue reported in comment:17

Changed 15 months ago by taylor.smock

Attachment: 18638.shortcuts_names.patch added

Modify GerdP's patch to add consistency in action UI descriptions

comment:19 Changed 15 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 16090/josm:

fix #18638 - add consistency in action UI descriptions (patch by taylor.smock)

comment:20 Changed 15 months ago by Don-vip

In 16097/josm:

see #18638 - checkstyle

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.