Cycle between windows of the same desktop on switch
ClosedPublic

Authored by pedroarthurp on Dec 5 2016, 6:05 PM.

Details

Summary

When switching virtual desktops using shortcuts, the behavior is to switch to a virtual desktop in the opposite direction if the current one is on the edges of the layout. For example, in a one row layout with 4 virtual desktops, "Switch One Desktop to the Left" while in virtual desktop # 1 will send the user to virtual desktop # 4. Likewise, in a 3 rows layout with 9 virtual desktops, "Switch One Desktop Down" while in virtual desktop # 8 will lead the user to desktop # 2.

This patch uses the same behavior whilst changing windows using "Switch to Window Above|Below|to the Right|to the Left". For example, in a 3 display set-up (my set-up), the user would go from an application in the rightmost position to an application in the leftmost position using just one key combination: "Switch to Window to the Right". Currently, the shortcuts are no-op in these cases (ie, trying "Switch to Window to the Left" from the leftmost window has no outcome, which is mostly accurate with the shortcut semantics but totally useless in behavior).

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
pedroarthurp updated this revision to Diff 8781.Dec 5 2016, 6:05 PM
pedroarthurp retitled this revision from to Cycle between windows of the same desktop on switch.
pedroarthurp updated this object.
pedroarthurp edited the test plan for this revision. (Show Details)
pedroarthurp set the repository for this revision to R108 KWin.
Restricted Application added a project: KWin. · View Herald TranscriptDec 5 2016, 6:05 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
broulik added a reviewer: KWin.Dec 5 2016, 6:15 PM

Conceptually I'm not sure about it. If I click switch to window on left, I expect it to do that and not to switch to a window right of it. What does a user expect and is your changed behavior still in accordance to what the user expects?

I'm not going to decide on that, this needs input from the usability experts.

useractions.cpp
1579

this method does not need to be a member of Workspace. You can just turn it into a static local method.

workspace.h
506–508

the old method just delegates to the new one. Remove it and use a default argument.

507

please prevent boolean trap in new API: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap

508

please prevent boolean trap in new API

graesslin added a subscriber: colomar.
pedroarthurp added a comment.EditedDec 5 2016, 10:52 PM

Conceptually I'm not sure about it. If I click switch to window on left, I expect it to do that and not to switch to a window right of it. What does a user expect and is your changed behavior still in accordance to what the user expects?

I'm not going to decide on that, this needs input from the usability experts.

First things first, thank you for the reply. I will address the coding issues ASAP.

I do agree with you. However, I consider it consistent with Virtual Desktop switching behavior. Indeed, I thought of this feature in the first place when I found that Virtual Desktop behavior was the one I described.

As a heavy keyboard user, I think it is mostly useful. Just to try to make you realize it, I am a distributed software developer. I have a set-up consisting of three wide screen displays: a 22' display on the left, a 20' display rotated 90 degree in the middle, and a 22' display on the right. In the middle, I keep a full-screen IDE window. In the left and the right, I keep up to four konsole windows where I inspect logs, network packets, system resources, etc. To organize the windows, I use the tiling shortcuts (ie, "Quick Tile Window to the Bottom|Top Left|Right"). As you may know, these shortcuts will divide screens in smaller quadrants. So, in these setting, I will end up with 5 "sectors" at most (ie, two in the left display, one in the middle, and other two in the right one). In order to quickly navigate, I use the switching shortcuts I tweaked in the patch.

The problem arise when I am in a window in the first "sector" (considering the leftmost display as the first one) and I need to focus the input in a window in the last "sector". Although I could use "Walk through windows", I think it consumes a lot more "attention" then using the "Switch to Windows A|B|tL|tR". Hence, I end up using this latter method. Indeed, I just use "Walk through Windows" when I have just two maximized windows. With the tweaks, I propose in this patch, I can easy navigate from one place to another with fewer shortcuts combinations.

Just as a reference, I posted my shortcuts at https://gist.github.com/pedroarthur/3ffac1752fa2fadbbe9448cbcc78d058

As I said: I won't decide on it. That's up for the usability experts to decide on.

romangg added a subscriber: romangg.EditedDec 6 2016, 6:53 PM

I don't use directional shortcuts for switching between windows, but in general I like your idea very much and if Thomas gives his ok, it would improve the user experience in usecases like your one. You described it very well.

Regarding your code: What's the reason you use the bool tryOpposite parameter in switchWindow in the first place? As far as I can see the program now works always as follows:

Useraction calls switchWindow(direction), which in return calls switchWindow(direction, true), i.e. tryOpposite is always true, which makes it unnecessary (and the later conditional statement in line 1650 aswell). Correct me, if I'm wrong.

In D3602#67538, @subdiff wrote:

I don't use directional shortcuts for switching between windows, but in general I like your idea very much and if Thomas gives his ok, it would improve the user experience in usecases like your one. You described it very well.

