GNOME Bugzilla – Bug 683305
Be able to raise the lock screen shield with bumping
Last modified: 2013-05-15 20:48:06 UTC
Just something I thought of last night. I'd like to get designer approval or thoughts. If we can't, I think the scroll wheel part is a nice addition.
Created attachment 223391 [details] [review] screenShield: Fix typo We aren't French, no matter how much we may want to be.
Created attachment 223392 [details] [review] screenShield: Add a bump threshold, and animate scrolling Allow people to raise the dialog by repeatedly mashing keys, and add a nice scroll animation too.
Review of attachment 223391 [details] [review]: The code looks fine the commit message ....
Review of attachment 223392 [details] [review]: Some comments inline for the implementation. I'm not sure of the design. Did you talk with Allan, Jakub or Jon? ::: js/ui/screenShield.js @@ +456,3 @@ delta = Math.max(0, event.get_scroll_delta()[0]); + this._bumpLockScreen(delta); If I read the code correctly, you now need 25 * 4 / 2 = 50 "standard" scrolls (or 100 px of smooth scrolling) to get an unlock, over the previous 7 scrolls / 35 px. Also, scrolls now bump the screen and they don't cumulate after the bumping ends. @@ +491,2 @@ _onDragEnd: function(action, actor, eventX, eventY, modifiers) { + if (this._lockScreenGroup.y <= -RAISE_THRESHOLD) { This makes the heuristics independent on resolution. I'm not sure this is a good idea, especially as 100px on a 1024x600 is 14 %, while 100px on a 2880x1800 is just 5.5 %. @@ +580,3 @@ + transition: 'easeInQuad', + onComplete: Lang.bind(this, function() { + delete this._lockScreenGroupY; I'd rather have this._lockScreenGroupY = undefined, to avoid deoptimizing the object to an hash table.
(In reply to comment #4) > Review of attachment 223392 [details] [review]: > > Some comments inline for the implementation. > I'm not sure of the design. Did you talk with Allan, Jakub or Jon? > > ::: js/ui/screenShield.js > @@ +456,3 @@ > delta = Math.max(0, event.get_scroll_delta()[0]); > > + this._bumpLockScreen(delta); > > If I read the code correctly, you now need 25 * 4 / 2 = 50 "standard" scrolls > (or 100 px of smooth scrolling) to get an unlock, over the previous 7 scrolls / > 35 px. I've found it very natural with my touchpad; I haven't tried it with my mouse. > Also, scrolls now bump the screen and they don't cumulate after the bumping > ends. > > @@ +491,2 @@ > _onDragEnd: function(action, actor, eventX, eventY, modifiers) { > + if (this._lockScreenGroup.y <= -RAISE_THRESHOLD) { > > This makes the heuristics independent on resolution. I'm not sure this is a > good idea, especially as 100px on a 1024x600 is 14 %, while 100px on a > 2880x1800 is just 5.5 %. Do we want to make BUMP_SIZE proportional too, then? > @@ +580,3 @@ > + transition: 'easeInQuad', > + onComplete: Lang.bind(this, function() { > + delete this._lockScreenGroupY; > > I'd rather have this._lockScreenGroupY = undefined, to avoid deoptimizing the > object to an hash table.
(In reply to comment #4) > Review of attachment 223392 [details] [review]: > > Some comments inline for the implementation. > I'm not sure of the design. Did you talk with Allan, Jakub or Jon? Oh, and no. I was going to, but I forgot.
Comment on attachment 223391 [details] [review] screenShield: Fix typo Attachment 223391 [details] pushed as b37e02c - screenShield: Fix typo
Created attachment 224258 [details] [review] screenShield: Add a bump threshold, and animate scrolling Allow people to raise the dialog by repeatedly mashing keys, and add a nice scroll animation too.
We no longer bump the screen shield, let's close this.