Fix: activate "next/previous layer" shortcut for layers or group which are invisible.
ClosedPublic

Authored by anikethgirish on Jan 31 2017, 4:13 PM.

Details

Summary

use PgDn, PgUp as shortcuts to move through the invisible layers inside a group.

Test Plan

Checked by building it.

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anikethgirish retitled this revision from to Fix: activate "next/previous layer" shortcut for layers or group which are invisible..
anikethgirish updated this object.
anikethgirish edited the test plan for this revision. (Show Details)
anikethgirish added a reviewer: rempt.
anikethgirish set the repository for this revision to R504 Krita Extension - Deskew.
anikethgirish added a project: Krita.
anikethgirish added a subscriber: Krita.
Restricted Application added a subscriber: woltherav. · View Herald TranscriptJan 31 2017, 4:13 PM
anikethgirish edited the test plan for this revision. (Show Details)Jan 31 2017, 4:13 PM
dkazakov requested changes to this revision.Feb 2 2017, 8:11 AM
dkazakov added a reviewer: dkazakov.
dkazakov added subscribers: Bollebib, dkazakov.

Hi, @anikethgirish!

The patch has a couple of problems. It introduces strange checks, which are effectively (A || !A) conditions. I mean, that if you want exactly this behavior, like "enter every layer", you can just remove the check for isEditable() and it'll be exactly the same code as in your patch. Another question, if the users also want to enter locked groups. If so, then just removing isEditable() will work. If not, then it should be changed to isLocked() or something. Please as @Bollebib if he thinks the shortcut should also enter locked groups (I guess it should, because we don't skip locked layers outside groups).

libs/ui/kis_node_manager.cpp
838

As far as I can tell, this is also a (A || !A) change, but without a loop.

865

What do you want to achieve with this change? Your code is basically ad equivalent of

if (activeNode->childCount() > 0) {
    node = activeNode->lastChild();
}

because (!A || A) is always true.

This revision now requires changes to proceed.Feb 2 2017, 8:11 AM
anikethgirish edited edge metadata.
anikethgirish removed R504 Krita Extension - Deskew as the repository for this revision.

@dkazakov Hi,

Okay. So it seems like if we remove the check that if the node is editable the current bug will be fixed right. But what if we don't want to enter into nodes which are locked. Or doing this should also allow us to enter the locked layers too?

As of now, I have changed it into like we could enter invisible layers but not locked layers. Maybe this helps in solving the issue.

dkazakov requested changes to this revision.Feb 3 2017, 2:44 PM
dkazakov edited edge metadata.

Hi, @anikethgirish!

@Bollebib answered that he would prefer if the shortcut would enter any kind of layer: invisible and locked.

17:05 < dmitryK|log> Bollebib_Laptop: hi! could you please check clarify a bit about the bug 336899
17:05 < sKreamer> KDE bug 336899 in krita (Layer Stack) "JJ "activate next/previous layer" shorcut" [wishlist,] https://bugs.kde.org/show_bug.cgi?id=336899
17:05 < dmitryK|log> Bollebib_Laptop: should the PgDn shortcut also enter the locked groups?
17:06 < dmitryK|log> Bollebib_Laptop: or should it skip only invisible layers?
17:13 < Bollebib_Laptop> dmitryK|log: it should skip nothing,in my opinion
17:16 < dmitryK|log> Bollebib_Laptop: okay
17:17 < dmitryK|log> Bollebib_Laptop: thank you :)
This revision now requires changes to proceed.Feb 3 2017, 2:44 PM
anikethgirish edited edge metadata.

So @dkazakov, I have made the required changes. Hope this will be helpful.

This revision was automatically updated to reflect the committed changes.