From kde-kimageshop Tue Jul 10 13:01:09 2012 From: Sven Langkamp Date: Tue, 10 Jul 2012 13:01:09 +0000 To: kde-kimageshop Subject: Re: Locking branch ready for testing Message-Id: X-MARC-Message: https://marc.info/?l=kde-kimageshop&m=134192552003800 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============1379092681208871237==" --===============1379092681208871237== Content-Type: multipart/alternative; boundary=485b397dd665fe5a7604c479528f --485b397dd665fe5a7604c479528f Content-Type: text/plain; charset=ISO-8859-1 On Mon, Jul 9, 2012 at 10:25 PM, Sven Langkamp wrote: > On Mon, Jul 9, 2012 at 12:14 AM, T Hall wrote: > >> On Sun, 8 Jul 2012 22:02:25 +0200 >> Sven Langkamp wrote: >> >> > On Sun, Jul 8, 2012 at 4:39 PM, T Hall wrote: >> > >> > > Hi Sven, >> > > >> > > I've just compiled your branch and given it a quick test. These are >> > > my observations so far: >> > > >> > > > - tools can be changed even if the active layer is >> > > > locked/invisible, no longer grayed out (unless it's a shape layer) >> > > Seems to work fine. >> > > > - cursor/brush outline is still shown when the layer is >> > > > locked/invisible >> > > Works fine. >> > > > - selections can be edited even if the current layer is locked, >> > > > unless there is a locked local selection >> > > Selection tools work. However, there are a couple of functions >> > > (Deselect, Reselect and, Invert Selection), which aren't allowed on >> > > locked layers (Greyed-out in menu, shortcuts do nothing). Since >> > > these functions only edit selections, and don't affect the layer, I >> > > think they should be allowed when the selected layer is locked. >> > > >> > >> > Fixed, now just depends on the selection state. >> Invert Selection is still greyed-out for locked layers. In theory, it >> should work on any selection. It is available when the layer is not >> locked, so it's definitely a locking issue. (I also think it *should* >> work when nothing is selected. Invert nothing == select all. I fail to >> see any advantage to blocking this possibility) >> > > Fixed all the selection actions. The layer actions still are showing the > old behavior, but that's not a regression that comes from the branch. > > We need someone to go through all of them and then set the correct > behavior for each. It's quite easy so would be ideal for a junior job. > >> > >> > >> > > > - if the user tries to paint on a locked layer, Krita will show a >> > > > small notification >> > > Works fine. However, no notification is shown if the user tried to >> > > edit the via a shortcut. e.g. pressing backspace to fill the layer >> > > with the foreground colour (the application behaves correctly, in >> > > that it doesn't allow the layer to be filled, it's only the >> > > notification that's missing). I think it would be nice if the >> > > notification appeared for such actions. (I haven't looked at how >> > > locking a layer disables/blocks shortcuts/actions like this, so >> > > don't know how difficult it would be to implement this) >> > > >> > > Additionally: >> > > Items from the "Layer" menu (Mirror Horizontally, Mirror >> > > Vertically, Shear Layer, Scale Layer, Rotate..., Convert Layer Type) >> > > are still available for use on locked layers. Most of these are >> > > destructive (not revertable except via undo), so definitely >> > > shouldn't be allowed on locked layers. >> > > >> > >> > This is kind of a problem. In KDE it usual that if the action is >> > disabled menu entry and shortcut are disabled. So we could either >> > have a notification or disable the menu entry. Not sure what to do >> > there. >> I guess we'll have to manage without notifications for >> disallowed shortcuts. If the user is using the menus, the item being >> greyed-out at least tells the user that the action is not permitted (as >> opposed to the various possibilities when nothing appears to happen >> when you press a shortcut: could be you pressed the wrong key, maybe it >> did something but it wasn't visible/what you expected to see, etc. >> etc.) Presumably if KDE is designed to not do anything for disabled >> shortcuts/menu items, then any workaround would probably be rather >> messy and more effort than it's worth. >> >> In the mean time, I notice the items from the Layer menu are still >> active. I really don't think they should be. >> > >> > >> > >> > > The transform tool also exhibits some odd behaviour (What's >> > > new :p). It allows you to make the transformation, as long as you >> > > don't apply it (Seems reasonable). However, when you try to apply >> > > it (and get the "Layer is locked" notification), it returns all the >> > > handles to their original position. >> > > 1. It appears you can't keep your transformation and either carry on >> > > transforming or unlock the layer then apply it (if you really meant >> > > to do the transformation, but forgot to unlock the layer first). >> > > 2. Actually, if (before attempting further transformation) you >> > > switch to another tool, then switch back to the transform tool, >> > > then your transformation reappears as it was before you tried to >> > > apply it (allowing you to carry on transforming, or unlock and >> > > apply). >> > > >> > > I believe correct behaviour should be: >> > > When you try to apply the transformation, handles remain in their >> > > transformed position, allowing you to continue working with the >> > > transformation (without having to switch to another tool and back). >> > > >> > >> > Fixed, handles remain now. >> Handles remain, but it isn't possible to manipulate them further >> without switching to another tool, then switching back to the transform >> tool. (try to apply, then unlock and apply now works, it's only if you >> want to tweak the transformation after trying to apply that it still >> doesn't work) >> > > Works here. > > >> > >> > >> > > Probably unrelated: I notice I can't make a selection when the >> > > group layer is selected, be it unlocked or locked. Following the >> > > logic of selection tools do not edit the selected layer, perhaps >> > > they should be allowed? This could also be related to allowing the >> > > transform tool to transform multiple layers at the same time by >> > > operating on a group layer (feature request detailed in bug >> > > 297927). Anyway, I don't think this is related to 'lockedness' or >> > > specific to your branch, so I guess it's offtopic in this email. >> > > >> > > >> > Fixed. >> For a moment, I wondered if you'd fixed bug 297927, but I see we're now >> back to being able to make selections with a group layer selected, >> which at least means the bug is now accurately reproducible once more :) >> I suspect when that bug is fixed, the group layer will need to take into >> account the 'lockedness' of all it's child layers when being edited. (I >> am thinking that transforming a whole group of layers should not be >> allowed if one of them is locked.) >> > >> > >> > > So to summarise: >> > > - Selection functions disallowed >> > > - No notifications shown for shortcuts/menu actions >> > > - Items from the Layer menu allowed to edit locked layers >> > > - Transform tool oddities >> > > - Incorrect notification for group layer locked >> > > >> > > Hope that helps. >> > > >> > >> > It did :). Thanks a lot for the detailed testing. >> >> > Branch is now merged to master. --485b397dd665fe5a7604c479528f Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Mon, Jul 9, 2012 at 10:25 PM, Sven Langkamp <= span dir=3D"ltr"><sven.langkamp@gmail.com> wrote:
On Mon, Jul 9, 2012 at 12= :14 AM, T Hall <tahall256@gmail.com> wrote:
On Sun, 8 Jul 2012 22:02:25 +0200
Sven Langkamp <sven.langkamp@gmail.com> wrote:

