From kwin Thu Mar 01 11:59:17 2012 From: "Hans Chen" Date: Thu, 01 Mar 2012 11:59:17 +0000 To: kwin Subject: Re: Review Request: Refactor checks for inclusion of TabBoxClients in the client list Message-Id: <20120301115917.6021.25218 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kwin&m=133060320510038 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============5896433456575088825==" --===============5896433456575088825== Content-Type: multipart/alternative; boundary="===============4446788923444192300==" --===============4446788923444192300== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On March 1, 2012, 11:19 a.m., Hans Chen wrote: > > (Disclaimer: I'm just a user, hope it's OK I comment on the UI. :) > > = > > Personally I think the screenshot looks a bit daunting because these co= mboboxes seem to represent a lot of options. How many items are there in ea= ch dropdown list? If there are only two, I would personally prefer checkbox= es like in the taskmanager widget settings, e.g. > > = > > Only show windows from the current application [ ] > > Only show windows from the current screen [ ] > > Only show windows from the current desktop [ ] > > Only show windows from the current activity [ ] > > Only show windows that are minimized [ ] > > Include desktop in the list [ ] > > = > > In my opinion it makes it clearer what each option does, you only have = two choices for each option (yes/no) and it makes it consistent with Plasma= (note that the taskmanager uses "tasks" instead of "windows"). > > = > > I noticed that you wrote "you [Martin] requested all checks to be combo= boxes" - was this what you referred to? > = > Martin Gr=C3=A4=C3=9Flin wrote: > the user interface has to be changed, that is quite clear. But it is = unrelated to the refactoring changes in fact. > = > What I want to see is making the layout selection the most prominent = part of the UI and "hide" the settings. For the settings I'm thinking of an= approach like the mouse actions for Plasma Desktop. Using a button to "Add= another restriction" or something like that. Would make the view much less= cluttered. > = > Stefano Avallone wrote: > As Martin said, the UI has to be redesigned. To reply to Hans' questi= on: the Applications, Minimized and MultiScreen combo boxes have three opti= ons (please have a look at the enum types in the new version of tabboxconfi= g.h) > = > To Martin: by the week end, I'll try to upload a new version of the d= iff addressing your comment above and rebased against current master Thank you both for your replies! @Martin: I had a similar thought but would prefer one that requires as litt= le "work" from the user as possible. @Stefano: I see, that makes it more complicated. Since the UI is unrelated to this patch I'll add my comments regarding it s= omewhere else, possibly the KWin mailing list. - Hans ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104025/#review11049 ----------------------------------------------------------- On Feb. 21, 2012, 3:42 p.m., Stefano Avallone wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104025/ > ----------------------------------------------------------- > = > (Updated Feb. 21, 2012, 3:42 p.m.) > = > = > Review request for kwin and Martin Gr=C3=A4=C3=9Flin. > = > = > Description > ------- > = > This patch refactors the checks for inclusion of TabBox clients in the cl= ient list. All the checks have been implemented as dropdowns. Quoting Marti= n, the checks are: > = > * Desktop: Windows From All Desktops, Windows From Current Desktop > * Activities: Windows From All Activities, Windows From Current Activity > * Applications: All Windows From All Applications, Only one Window per > Application, Only all Windows of Current Application > * Minimized: Ignore, Minimized windows only, Exclude Minimized Windows > * Desktop: Don't Show Desktop, Show Desktop to minimize all windows > * Multi Screen: Ignore, Only Windows from current screen, Exclude windows= from > current screen > = > Each of such checks has been implemented as a private method of TabBoxHan= dlerImpl. They are called by TabBoxHandlerImpl::clientToAddToList, while Cl= ientModel::createClientList only handles the switching mode (FocusChain/Sta= ckingOrder) and whether a desktop client should be added to the list to min= imize all the clients. In TabBoxHandlerImpl::clientToAddToList, I left the = following line: > = > addClient =3D addClient && current->wantsTabFocus() && !current->skipSwit= cher(); > = > as I am not sure if it is needed or not. > = > The UI is a bit crowded now, but I would leave a UI refactor for a future= commit. > = > = > Diffs > ----- > = > kwin/kcmkwin/kwintabbox/main.cpp 3f047d1 = > kwin/kcmkwin/kwintabbox/main.ui a316b28 = > kwin/tabbox/clientmodel.cpp 4ca7fef = > kwin/tabbox/declarative.cpp dd4f242 = > kwin/tabbox/tabbox.h 6d4e2b2 = > kwin/tabbox/tabbox.cpp 3f26397 = > kwin/tabbox/tabboxconfig.h 32facde = > kwin/tabbox/tabboxconfig.cpp 9feaa16 = > kwin/tabbox/tabboxhandler.h 27b475a = > = > Diff: http://git.reviewboard.kde.org/r/104025/diff/ > = > = > Testing > ------- > = > Desktop, Minimized, Show desktop: work fine > = > Multi Screen: could not test, as I do not have a multi screen configurati= on > = > Activities: works fine. However, when using the present windows effect as= task switcher, the previews of the windows on other activities are not vis= ible. They are actually there, because moving the mouse to the area where t= hey (presumably) are makes the close button appear and allows to activate t= hem. When using a layout based switcher, thumbnails of windows on other act= ivities are correctly shown. So, I guess, it is something unrelated to the = checks I made. > = > Applications: the new case I implemented is "Only all Windows of Current = Application" (others work fine). I tested it with KDevelop+About KDE+About = Kdevelop, and it works fine. It doesn't work with the three windows opened = by Gimp, in the sense that only one window (instead of all of them) is show= n. Not sure if my check is incorrect, here. > = > = > Screenshots > ----------- > = > Modified UI > http://git.reviewboard.kde.org/r/104025/s/438/ > = > = > Thanks, > = > Stefano Avallone > = > --===============4446788923444192300== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/104025/

