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 655746 - Allow disabling window animations
Allow disabling window animations
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 685452 (view as bug list)
Depends on:
Blocks: 680195 fallback
 
 
Reported: 2011-08-01 18:54 UTC by lzap
Modified: 2013-02-15 01:48 UTC
See Also:
GNOME target: 3.8
GNOME version: Unversioned Enhancement


Attachments
Adding disable-window-animations ext. (BZ 655746) (4.55 KB, patch)
2011-08-01 18:59 UTC, lzap
none Details | Review
windowManager: respect animations settings (1.30 KB, patch)
2011-08-16 14:06 UTC, Giovanni Campagna
needs-work Details | Review
endSessionDialog: don't use Tweener for the timer (2.51 KB, patch)
2013-01-26 08:53 UTC, Cosimo Cecchi
reviewed Details | Review
tweener: follow org.gnome.desktop.interface enable-animations (1.51 KB, patch)
2013-01-26 08:53 UTC, Cosimo Cecchi
reviewed Details | Review
endSessionDialog: don't use Tweener for the timer (2.76 KB, patch)
2013-02-09 00:28 UTC, Cosimo Cecchi
committed Details | Review
tweener: follow org.gnome.desktop.interface enable-animations (1.51 KB, patch)
2013-02-09 00:30 UTC, Cosimo Cecchi
committed Details | Review

Description lzap 2011-08-01 18:54:38 UTC
Include disable-window-animations in gnome-shell-extensions.

https://github.com/lzap/disable-window-animations
Comment 1 lzap 2011-08-01 18:59:32 UTC
Created attachment 193001 [details] [review]
Adding disable-window-animations ext. (BZ 655746)

#	modified:   README
#	modified:   configure.ac
#	new file:   extensions/disable-window-animations/Makefile.am
#	new file:   extensions/disable-window-animations/extension.js
#	new file:   extensions/disable-window-animations/metadata.json.in
#	new file:   extensions/disable-window-animations/stylesheet.css
Comment 2 Giovanni Campagna 2011-08-02 10:52:46 UTC
There is already a GSettings key called org.gnome.desktop.interface.enable-animations, and gnome-shell core should be patched to look at it.
Comment 3 lzap 2011-08-02 11:02:47 UTC
Ouch :-)

So you say Gnome Shell will eventually read this setting. Feel free to close it then. From what version is it working?
Comment 4 Giovanni Campagna 2011-08-02 12:03:50 UTC
No, I said that gnome-shell *should* look at this. Code is missing, at the moment.
Comment 5 Giovanni Campagna 2011-08-16 14:06:59 UTC
Created attachment 193949 [details] [review]
windowManager: respect animations settings

The org.gnome.desktop.interface schema has a setting for enabling/
disabling animations in the whole desktop. We should respect that
when animating window operations.
Comment 6 lzap 2011-10-20 08:57:20 UTC
You patch works for me in 3.0 (Fedora 15).
Comment 7 lzap 2011-10-20 08:58:22 UTC
Review of attachment 193949 [details] [review]:

