Implement option to toggle page navigation wraps around for pager plasmoid
ClosedPublic

Authored by phuongn on Aug 22 2018, 8:01 AM.

Details

Summary

BUG: 361672

This patch implement an option to disable navigation wraps around for the pager plasmoid, which potentially cause inconveniences for user who switch desktop using mouse wheel or touchpad scroll.

The patch is implemented since "Desktop navigation wraps around" option in "System Settings -> Desktop Behavior -> Virtual Desktops -> Switching" only affects keyboard shortcut mapping.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
phuongn created this revision.Aug 22 2018, 8:01 AM
Restricted Application added a subscriber: plasma-devel. ยท View Herald TranscriptAug 22 2018, 8:01 AM
phuongn requested review of this revision.Aug 22 2018, 8:01 AM
phuongn added a reviewer: Plasma.

Shouldn't this just follow KWin's "screens wrap around" policy?

I couldn't find an easy way to access KWindowSystem config from there.
On the other hand, I personally prefer wrapping when using keyboard shortcut and no wrapping when using mouse or touchpad gesture. I figured some people might need the same flexibility.

You would need to read kwinrc manually, I suppose.
I don't use virtual desktops, so I cannot comment, but I don't think three separate options is worthwhile.

broulik added a reviewer: VDG.Aug 22 2018, 8:18 AM
davidedmundson added inline comments.
applets/pager/package/contents/ui/main.qml
130

would

var nextPage = plasmoid.configuration.wrapPage?
                  (pagerModel.currentPage + 1) % repeater.count :
                   Math.max(pagerModel.currentPage + 1, repeater.count)

be more readable?

phuongn marked an inline comment as done.Aug 28 2018, 11:35 AM
phuongn added inline comments.
applets/pager/package/contents/ui/main.qml
130

Agree. Sorry I wasn't aware of Math functions in the context. Good tip ๐Ÿ‘

phuongn updated this revision to Diff 40549.Aug 28 2018, 11:44 AM
phuongn marked an inline comment as done.

Utilize Math methods (min, max) to make the code more readable

davidedmundson accepted this revision.Aug 28 2018, 11:47 AM
This revision is now accepted and ready to land.Aug 28 2018, 11:47 AM
abetts added a subscriber: abetts.Aug 28 2018, 2:24 PM

Could you please put a video or gif that shows the effect?

Hi, it's just the default desktop changing effect. Activated by scrolling with the cursor hovering on the pager. I haven't changed the effect, just added an option to toggle page wrapping

Hi, it's just the default desktop changing effect. Activated by scrolling with the cursor hovering on the pager. I haven't changed the effect, just added an option to toggle page wrapping

Thank you for the video. Just wanted to be sure I was understanding the patch. Looks good to me. I am not sure that the effect should be called "wrap around" since wrapping around evokes ideas about reaching a corner or a continuation of a sheet for example. What do you think of using a string that says something like:"

"Enable desktop pager scrolling"
"Enable desktop switch on pager"

Currently there is already "Desktop navigation wraps around" option for Virtual Desktops, which serve exact same purpose as this patch, but only for keyboard shortcut.
I used "Page navigation wraps around" because the pager can be used to control either Desktop or Activity, so I used "Page" instead of "Desktop" and keep the rest of the string similar to existing option from Virtual Desktops settings. Using other strings might confuse user of what this does.

I think the terminology "wraps around" is fine. Seems pretty clear to me.

I think the terminology "wraps around" is fine. Seems pretty clear to me.

Cool, let's go with that.

zzag added a subscriber: zzag.Aug 28 2018, 5:45 PM
zzag added inline comments.
applets/pager/package/contents/ui/main.qml
130

I think it should be Math.min.

135

Math.max(pagerModel.currentPage - 1, 0)

zzag added inline comments.Aug 28 2018, 5:47 PM
applets/pager/package/contents/ui/main.qml
130

Also, maybe repeater.count - 1.

phuongn updated this revision to Diff 40618.EditedAug 29 2018, 6:39 AM

Good spot! I was too careless there. The erroneous logic for determining the next and previous desktop index is now corrected.

phuongn marked 3 inline comments as done.Aug 29 2018, 6:41 AM
hein added a subscriber: hein.Sep 4 2018, 10:37 AM

This patch is good, but I share Kai's concern that it's perhaps suboptimal we have two-three checkboxes now to control similar behavior in different places. Does the VDG have an opinion on that @ngraham?

hein added a comment.Sep 4 2018, 10:38 AM

And @phuongn, I'm guessing you set all of these options to disabling wrap-around, right?

In D14988#320052, @hein wrote:

And @phuongn, I'm guessing you set all of these options to disabling wrap-around, right?

I wasn't the original author of the very first wrap-around option for KWin, which only work with keybindings.
But yes, the 2 patches that I submitted recently for review introduce 2 additional places to customize the behavior in separate cases.
As mentioned before, I couldn't find an easy way to access KWin config and also consider the need of personal preferences in separate cases. Would be nice to hear other opinions as well, is it be better to unify them all or keep them separate for flexibility?

My sense is that it would be best to put this option alongside the existing one in the virtual desktops KCM.

Navigation wraps around: [] When switching with keyboard shortcuts
                         [] When switching with a scroll

Or something like that. Just a rough idea.

This revision was automatically updated to reflect the committed changes.
hein added a comment.Nov 16 2018, 8:08 AM

@ngraham Let's progress this, do you want @phuongn to change the patch?