D14988: Implement option to toggle page navigation wraps around for pager plasmoid

hein added a comment. @ngraham Let's progress this, do you want @phuongn to change the patch? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn, #plasma, #vdg, davidedmundson Cc: hein, zzag, ngraham, abetts, davidedmundson, broulik,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

This revision was automatically updated to reflect the committed changes. Closed by commit R119:a2b683a10802: Implement option to toggle page navigation wraps around for pager plasmoid (authored by phuongn, committed by davidedmundson). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

ngraham added a comment. 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

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn added a comment. 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

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

hein added a comment. And @phuongn, I'm guessing you set all of these options to disabling wrap-around, right? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn, #plasma, #vdg, davidedmundson Cc: hein, zzag, ngraham, abetts, davidedmundson,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

hein added a comment. 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? REPOSITORY R119 Plasma Desktop REVISION DETAIL

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn updated this revision to Diff 40618. phuongn added a comment. Correct the erroneous logic for determining the next and previous desktop index REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14988?vs=40549=40618 REVISION DETAIL

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

zzag added inline comments. INLINE COMMENTS > zzag wrote in main.qml:130 > I think it should be `Math.min`. Also, maybe `repeater.count - 1`. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn, #plasma, #vdg, davidedmundson Cc: zzag, ngraham,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

zzag added inline comments. INLINE COMMENTS > main.qml:130 > +(pagerModel.currentPage + 1) % repeater.count : > +Math.max(pagerModel.currentPage + 1, repeater.count); > +pagerModel.changePage(nextPage); I think it should be `Math.min`. >

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

abetts added a comment. In D14988#316672 , @ngraham wrote: > I think the terminology "wraps around" is fine. Seems pretty clear to me. Cool, let's go with that. REPOSITORY R119 Plasma Desktop REVISION DETAIL

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

ngraham added a comment. I think the terminology "wraps around" is fine. Seems pretty clear to me. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn, #plasma, #vdg, davidedmundson Cc: ngraham, abetts, davidedmundson, broulik, plasma-devel,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn added a comment. F6220957: image.png 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

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

abetts added a comment. In D14988#316633 , @phuongn wrote: > 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

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn added a comment. 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 F6220920: simplescreenrecorder-2018-08-28_17.44.42.mp4

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

abetts added a comment. Could you please put a video or gif that shows the effect? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn, #plasma, #vdg, davidedmundson Cc: abetts, davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn updated this revision to Diff 40549. phuongn marked an inline comment as done. phuongn added a comment. Utilize Math methods (min, max) to make the code more readable REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14988?vs=40188=40549

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn marked an inline comment as done. phuongn added inline comments. INLINE COMMENTS > davidedmundson wrote in main.qml:130 > would > > var nextPage = plasmoid.configuration.wrapPage? > (pagerModel.currentPage + 1) % repeater.count : >

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

davidedmundson added inline comments. INLINE COMMENTS > main.qml:130 > +(pagerModel.currentPage + 1) % repeater.count : > pagerModel.currentPage + 1; > +if (nextPage >= repeater.count) nextPage = repeater.count - > 1; > +

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

broulik added a comment. 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. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

broulik added a comment. Shouldn't this just follow KWin's "screens wrap around" policy? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D14988 To: phuongn, #plasma Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg,

D14988: Implement option to toggle page navigation wraps around for pager plasmoid

phuongn created this revision. phuongn added a project: Plasma. Herald added a subscriber: plasma-devel. phuongn requested review of this revision. REVISION SUMMARY BUG: 361672 This patch implement an option to disable navigation wraps around for the pager plasmoid, which potentially