After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 689062 - Remove custom overview swipe scrolling code
Remove custom overview swipe scrolling code
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 689061
Blocks:
 
 
Reported: 2012-11-26 05:01 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2012-11-30 23:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
viewSelector: Set the active page immediately (1.39 KB, patch)
2012-11-26 05:01 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
searchDisplay, appDisplay: Use ClutterPanAction for re-swipe scrolling (3.42 KB, patch)
2012-11-26 05:01 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
workspacesView: Clean up scroll code a bit (2.58 KB, patch)
2012-11-26 05:01 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
workspacesView: Don't use the overview swipe scrolling system (5.85 KB, patch)
2012-11-26 05:01 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
overview: Remove custom swipe scrolling implementation (8.98 KB, patch)
2012-11-26 05:02 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
workspacesView: Don't use the overview swipe scrolling system (6.01 KB, patch)
2012-11-29 23:16 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
searchDisplay, appDisplay: Use ClutterPanAction for re-swipe scrolling (4.66 KB, patch)
2012-11-29 23:16 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-11-26 05:01:48 UTC
Nowadays, Clutter has all the API we need to do nifty swipe scrolling.
This also makes the experience smoother for the app/search lists, rather
than bouncing back to arbitrary pages when releasing the pointer, and
also adds kinetic scrolling.
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-11-26 05:01:50 UTC
Created attachment 229876 [details] [review]
viewSelector: Set the active page immediately

This prevents some focus management issues.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-11-26 05:01:53 UTC
Created attachment 229877 [details] [review]
searchDisplay, appDisplay: Use ClutterPanAction for re-swipe scrolling

The sooner we can kill off the custom overview swipe scroll code, the
better.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-11-26 05:01:56 UTC
Created attachment 229878 [details] [review]
workspacesView: Clean up scroll code a bit

There's no code path that results in us not animating.
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-11-26 05:01:59 UTC
Created attachment 229879 [details] [review]
workspacesView: Don't use the overview swipe scrolling system

Switch to a ClutterPanAction instead too.
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-11-26 05:02:02 UTC
Created attachment 229880 [details] [review]
overview: Remove custom swipe scrolling implementation
Comment 6 Cosimo Cecchi 2012-11-29 21:58:50 UTC
Review of attachment 229876 [details] [review]:

Which issues specifically does this fix?

::: js/ui/viewSelector.js
@@ +215,1 @@
             Tweener.addTween(this._activePage,

Change this._activePage -> oldPage here too (so we don't miss it if some day we move around the oldPage/this._activePage assignments)
Comment 7 Cosimo Cecchi 2012-11-29 22:07:52 UTC
Review of attachment 229877 [details] [review]:

::: js/ui/searchDisplay.js
@@ +216,3 @@
+    _onPan: function(action) {
+        let [dx, dy] = action.get_motion_delta(0);
+        let adjustment = this.actor.vscroll.adjustment;

Don't you need the scrollView's adjustment here? this.actor is a StBoxLayout.
Comment 8 Cosimo Cecchi 2012-11-29 22:10:09 UTC
Review of attachment 229878 [details] [review]:

Looks good.
Comment 9 Cosimo Cecchi 2012-11-29 22:14:34 UTC
Review of attachment 229879 [details] [review]:

::: js/ui/workspacesView.js
@@ +454,3 @@
+        Main.overview.addAction(action);
+        this.actor.connect('notify::mapped', Lang.bind(this, function() {
+        }));

Can you use g_object_bind_property()?
Comment 10 Cosimo Cecchi 2012-11-29 22:15:33 UTC
Review of attachment 229880 [details] [review]:

Looks good to me.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-11-29 23:16:03 UTC
(In reply to comment #6)
> Review of attachment 229876 [details] [review]:
> 
> Which issues specifically does this fix?

Hmm, I tried to reproduce it and the issue is now gone. Let's drop this patch for now.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-11-29 23:16:37 UTC
Created attachment 230252 [details] [review]
workspacesView: Don't use the overview swipe scrolling system

Switch to a ClutterPanAction instead too.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-11-29 23:16:54 UTC
Created attachment 230253 [details] [review]
searchDisplay, appDisplay: Use ClutterPanAction for re-swipe scrolling

The sooner we can kill off the custom overview swipe scroll code, the
better.
Comment 14 Cosimo Cecchi 2012-11-30 21:01:50 UTC
Review of attachment 230252 [details] [review]:

This looks fine to me now.
Comment 15 Cosimo Cecchi 2012-11-30 21:02:29 UTC
Review of attachment 230253 [details] [review]:

++
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-11-30 23:04:08 UTC
Attachment 229878 [details] pushed as 57d9e7d - workspacesView: Clean up scroll code a bit
Attachment 229880 [details] pushed as 98f4b99 - overview: Remove custom swipe scrolling implementation
Attachment 230252 [details] pushed as 830e701 - workspacesView: Don't use the overview swipe scrolling system
Attachment 230253 [details] pushed as 5fc16bb - searchDisplay, appDisplay: Use ClutterPanAction for re-swipe scrolling