Created attachment 215599 [details] [review]
modalDialog: Remove the fade in buttons code

Due to a typo, it never worked correctly. After a fix-up to the appropriate
method, the behavior is suboptimal, as the buttons only fade in the first time
the modal dialog is constructed. Just remove the fade-in behavior, rather than
keeping this non-working code around.

Created attachment 215600 [details] [review]
js: Remove unnecessary versions of clutter_actor_get_children

clutter_actor_get_children requires making a temporary GSList from
a linked list structure, and then creating a JS Array from that GSList.
For simple cases like the number of children, use clutter_actor_get_n_children.

Review of attachment 215600 [details] [review]:

Makes sense.

::: js/ui/endSessionDialog.js
@@ -33,2 @@
 const GnomeSession = imports.misc.gnomeSession;
-const Lightbox = imports.ui.lightbox;

Unrelated?

::: js/ui/lookingGlass.js
@@ +39,3 @@
                     'const color = function(pixel) { let c= new Clutter.Color(); c.from_pixel(pixel); return c; }; ' +
                     /* Special lookingGlass functions */
+                    'const it = Main.lookingGlass.getIt(); ' +

Also unrelated?

I just decided to clean those up while I was at it and hope nobody would notice/care. It just seemed silly to put those in their own patch.

(In reply to comment #1)
> Created an attachment (id=215599) [details] [review]
> modalDialog: Remove the fade in buttons code
> 
> Due to a typo, it never worked correctly. After a fix-up to the appropriate
> method, the behavior is suboptimal, as the buttons only fade in the first time
> the modal dialog is constructed. Just remove the fade-in behavior, rather than
> keeping this non-working code around.

The question is do we want to just remove it?
Or rather fix it to work correctly?

Needs design input on that.

Comment on attachment 215600 [details] [review]
js: Remove unnecessary versions of clutter_actor_get_children

Attachment 215600 [details] pushed as c929619 - js: Remove unnecessary versions of clutter_actor_get_children

(In reply to comment #5)
> The question is do we want to just remove it?
> Or rather fix it to work correctly?
> 
> Needs design input on that.

I think we should just remove it.

Had some issues with git-bz, but the modal dialog thing has been pushed:

<magcius> jimmac, ping
<magcius> jimmac, right now we have code in place to fade in the buttons of a modal dialog, but it had a bug and never worked. When fixed, it looks really weird for the modal dialog to appear instantly but the buttons to fade in. Should we remove the effect outright?
<magcius> jimmac, it's also strange as we sometimes keep certain modal dialogs around, like the end session dialog, so the buttons only fade in the first time
<jimmac> yea that sounds really odd
<magcius> jimmac, so just remove them?
<drago01> the later could be fixed though
<drago01> (the end session thing)
<jimmac> if I build git master, I can see this?
<magcius> jimmac, not right now, because the code that actually triggered the fade was broken.
<jimmac> what you are describing is that a modal dialog appears immediately and the buttons fade in?
<magcius> jimmac, yep. Content and everything appears immediately, buttons fade in.
<magcius> Let me see if I can record and upload a screencast.
<jimmac> yea that makes little sense
<mclasen> sounds odd
<magcius> I bet the code was never tested.
<magcius> jimmac, do you need a screencast or should I just remove it?
<jimmac> having a fast transition for the whole dialog might be nice, but applying a transition to the buttons only is wrong. Just remove it
<magcius> OK.

<halfline> wait
<halfline> we've had this dicussion before
<halfline> IIRC there was one unexpected surprise
<magcius> Too late.
<halfline> uh
<halfline> itchy trigger finger
<jimmac> heh
<jimmac> halfline, go on
<halfline> so iirc the bug in the code is something like
<halfline> hasButtons = this.buttonBox.get_children() > 0
<magcius> get_children() > 0;
<magcius> Yep.
<halfline> instead of get_n_children()
<halfline> but javascript is weird
<halfline> [1, 2, 3] > 0 is false
<halfline> so hadButtons is always false
<halfline> or something
<magcius> Yep.
<halfline> let me pull open the code
<halfline> magcius: last time we talked about this, the opposite thing was happening from what you thought
<magcius> halfline, hm?
<halfline> right so here's the code
<halfline>         let hadChildren = this._buttonLayout.get_children() > 0;•
<halfline>         if (!hadChildren && buttons.length > 0) {•
<halfline>  /* do the animation */
<halfline> }
<halfline> hadChildren is always FALSE
<halfline> if there are children
<halfline> so we're always doing the fade
<halfline> not never doing the fade
<magcius> I'd have to dig into it again.
<halfline> please do
<halfline> FWIW, i think any time there's a "pop" from one widget state to the next that's a bug
<halfline> definitely the dialog should fade in as one cohesive unit initially
<halfline> but when buttons are changed after the dialog is mapped, we need to have a transition
<magcius> halfline, are you going to fix the login dialog code for 3.8 so that it's a simple fade, too?
<magcius> That would be convenient.
<halfline> yea
<halfline> i started working on that a few days ago but got distracted
<magcius> halfline, so the thing is that we faded in buttons, but I guess nobody saw since it was such a small animation
<halfline> so i guess normally when the buttons change the dialog content changes too
<halfline> and anytime the two are changing at the same time, we should change them together
<halfline> So I guess i'm now of the opinion removing the one off transition is right, and we should fix all the callers to do a full scale transition
<halfline> i'll take care of it when i redo the login screen transitions