Thanks!

Regarding your code: What's the reason you use the bool tryOpposite parameter in switchWindow in the first place? As far as I can see the program now works always as follows:

Useraction calls switchWindow(direction), which in return calls switchWindow(direction, true), i.e. tryOpposite is always true, which makes it unnecessary (and the later conditional statement in line 1650 aswell). Correct me, if I'm wrong.

The reason I put tryOpposite in switchWindows was because I didn't know C++ had default arguments. My bad. This is my very first attempt to code C++. However, in switchOrCycleWindow, tryOpposite is used in line 1650 to decide if I should recurse in the method to try to find a window in the opposite direction. In other words, when no window is found in the direction the user asked in the first place and tryOpposite is true, I calculate the opposite position (1579) and I recurse-call switchOrCycleWindow with tryOpposite set to false (1650) in order to halt the processing if no other candidate window exist. Is this explanation clear?

In order to fix the boolean trap I made with tryOpposite, what should I do? Create a enum Opposite { TRY, HALT }? In other contexts, I would use some kind of design pattern or make some methods high-order functions.

This is my very first attempt to code C++.

Awesome! For a first time C++ developer that's a really good change.

However, in switchOrCycleWindow, tryOpposite is used in line 1650 to decide if I should recurse in the method to try to find a window in the opposite direction.

What Roman wants to say is that tryOpposite is always true. There is only one call to switchWindow with the tryOpposite arg and that's always true.

romangg added a comment.EditedDec 6 2016, 8:25 PM

The reason I put tryOpposite in switchWindows was because I didn't know C++ had default arguments. My bad. This is my very first attempt to code C++. However, in switchOrCycleWindow, tryOpposite is used in line 1650 to decide if I should recurse in the method to try to find a window in the opposite direction. In other words, when no window is found in the direction the user asked in the first place and tryOpposite is true, I calculate the opposite position (1579) and I recurse-call switchOrCycleWindow with tryOpposite set to false (1650) in order to halt the processing if no other candidate window exist. Is this explanation clear?

But it's not really necessary in switchWindow, right? I mean you just always call in switchWindow(direction, bool) in switchOrCycleWindow(c, direction, curPos, desktopNumber, tryOpposite) and never come back to switchWindow in the recursion. Instead don't overwrite switchWindow and just make the call switchOrCycleWindow(c, direction, curPos, desktopNumber, true) at the end.

In order to fix the boolean trap I made with tryOpposite, what should I do? Create a enum Opposite { TRY, HALT }? In other contexts, I would use some kind of design pattern or make some methods high-order functions.

Not sure what you mean by design pattern, but in general you can use enum class instead of bool in such cases. But since in the end you don't need the boolean as an argument in switchWindow and only need it as the fith argument of switchOrCycleWindow, which is moreover only a helper function, I think in this case using a boolean is fine as long as Martin agrees.

EDIT: You're right though, that the conditional statement in line 1650 is not trivial in the recursion.

Featurewise, there's iirc a config item to wrap around virtual desktops - maybe this should be invoked here? (Although that link is currently not made clear by the config dialog)

useractions.cpp
1579

or a closure or dropped altogether - afaics it's only called once and "opposite position" (opposite to what...) is a rather meaningless name as well.

1651