> On Sun, Jul 8, 2012 at 4:39 PM, T Hall <tahall256@gmail.com> wrote:
>
> > Hi Sven,
> >
> > I've just compiled your branch and given it a quick test. The= se are
> > my observations so far:
> >
> > > - tools can be changed even if the active layer is
> > > locked/invisible, no longer grayed out (unless it's a sh= ape layer)
> > Seems to work fine.
> > > - cursor/brush outline is still shown when the layer is
> > > locked/invisible
> > Works fine.
> > > - selections can be edited even if the current layer is lock= ed,
> > > unless there is a locked local selection
> > Selection tools work. However, there are a couple of functions > > (Deselect, Reselect and, Invert Selection), which aren't allo= wed on
> > locked layers (Greyed-out in menu, shortcuts do nothing). Since > > these functions only edit selections, and don't affect the la= yer, I
> > think they should be allowed when the selected layer is locked. > >
>
> Fixed, now just depends on the selection state.
Invert Selection is still greyed-out for locked layers. In theory, it=
should work on any selection. It is available when the layer is not
locked, so it's definitely a locking issue. (I also think it *should* work when nothing is selected. Invert nothing =3D=3D select all. I fail to<= br> see any advantage to blocking this possibility)

Fixed all the selection actions. The layer actions still are show= ing the old behavior, but that's not a regression that comes from the b= ranch.

We need someone to go through all of them and then set the correct=20 behavior for each. It's quite easy so would be ideal for a junior job.<= /div>
>
>
> > > - if the user tries to paint on a locked layer, Krita will s= how a
> > > small notification
> > Works fine. However, no notification is shown if the user tried t= o
> > edit the via a shortcut. e.g. pressing backspace to fill the laye= r
> > with the foreground colour (the application behaves correctly, in=
> > that it doesn't allow the layer to be filled, it's only t= he
> > notification that's missing). I think it would be nice if the=
> > notification appeared for such actions. (I haven't looked at = how
> > locking a layer disables/blocks shortcuts/actions like this, so > > don't know how difficult it would be to implement this)
> >
> > Additionally:
> > Items from the "Layer" menu (Mirror Horizontally, Mirro= r
> > Vertically, Shear Layer, Scale Layer, Rotate..., Convert Layer Ty= pe)
> > are still available for use on locked layers. Most of these are > > destructive (not revertable except via undo), so definitely
> > shouldn't be allowed on locked layers.
> >
>
> This is kind of a problem. In KDE it usual that if the action is
> disabled menu entry and shortcut are disabled. So we could either
> have a notification or disable the menu entry. Not sure what to do
> there.
I guess we'll have to manage without notifications for
disallowed shortcuts. If the user is using the menus, the item being
greyed-out at least tells the user that the action is not permitted (as
opposed to the various possibilities when nothing appears to happen
when you press a shortcut: could be you pressed the wrong key, maybe it
did something but it wasn't visible/what you expected to see, etc.
etc.) Presumably if KDE is designed to not do anything for disabled
shortcuts/menu items, then any workaround would probably be rather
messy and more effort than it's worth.

In the mean time, I notice the items from the Layer menu are still
active. I really don't think they should be.
>
>
>
> > The transform tool also exhibits some odd behaviour (What's > > new :p). It allows you to make the transformation, as long as you=
> > don't apply it (Seems reasonable). However, when you try to a= pply
> > it (and get the "Layer is locked" notification), it ret= urns all the
> > handles to their original position.
> > 1. It appears you can't keep your transformation and either c= arry on
> > transforming or unlock the layer then apply it (if you really mea= nt
> > to do the transformation, but forgot to unlock the layer first).<= br> > > 2. Actually, if (before attempting further transformation) you > > switch to another tool, then switch back to the transform tool, > > then your transformation reappears as it was before you tried to<= br> > > apply it (allowing you to carry on transforming, or unlock and > > apply).
> >
> > I believe correct behaviour should be:
> > When you try to apply the transformation, handles remain in their=
> > transformed position, allowing you to continue working with the > > transformation (without having to switch to another tool and back= ).
> >
>
> Fixed, handles remain now.
Handles remain, but it isn't possible to manipulate them further<= br> without switching to another tool, then switching back to the transform
tool. (try to apply, then unlock and apply now works, it's only if you<= br> want to tweak the transformation after trying to apply that it still
doesn't work)

Works here.
=A0