On March 1st, 2012, 11:19 a.m., Hans Chen wrote:

(Disclaim=
er: I'm just a user, hope it's OK I comment on the UI. :)

Personally I think the screenshot looks a bit daunting because these combob=
oxes seem to represent a lot of options. How many items are there in each d=
ropdown list? If there are only two, I would personally prefer checkboxes l=
ike in the taskmanager widget settings, e.g.

Only show windows from the current application [ ]
Only show windows from the current screen [ ]
Only show windows from the current desktop [ ]
Only show windows from the current activity [ ]
Only show windows that are minimized [ ]
Include desktop in the list [ ]

In my opinion it makes it clearer what each option does, you only have two =
choices for each option (yes/no) and it makes it consistent with Plasma (no=
te that the taskmanager uses "tasks" instead of "windows&quo=
t;).

I noticed that you wrote "you [Martin] requested all checks to be comb=
o boxes" - was this what you referred to?

On March 1st, 2012, 11:25 a.m., Martin Gr=C3=A4=C3=9Flin wrote:<= /p> 

the user =
interface has to be changed, that is quite clear. But it is unrelated to th=
e refactoring changes in fact.

What I want to see is making the layout selection the most prominent part o=
f the UI and "hide" the settings. For the settings I'm thinki=
ng of an approach like the mouse actions for Plasma Desktop. Using a button=
 to "Add another restriction" or something like that. Would make =
the view much less cluttered.

On March 1st, 2012, 11:43 a.m., Stefano Avallone wrote:

As Martin=
 said, the UI has to be redesigned. To reply to Hans' question: the App=
lications, Minimized and MultiScreen combo boxes have three options (please=
 have a look at the enum types in the new version of tabboxconfig.h)

To Martin: by the week end, I'll try to upload a new version of the dif=
f addressing your comment above and rebased against current master
Thank you b=
oth for your replies!

@Martin: I had a similar thought but would prefer one that requires as litt=
le "work" from the user as possible.

@Stefano: I see, that makes it more complicated.

Since the UI is unrelated to this patch I'll add my comments regarding =
it somewhere else, possibly the KWin mailing list.

- Hans


On February 21st, 2012, 3:42 p.m., Stefano Avallone wrote:

Review request for kwin and Martin Gr=C3=A4=C3=9Flin.
By Stefano Avallone.

Updated Feb. 21, 2012, 3:42 p.m.

Descripti= on 

This patch refactors the checks for inclusion of TabBox clie=
nts in the client list. All the checks have been implemented as dropdowns. =
Quoting Martin, the checks are:

* Desktop: Windows From All Desktops, Windows From Current Desktop
* Activities: Windows From All Activities, Windows From Current Activity
* Applications: All Windows From All Applications, Only one Window per
Application, Only all Windows of Current Application
* Minimized: Ignore, Minimized windows only, Exclude Minimized Windows
* Desktop: Don't Show Desktop, Show Desktop to minimize all windows
* Multi Screen: Ignore, Only Windows from current screen, Exclude windows f=
rom
current screen

Each of such checks has been implemented as a private method of TabBoxHandl=
erImpl. They are called by TabBoxHandlerImpl::clientToAddToList, while Clie=
ntModel::createClientList only handles the switching mode (FocusChain/Stack=
ingOrder) and whether a desktop client should be added to the list to minim=
ize all the clients. In TabBoxHandlerImpl::clientToAddToList, I left the fo=
llowing line:

addClient =3D addClient && current->wantsTabFocus() && !=
current->skipSwitcher();

as I am not sure if it is needed or not.

The UI is a bit crowded now, but I would leave a UI refactor for a future c=
ommit.

Testing <= /h1> 
Desktop, Minimized, Show desktop: work fine

Multi Screen: could not test, as I do not have a multi screen configuration

Activities: works fine. However, when using the present windows effect as t=
ask switcher, the previews of the windows on other activities are not visib=
le. They are actually there, because moving the mouse to the area where the=
y (presumably) are makes the close button appear and allows to activate the=
m. When using a layout based switcher, thumbnails of windows on other activ=
ities are correctly shown. So, I guess, it is something unrelated to the ch=
ecks I made.

Applications: the new case I implemented is "Only all Windows of Curre=
nt Application" (others work fine). I tested it with KDevelop+About KD=
E+About Kdevelop, and it works fine. It doesn't work with the three win=
dows opened by Gimp, in the sense that only one window (instead of all of t=
hem) is shown. Not sure if my check is incorrect, here.

Diffs=

  • kwin/kcmkwin/kwintabbox/main.cpp (3f047d1)=
  • kwin/kcmkwin/kwintabbox/main.ui (a316b28)<= /span>
  • kwin/tabbox/clientmodel.cpp (4ca7fef)
  • kwin/tabbox/declarative.cpp (dd4f242)
  • kwin/tabbox/tabbox.h (6d4e2b2)
  • kwin/tabbox/tabbox.cpp (3f26397)
  • kwin/tabbox/tabboxconfig.h (32facde)
  • kwin/tabbox/tabboxconfig.cpp (9feaa16)
  • kwin/tabbox/tabboxhandler.h (27b475a)

View Diff

Screensho= ts

3D"Modified
--===============4446788923444192300==-- --===============5896433456575088825== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kwin mailing list kwin@kde.org https://mail.kde.org/mailman/listinfo/kwin --===============5896433456575088825==--