you could turn the payload of the function into a closure (Martin locves lambdas ;-) and call that with the original (and if required) flipflopped point.
Right now, the bool trap is prone to cause an infinite recursion if you do bad things with the "tryOpposite" parameter (which isn't even const)

Martin's take, though.

workspace.h
506–508

is it actually ever used or "try opposite" used rather unconditionally?
(ie drop the parameter)

Featurewise, there's iirc a config item to wrap around virtual desktops - maybe this should be invoked here? (Although that link is currently not made clear by the config dialog)

It would need a seperate config option probably, since it's about two different objects and work flows. In this case, although an option to deactivate it would be nice to have of course, I would try to ignore it for now, since it's @pedroarthurp's first contribution and saving/loading config options would complicate it much more.

Also this way we could wait on user feedback, if a deactivating option is really needed. For example the task switcher also cycles through all available windows and doesn't have an option to deactivate round trips.

pedroarthurp removed R108 KWin as the repository for this revision.

Changes the design to remove the recursion and the tryOpposite boolean flag.

pedroarthurp set the repository for this revision to R108 KWin.Dec 14 2016, 4:33 PM
pedroarthurp marked 4 inline comments as done.Dec 14 2016, 4:38 PM

Overall I would also love to see an autotest added for the new behavior

useractions.cpp
1576

given that it's only called from one method I think this should be turned into a lambda or static local method.

1580–1586

the reference points you are calculating might be outside the visible area.

1587–1589

please add a default case with Q_UNREACHABLE() and remove the return

pedroarthurp added inline comments.Dec 14 2016, 8:18 PM
useractions.cpp
1576

In terms of code (I'm new in C++), to turn this method in static local method, I must append a static keywork in the method declaration in workspace.h or there is something else I must do?

Regarding to turning it into a lambda, are you suggesting to inline the code using lambda syntax or to create a variable to hold its body? Given that this method is non-trivial, I would find it harder to understand its purpose if it was somehow anonymous.

1580–1586

There is no problem in this case. switchWindow will use it just as a reference to find the closest window to that point.

luebking added inline comments.Dec 14 2016, 8:39 PM
useractions.cpp
1576

a "static local" is a function w/o a declaration in the header (it's not part of any class) and a "static" keyword. The latter hides its visibility, so you'd simply put

static QPoint oppositePosition(.)
{
...
}

above the usage (that's important)

An anonymous closure makes no sense at all

(But you could really unroll the function, ie. move the code to where it's called. The function is absolutely trivial and flips the direction In doubt a comment should do.
Would save the unreachable handling as well ;-)

pedroarthurp added inline comments.Dec 20 2016, 5:21 PM
useractions.cpp
1576

Got it for the static function, but I run into the following errors while trying to follow your tip:

/home/pedroarthur/repositories/kde/kwin/useractions.cpp:1557:46: error: ‘Direction’ has not been declared
 static QPoint oppositePosition(QPoint point, Direction direction)
                                              ^~~~~~~~~

Among others. If I put workspace::Direction in the direction parameter, I get:

 /home/pedroarthur/repositories/kde/kwin/useractions.cpp: In function ‘QPoint KWin::oppositePosition(QPoint, KWin::Workspace::Direction)’:
/home/pedroarthur/repositories/kde/kwin/useractions.cpp:1557:57: error: ‘enum KWin::Workspace::Direction’ is private within this context
 static QPoint oppositePosition(QPoint point, Workspace::Direction direction)
                                                         ^~~~~~~~~
In file included from /home/pedroarthur/repositories/kde/kwin/useractions.cpp:38:0:
/home/pedroarthur/repositories/kde/kwin/workspace.h:499:10: note: declared private here
     enum Direction {
          ^~~~~~~~~                 ^~~~~~~~~~~~~

Any suggestion?

About the function, IMHO, it has a trivial implementation, but a obscure (non-trivial) purpose; it deserves a name. Also, I think that the callee is already big enough, so I wouldn't unroll it if I had my way.

graesslin added inline comments.Dec 20 2016, 5:44 PM
useractions.cpp
1576

Unrolling or using a lambda function would fix it. I don't like adding new methods to workspace as it tends to be a dumping ground. And that would be a method only called from one place. So even if the method is already looking in this case a lambda or unrolling might be the better option

pedroarthurp marked 2 inline comments as done.Dec 20 2016, 7:04 PM

Sorry for not replying earlier.
While this proposed behavior might indeed not be expected by everyone, this is not really a problem: Those who would not expect the wrap-around behavior would not expect anything to happen at all, and therefore not even press the shortcut if they are at the left-/rightmost window. If they do press it anyway (maybe by accident), they might be surprised at first, but it will be easy to them to get back to where they were before, and then they can still decide whether or not they want to use the shortcut in such situations in the future.
So, long story short: The benefits clearly outweigh the risks here, so +1 for the patch from the usability side.

graesslin accepted this revision.Dec 22 2016, 6:43 PM
graesslin added a reviewer: graesslin.

Do you have commit rights or should I push the change for you?

This revision is now accepted and ready to land.Dec 22 2016, 6:43 PM

Do you have commit rights or should I push the change for you?

I think you should push. AFAIK, I have no commit rights.

This revision was automatically updated to reflect the committed changes.

@colomar

While this proposed behavior might indeed not be expected by everyone, this is not really a problem: Those who would not expect the wrap-around behavior would not expect anything to happen at all, and therefore not even press the shortcut if they are at the left-/rightmost window. If they do press it anyway (maybe by accident), they might be surprised at first, but it will be easy to them to get back to where they were before, and then they can still decide whether or not they want to use the shortcut in such situations in the future.

I disagree with this reasoning. See my feature request -> bug report at https://bugs.kde.org/show_bug.cgi?id=376457 where I was told that this is a better place to raise this.

To summarize, the key argument that and therefore not even press the shortcut if they are at the left-/rightmost window. is omitting the fact that this can happen with keyboard auto-repeating. This used to be a shortcut that can be used to shift the focus to one of the windows on the left/right/top/bottom by holding the key down but it cannot be used like that anymore.

While then they can still decide whether or not they want to use the shortcut in such situations in the future. is technically true and although I have to decide to not use it in this situation, I cannot get used to this behavior after one week of using it whereas other behavior changes usually take less than a day to get used to in my experience.

I don't think the above are strong enough to make a change if the new behavior has always been there but I think they are enough to make the new behavior not better than the old one and therefore the change shouldn't be made and should be reverted.