Tested on Gnome 3.0 from Fedora 15. I could test it against F16 (Gnome 3.2) in few weeks as well if needed.
Comment 8 Giovanni Campagna 2011-10-20 13:13:27 UTC
(In reply to comment #7)
> Review of attachment 193949 [details] [review]:
> 
> Tested on Gnome 3.0 from Fedora 15. I could test it against F16 (Gnome 3.2) in
> few weeks as well if needed.

I'll wait for a review from a member of the shell team before pushing.
Comment 9 Jasper St. Pierre (not reading bugmail) 2011-10-20 14:56:11 UTC
Review of attachment 193949 [details] [review]:

Makes sense to me.
Comment 10 Florian Müllner 2011-10-20 15:06:48 UTC
Comment on attachment 193949 [details] [review]
windowManager: respect animations settings

Can we please wait for some designer input before pushing this?

For the record, it does not make much sense to me that the key disables some random animations (mapping, minimizing, ...), but not others (in particular the overview transition, which is very much a "windows animation")
Comment 11 Owen Taylor 2012-02-15 21:15:49 UTC
Review of attachment 193949 [details] [review]:

Hmm, I'm not sure we need designer feedback, because afaik, the key is no-where exposed in configuration. Still, it seems to me that we should try to do this at the tweener level, so that we aren't always fighting some animation that leaks out. The way I'd imagine it would work is that at the first ticker callback, any active tweens would be set to complete and properties set to their final value. Like the effect of St.set_slow_down_factor(<very small number>). Trying that, it's sort of jarring in some cases - message tray, workspace thumbnail slide ins, but I think it's a whole lot of work to define "good animations" vs. "bad animations", for something that's just hidden in a non-exposed GSetting.
Comment 12 Ray Strode [halfline] 2012-04-12 17:03:24 UTC
we did GNOME_SHELL_SLOWDOWN_FACTOR=0.001 on Matthias's slow software rendered machine, and it definitely improved the user experience in that case.

One side effect, though, was the logout timer abuses tweener, and no longer waited 60 seconds.
Comment 13 Matthias Clasen 2012-11-07 12:02:01 UTC
so, where does this stand ? 

do we need a patch to stop tweener abuses, and another one to set a slowdown factor in response to disable-animations ?
Comment 14 Matthias Clasen 2012-11-07 12:04:15 UTC
I'm asking, because we've identified a reduced resources mode as one of the tasks for dropping fallback mode.
Comment 16 Florian Müllner 2012-11-07 12:28:39 UTC
(In reply to comment #13)
> so, where does this stand ? 
> 
> do we need a patch to stop tweener abuses, and another one to set a slowdown
> factor in response to disable-animations ?

That sounds realistic, yes. Alternatively we could properly hook into tweener instead of re-using the slowdown factor for the second part, but we'd still need stopping tweener for non-animations.
Comment 17 Cosimo Cecchi 2013-01-26 08:53:49 UTC
Created attachment 234460 [details] [review]
endSessionDialog: don't use Tweener for the timer

We want to make Tweener short-circuit animations when resources are
constrained, so this is not going to work.
Instead, use a one-second timeout until the seconds left reach zero.
Comment 18 Cosimo Cecchi 2013-01-26 08:53:54 UTC
Created attachment 234461 [details] [review]
tweener: follow org.gnome.desktop.interface enable-animations

Disable the animations by making the animation time a very small value.
Comment 19 Ray Strode [halfline] 2013-01-30 18:53:49 UTC
Review of attachment 234460 [details] [review]:

::: js/ui/endSessionDialog.js
@@ +413,3 @@
+        this._timerId = Mainloop.timeout_add_seconds(1, Lang.bind(this,
+            function() {
+                this._secondsLeft -= 1;

It would be better to read current time and subtract initial time to calculate seconds left.
Comment 20 Ray Strode [halfline] 2013-01-30 19:03:06 UTC
Review of attachment 234461 [details] [review]:

::: js/ui/tweener.js
@@ +51,3 @@
 function init() {
     Tweener.setFrameTicker(new ClutterFrameTicker());
+    animationSettings = new Gio.Settings({ schema: 'org.gnome.desktop.interface' });

should watch for changes

@@ +79,3 @@
 
+    if (!animationSettings.get_boolean('enable-animations'))
+        tweeningParameters['time'] /= 1000000;

two thoughts:

1) this could just be an assignment instead of a division right? 
2) why not call Tweener.setTimeScale (0.0000001) or so at start time like the slow down factor works?
Comment 21 Florian Müllner 2013-01-30 19:48:41 UTC
(In reply to comment #20)
>      Tweener.setFrameTicker(new ClutterFrameTicker());
> +    animationSettings = new Gio.Settings({ schema:
> 'org.gnome.desktop.interface' });
> 
> should watch for changes

Should it? Any animation time that is big enough to justify cancellation on settings changes is probably an abuse of the tweener API ...
Comment 22 Matthias Clasen 2013-01-31 19:14:44 UTC
I think not listening for changes during an animation is fine, as long as we pick up changes between animations.
Comment 23 Ray Strode [halfline] 2013-01-31 20:05:24 UTC
(In reply to comment #21)
> (In reply to comment #20)
> >      Tweener.setFrameTicker(new ClutterFrameTicker());
> > +    animationSettings = new Gio.Settings({ schema:
> > 'org.gnome.desktop.interface' });
> > 
> > should watch for changes
> 
> Should it? Any animation time that is big enough to justify cancellation on
> settings changes is probably an abuse of the tweener API ...

Indeed you're right, just querying it in _wrapTweening like he does is fine.
Comment 24 Bastien Nocera 2013-01-31 21:47:27 UTC
*** Bug 685452 has been marked as a duplicate of this bug. ***
Comment 25 Matthias Clasen 2013-02-09 00:01:09 UTC
would be cool to produce an updated patch and get this out of the way
Comment 26 Cosimo Cecchi 2013-02-09 00:28:03 UTC
Created attachment 235563 [details] [review]
endSessionDialog: don't use Tweener for the timer

--

Updated as for Ray's review to calculate seconds left from the monotonic time.
Comment 27 Cosimo Cecchi 2013-02-09 00:30:08 UTC
Created attachment 235564 [details] [review]
tweener: follow org.gnome.desktop.interface enable-animations

--

The reason I did it in tweener was really to avoid watching for changes, since GSettings reads should be cheap. I modified this to an assignment now.
Comment 28 Jasper St. Pierre (not reading bugmail) 2013-02-09 00:39:22 UTC
Review of attachment 235563 [details] [review]:

OK.
Comment 29 Jasper St. Pierre (not reading bugmail) 2013-02-09 00:39:46 UTC
Review of attachment 235563 [details] [review]:

Actually, I wonder if it makes sense to use something like WallClockSource from GnomeDesktop.
Comment 30 Cosimo Cecchi 2013-02-11 17:13:15 UTC
(In reply to comment #29)
> Review of attachment 235563 [details] [review]:
> 
> Actually, I wonder if it makes sense to use something like WallClockSource from
> GnomeDesktop.

WallClockSource doesn't seem to be public (and GnomeWallClock doesn't really seem like a good fit to me, we want to follow monotonic time, no?).
Since we want to update the seconds left string, I really think we need a timeout here.
Comment 31 Jasper St. Pierre (not reading bugmail) 2013-02-11 17:32:05 UTC
timerfd allows you to create a monotonic source, but maybe this is good enough.
Comment 32 Matthias Clasen 2013-02-11 19:22:04 UTC
I think it is good enough. This is not about high-precision measurements, but about giving the user some sense of a ticking clock...
Comment 33 Jasper St. Pierre (not reading bugmail) 2013-02-12 21:35:45 UTC
Review of attachment 235563 [details] [review]:

Yeah, I just realized that. If they're logging out, they also probably don't care about wakeups too much.
Comment 34 Jasper St. Pierre (not reading bugmail) 2013-02-12 21:38:23 UTC
Review of attachment 235564 [details] [review]:

Why can't we set it to 0?

::: js/ui/tweener.js
@@ +79,3 @@
 
+    if (!animationSettings.get_boolean('enable-animations'))
+        tweeningParameters['time'] = 0.000001;

undefined
Comment 35 Jasper St. Pierre (not reading bugmail) 2013-02-12 21:39:16 UTC
(In reply to comment #34)
> ::: js/ui/tweener.js
> @@ +79,3 @@
> 
> +    if (!animationSettings.get_boolean('enable-animations'))
> +        tweeningParameters['time'] = 0.000001;
> 
> undefined

Ignore this part. It seems that Splinter broke.
Comment 36 Cosimo Cecchi 2013-02-13 14:23:51 UTC
(In reply to comment #34)
> Review of attachment 235564 [details] [review]:
> 
> Why can't we set it to 0?

Other things will break if we just set it to zero (I don't remember exactly what part without trying again though).
Comment 37 Jasper St. Pierre (not reading bugmail) 2013-02-13 15:31:23 UTC
(In reply to comment #36)
> Other things will break if we just set it to zero (I don't remember exactly
> what part without trying again though).

We should look into that. Tweener has a fast-path if you have 0 time to end the tween at its final value and call onComplete, so maybe onUpdate handlers not  getting called breaks something?
Comment 38 Cosimo Cecchi 2013-02-13 17:04:39 UTC
(In reply to comment #37)

> We should look into that. Tweener has a fast-path if you have 0 time to end the
> tween at its final value and call onComplete, so maybe onUpdate handlers not 
> getting called breaks something?

It's not just that; we have code that assumes Tweener will give back control to the mainloop before calling onComplete, which it doesn't when the time is zero.

So, code like this will suddenly break.

Tweener.addTween(foo, { time: 0.30,
                        onComplete: Lang.bind(this, this._foo) });
this._barExpectedToRunBeforeFoo();

While it might be a good idea, I think it's too late to fix all the code with such assumptions for this cycle.
Comment 39 Jasper St. Pierre (not reading bugmail) 2013-02-14 18:21:24 UTC
Review of attachment 235564 [details] [review]:

OK, let's roll with it.

::: js/ui/tweener.js
@@ +79,3 @@
 
+    if (!animationSettings.get_boolean('enable-animations'))
+        tweeningParameters['time'] = 0.000001;

I wonder if this follow the GTK+ XSetting "gtk-enable-animations" instead. I don't mind either way.
Comment 40 Matthias Clasen 2013-02-14 19:59:52 UTC
org.gnome.desktop.interface enable-animations is what backs the gtk-enable-animations xsetting
Comment 41 Jasper St. Pierre (not reading bugmail) 2013-02-14 20:07:46 UTC
Yes, so this would simply add the ability to override with ~/.config/gtk-3.0/settings.ini or with gnome-settings-daemon xsettings overrides

Whether that's wanted or not, I don't know.
Comment 42 Cosimo Cecchi 2013-02-15 01:47:56 UTC
Attachment 235563 [details] pushed as 1c57c0e - endSessionDialog: don't use Tweener for the timer
Attachment 235564 [details] pushed as 5f95351 - tweener: follow org.gnome.desktop.interface enable-animations

Thanks for the review.
The gnome-settings-daemon plugin sets the GSetting value, so I think it's OK for us to do the same.