Modify

Opened 4 months ago

Closed 3 months ago

Last modified 3 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 4 months ago.
Initial patch (no tests)
18638.1.patch (10.5 KB) - added by taylor.smock 4 months ago.
Switch back to [ and ] from { and } along with tests
18638.review.patch (10.9 KB) - added by Don-vip 3 months ago.
codestyle fixed. Beware to please the Shortcut parser
18638_junit.jpg (25.5 KB) - added by Don-vip 3 months ago.
18638.2.patch (10.0 KB) - added by taylor.smock 3 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 3 months ago.
Actually initialize the actions (in LayerListDialog)
18638.shortcuts.patch (1.8 KB) - added by GerdP 3 months ago.
18638.shortcuts_names.patch (2.8 KB) - added by taylor.smock 3 months ago.
Modify GerdP's patch to add consistency in action UI descriptions

Download all attachments as: .zip

Change History (28)

Changed 4 months ago by taylor.smock

Attachment: 18638.patch added

Initial patch (no tests)

comment:1 Changed 4 months ago by taylor.smock

Description: modified (diff)

Changed 4 months ago by taylor.smock

Attachment: 18638.1.patch added

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

comment:2 Changed 4 months ago by taylor.smock

Description: modified (diff)

comment:3 Changed 4 months ago by taylor.smock

Ping

comment:4 Changed 3 months ago by Don-vip

Milestone: 20.02

comment:5 Changed 3 months ago by Don-vip

Milestone: 20.0220.03

Changed 3 months ago by Don-vip

Attachment: 18638.review.patch added

codestyle fixed. Beware to please the Shortcut parser

comment:6 Changed 3 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:
No image "18638_junit" attached to Ticket #18638

Version 0, edited 3 months ago by Don-vip (next)

Changed 3 months ago by Don-vip

Attachment: 18638_junit.jpg added

Changed 3 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 3 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 3 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 3 months ago by skyper

Milestone 20.2

comment:10 Changed 3 months ago by taylor.smock

Resolution: fixed
Status: closedreopened

I forgot to add the initialization code to the patch.

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

Changed 3 months ago by taylor.smock

Attachment: 18638.3.patch added

Actually initialize the actions (in LayerListDialog)

comment:11 Changed 3 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 3 months ago by Klumbumbus

Milestone: 20.0320.02

comment:13 Changed 3 months ago by Zverikk

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

comment:14 Changed 3 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 3 months ago by Zverikk

Ah, indeed, thank you.

comment:16 Changed 3 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 3 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 3 months ago by GerdP

Attachment: 18638.shortcuts.patch added

comment:18 Changed 3 months ago by GerdP

Resolution: fixed
Status: closedreopened

My patch fixes only the issue reported in comment:17

Changed 3 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 3 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 3 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.