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 657082 - greeter support for gnome-shell
greeter support for gnome-shell
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 657077
 
 
Reported: 2011-08-22 15:04 UTC by Matthias Clasen
Modified: 2011-08-31 13:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st-box-layout: Document insertion apis (1.65 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
st-box-layout: Add methods for fetching sibling actors (3.31 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
popupMenu: Hide separators when they aren't separating (13.42 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
popupMenu: Use new convenience method for settings (9.71 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
StBoxLayout: Use adjustment value property, not getter (1.72 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
StAdjustment: Fix elastic property to be elastic (2.29 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
StAdjustment: Add alpha arg to interpolate method (2.86 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
StAdjustment: Document interpolate method (1.06 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
ModalDialog: Generalize and rename to Dialog (16.91 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
batch: Add mechanism for doing animation series (7.81 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
overview: Make optional (17.18 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
dateMenu: Force min-width of events area, not whole menu (2.09 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
dateMenu: Make events list optional (7.05 KB, patch)
2011-08-24 15:19 UTC, Ray Strode [halfline]
none Details | Review
global: Add concept of "session type" (6.94 KB, patch)
2011-08-24 15:20 UTC, Ray Strode [halfline]
none Details | Review
main: Factor out user session specific bits (11.10 KB, patch)
2011-08-24 15:20 UTC, Ray Strode [halfline]
none Details | Review
Add support for login session (43.78 KB, patch)
2011-08-24 15:20 UTC, Ray Strode [halfline]
none Details | Review
st-box-layout: Document insertion apis (1.65 KB, patch)
2011-08-26 04:00 UTC, Ray Strode [halfline]
committed Details | Review
st-box-layout: Add methods for fetching sibling actors (3.31 KB, patch)
2011-08-26 04:01 UTC, Ray Strode [halfline]
reviewed Details | Review
popupMenu: Hide separators when they aren't separating (13.42 KB, patch)
2011-08-26 04:01 UTC, Ray Strode [halfline]
reviewed Details | Review
popupMenu: Use new convenience method for settings (9.71 KB, patch)
2011-08-26 04:03 UTC, Ray Strode [halfline]
needs-work Details | Review
StBoxLayout: Make adjustment animations work (3.56 KB, patch)
2011-08-26 04:03 UTC, Ray Strode [halfline]
needs-work Details | Review
StAdjustment: Fix elastic property to be elastic (2.29 KB, patch)
2011-08-26 04:03 UTC, Ray Strode [halfline]
accepted-commit_now Details | Review
StAdjustment: Add alpha arg to interpolate method (2.86 KB, patch)
2011-08-26 04:04 UTC, Ray Strode [halfline]
reviewed Details | Review
StAdjustment: Document interpolate method (1.06 KB, patch)
2011-08-26 04:04 UTC, Ray Strode [halfline]
accepted-commit_now Details | Review
ModalDialog: Generalize and rename to Dialog (16.91 KB, patch)
2011-08-26 04:04 UTC, Ray Strode [halfline]
needs-work Details | Review
batch: Add mechanism for doing animation series (7.81 KB, patch)
2011-08-26 04:05 UTC, Ray Strode [halfline]
reviewed Details | Review
overview: Make optional (17.18 KB, patch)
2011-08-26 04:05 UTC, Ray Strode [halfline]
rejected Details | Review
dateMenu: Force min-width of events area, not whole menu (2.09 KB, patch)
2011-08-26 04:05 UTC, Ray Strode [halfline]
reviewed Details | Review
dateMenu: Make events list optional (7.05 KB, patch)
2011-08-26 04:06 UTC, Ray Strode [halfline]
reviewed Details | Review
global: Add concept of "session type" (6.94 KB, patch)
2011-08-26 04:06 UTC, Ray Strode [halfline]
reviewed Details | Review
main: Factor out user session specific bits (11.10 KB, patch)
2011-08-26 04:07 UTC, Ray Strode [halfline]
reviewed Details | Review
Add support for login session (52.63 KB, patch)
2011-08-26 04:07 UTC, Ray Strode [halfline]
reviewed Details | Review
st-adjustment: Drop all animation-y stuff (10.10 KB, patch)
2011-08-29 05:31 UTC, Ray Strode [halfline]
committed Details | Review
popupMenu: Hide separators when they aren't separating (13.52 KB, patch)
2011-08-29 05:31 UTC, Ray Strode [halfline]
committed Details | Review
popupMenu: Use new convenience method for settings (9.98 KB, patch)
2011-08-29 05:31 UTC, Ray Strode [halfline]
committed Details | Review
modalDialog: fade in buttons when first showing them (1.98 KB, patch)
2011-08-29 05:31 UTC, Ray Strode [halfline]
committed Details | Review
modalDialog: add mode that leaves shell reactive (4.14 KB, patch)
2011-08-29 05:31 UTC, Ray Strode [halfline]
committed Details | Review
panelMenu: Separate from ui chrome layer (1.47 KB, patch)
2011-08-29 05:32 UTC, Ray Strode [halfline]
committed Details | Review
dateMenu: Force min-width of events area, not whole menu (2.11 KB, patch)
2011-08-29 05:32 UTC, Ray Strode [halfline]
committed Details | Review
dateMenu: Make events list optional (7.40 KB, patch)
2011-08-29 05:32 UTC, Ray Strode [halfline]
committed Details | Review
shell-global: require init call before shell_global_get() (3.38 KB, patch)
2011-08-29 05:32 UTC, Ray Strode [halfline]
committed Details | Review
global: Add concept of "session type" (6.88 KB, patch)
2011-08-29 05:32 UTC, Ray Strode [halfline]
committed Details | Review
overview: Make viewSelector private (3.11 KB, patch)
2011-08-29 05:33 UTC, Ray Strode [halfline]
committed Details | Review
overview: make dash private (4.00 KB, patch)
2011-08-29 05:33 UTC, Ray Strode [halfline]
committed Details | Review
overview: make shellInfo private (3.17 KB, patch)
2011-08-29 05:33 UTC, Ray Strode [halfline]
committed Details | Review
overview: Add dummy mode (4.02 KB, patch)
2011-08-29 05:33 UTC, Ray Strode [halfline]
committed Details | Review
panel: Dynamically match corner to style of nearest button (6.29 KB, patch)
2011-08-29 05:33 UTC, Ray Strode [halfline]
needs-work Details | Review
popupMenu: Hide setttings menus outside user session (2.12 KB, patch)
2011-08-29 05:33 UTC, Ray Strode [halfline]
committed Details | Review
main: Factor out remaining user session specific bits (10.84 KB, patch)
2011-08-29 05:34 UTC, Ray Strode [halfline]
reviewed Details | Review
batch: Add mechanism for doing animation series (8.09 KB, patch)
2011-08-29 05:34 UTC, Ray Strode [halfline]
committed Details | Review
Add support for gdm greeter session (54.79 KB, patch)
2011-08-29 05:34 UTC, Ray Strode [halfline]
reviewed Details | Review
popupMenu: Raise menu when popping it up (823 bytes, patch)
2011-08-29 16:45 UTC, Ray Strode [halfline]
committed Details | Review
panel: Dynamically match corner to style of nearest button (6.53 KB, patch)
2011-08-29 16:45 UTC, Ray Strode [halfline]
committed Details | Review
main: Factor out remaining user session specific bits (11.00 KB, patch)
2011-08-29 16:45 UTC, Ray Strode [halfline]
committed Details | Review
Add support for gdm greeter session (54.52 KB, patch)
2011-08-29 16:46 UTC, Ray Strode [halfline]
committed Details | Review

Description Matthias Clasen 2011-08-22 15:04:49 UTC
We want to provide the same user experience on the login screen than in the regular desktop.
This is described in detail here: http://live.gnome.org/ThreePointOne/Features/LoginScreen
The code for this currenty lives in branches:

for the shell part: http://git.gnome.org/browse/gnome-shell/log/?h=wip/gdm-shell
for the gdm part: http://git.gnome.org/browse/gdm/log/?h=wip/shell-greeter
Comment 1 Ray Strode [halfline] 2011-08-24 15:19:13 UTC
Created attachment 194609 [details] [review]
st-box-layout: Document insertion apis

This commit just adds some brief doc comments
to st_box_layout_insert_actor() and
st_box_layout_insert_before()
Comment 2 Ray Strode [halfline] 2011-08-24 15:19:17 UTC
Created attachment 194610 [details] [review]
st-box-layout: Add methods for fetching sibling actors

StBoxLayout currently lets its callers add actors relative
to other actors in the layout with st_box_layout_insert_actor()
and st_box_layout_insert_before().

There isn't currently any way to query the adjacent siblings of
actors already in the list, however.

This commit adds two functions:

st_box_layout_get_actor_before()
and
st_box_layout_get_actor_after()

that return the actors respectively before and after a given
child.
Comment 3 Ray Strode [halfline] 2011-08-24 15:19:20 UTC
Created attachment 194611 [details] [review]
popupMenu: Hide separators when they aren't separating

A separator only makes sense if there are items on both
sides of it.  There is quite a lot of code written
spread throughout the shell that manages the process
of showing and hiding separators as the items around those
separators change.

This commit drops all that code in favor of changes to the menu
implementation to dynamically hide or show separators as
appropriate, so the callers don't have to deal with it.
Comment 4 Ray Strode [halfline] 2011-08-24 15:19:23 UTC
Created attachment 194612 [details] [review]
popupMenu: Use new convenience method for settings

All the system status menus in the panel offer a
menu item to jump to a relevant part of the
control-center.

This means each status icon has the same, or nearly the
same bit of code to:

- Add a new "action" menu item and listen for its activation.
- Hide the overview if it's showing when the menu item is activated
- Find the relevant control-center panel from its desktop file
- Launch the control-center to the relevant panel

This commit consolidates all those details in a new method,
addSettingsAction.  This refactoring reduces code duplication and
slight inconsistencies in the code resulting from that duplication.
It will also make it easier in subsequent commits to hide settings menu
items when the shell is used in the login screen.
Comment 5 Ray Strode [halfline] 2011-08-24 15:19:27 UTC
Created attachment 194613 [details] [review]
StBoxLayout: Use adjustment value property, not getter

The getter ignores intermediate values during interpolation, so using
the former breaks the StAdjustments built-in animation features.

This commit changes the code to use the "value" property instead
which always reflects the current state of the adjustment value,
even during interpolation.
Comment 6 Ray Strode [halfline] 2011-08-24 15:19:30 UTC
Created attachment 194614 [details] [review]
StAdjustment: Fix elastic property to be elastic

StAdjustment has an elastic property that doesn't seem to do anything.

This commit makes it do what it looks like the code originally intended
it to do. Namely, make the interpolation animation springy.
Comment 7 Ray Strode [halfline] 2011-08-24 15:19:34 UTC
Created attachment 194615 [details] [review]
StAdjustment: Add alpha arg to interpolate method

This commit adds an optional "alpha" argument to
st_adjustment_interpolate that gives the caller
control over how the interpolation should happen.
Comment 8 Ray Strode [halfline] 2011-08-24 15:19:37 UTC
Created attachment 194616 [details] [review]
StAdjustment: Document interpolate method

st_adjustment_interpolate() currently lacks a doc string.

This commit adds one.
Comment 9 Ray Strode [halfline] 2011-08-24 15:19:40 UTC
Created attachment 194617 [details] [review]
ModalDialog: Generalize and rename to Dialog

Most dialogs presented in the shell right now are system modal.
They shade out, and block input from other parts of the OS until
the user addresses the dialog.

There are cases, however, where it would be useful to show a dialog
that doesn't desensitize other parts of the OS chrome (such as a
user list in a future gnome-shell based GDM greeter).

This commit renames ModalDialog to Dialog and adds a modal property,
that determines whether or not to block outside events, shade outside
the dialog with a lightbox, etc.
Comment 10 Ray Strode [halfline] 2011-08-24 15:19:44 UTC
Created attachment 194618 [details] [review]
batch: Add mechanism for doing animation series

In order for transformation animations to look good, they need to be
incremental and have some order to them (e.g., fade out hidden items,
then shrink to close the void left over).

Chaining animations in this way can be error prone and wordy using just
Tweener callbacks.

This commit adds a new set of classes to help:

 - Task.  encapsulates schedulable work to be run in a specific scope.

 - ConsecutiveBatch.  runs a series of tasks in order and completes
                      when the last in the series finishes.

 - ConcurrentBatch.  runs a set of tasks at the same time and completes
                     when the last to finish completes.

 - Hold.  prevents a batch from completing the pending task until
          the hold is released.

The tasks associated with a batch are specified in a list at batch
construction time as either task objects or plain functions.
Batches are task objects, themselves, so they can be nested.

These new APIs aren't specific to doing animations, they are generally
useful.
Comment 11 Ray Strode [halfline] 2011-08-24 15:19:48 UTC
Created attachment 194619 [details] [review]
overview: Make optional

In preparation for using the shell at the login screen, we'll need
to be able to run the shell without an activities overview.

This commit prefaces code that accesses the overview state with a
to check first to see if there is an overview to access.
Comment 12 Ray Strode [halfline] 2011-08-24 15:19:52 UTC
Created attachment 194620 [details] [review]
dateMenu: Force min-width of events area, not whole menu

The theme currently hard codes the minimum size of the calendar
menu to make sure there's a designated area for events
(even if their isn't anything currently scheduled).

A side-effect of the hard coded minimum width is that
if the events area is hidden, the menu ends up much
bigger than the calendar.  We don't currently ever hide
the events area, but we will in the future.

This commit moves the min-width restriction from the menu
specifically to the events area.
Comment 13 Ray Strode [halfline] 2011-08-24 15:19:56 UTC
Created attachment 194621 [details] [review]
dateMenu: Make events list optional

Right now, when a user clicks on the panel clock, a menu pops up with a
calendar and a list of events from the user's schedule.  The list of
events only makes sense from within a user's session, however.

As part of the prep work for making the shell a platform for the login
screen, this commit makes the events list optional.
Comment 14 Ray Strode [halfline] 2011-08-24 15:20:01 UTC
Created attachment 194622 [details] [review]
global: Add concept of "session type"

This commit introduces a "session type" for
gnome-shell.  It essentially defines what
mode of operation the shell runs in
(normal-in-a-users-session mode, or at-the-login-screen mode).

Note this commit only lays the groundwork.  Actually
looking at the key and appropriately differentiating
the UI will happen in a subsequent commit.
Comment 15 Ray Strode [halfline] 2011-08-24 15:20:06 UTC
Created attachment 194623 [details] [review]
main: Factor out user session specific bits

The shell has a number of things are are only relevant for
logged in users (e.g. calendar events, telepathy integration, a
user menu, etc).

This commit moves those user session specific bits into their
own functions in preparation for making the shell code ready
for use at login time.
Comment 16 Ray Strode [halfline] 2011-08-24 15:20:10 UTC
Created attachment 194624 [details] [review]
Add support for login session

This commit adds support for login sessions.

It provides a user list that talks to GDM,
handles authentication via PAM, etc.

It doesn't currently support fingerprint readers
and smartcards.
Comment 17 Ray Strode [halfline] 2011-08-24 15:47:53 UTC
So this set of stuff should be basically, okay, I think.  It's blocking on me landing the shell-greeter branch of gdm upstream, but I expect to do that soon.

I also have a patch that adds swipe scrolling to the user list, but i've not included it here, because I want to redo it in terms of ClutterSwipeAction which I didn't know about when I first implemented it.

This set of patches doesn't include fingerprint or smartcard support.  I don't think those features are critical for 3.2, but it shouldn't be hard to add them in.

Another oddity is there's no shutdown menu.  The design doesn't have it but we've traditionally told people one way to shut down is to log out first.
Comment 18 Ray Strode [halfline] 2011-08-25 14:10:22 UTC
So one thing I forgot to add is a session selector (that only shows up if more than one session is installed).  I'm in the process of adding that in now.
Comment 19 Ray Strode [halfline] 2011-08-26 04:00:33 UTC
Created attachment 194766 [details] [review]
st-box-layout: Document insertion apis

This commit just adds some brief doc comments
to st_box_layout_insert_actor() and
st_box_layout_insert_before()
Comment 20 Ray Strode [halfline] 2011-08-26 04:01:19 UTC
Created attachment 194767 [details] [review]
st-box-layout: Add methods for fetching sibling actors

StBoxLayout currently lets its callers add actors relative
to other actors in the layout with st_box_layout_insert_actor()
and st_box_layout_insert_before().

There isn't currently any way to query the adjacent siblings of
actors already in the list, however.

This commit adds two functions:

st_box_layout_get_actor_before()
and
st_box_layout_get_actor_after()

that return the actors respectively before and after a given
child.
Comment 21 Ray Strode [halfline] 2011-08-26 04:01:57 UTC
Created attachment 194768 [details] [review]
popupMenu: Hide separators when they aren't separating

A separator only makes sense if there are items on both
sides of it.  There is quite a lot of code written
spread throughout the shell that manages the process
of showing and hiding separators as the items around those
separators change.

This commit drops all that code in favor of changes to the menu
implementation to dynamically hide or show separators as
appropriate, so the callers don't have to deal with it.
Comment 22 Ray Strode [halfline] 2011-08-26 04:03:26 UTC
Created attachment 194769 [details] [review]
popupMenu: Use new convenience method for settings

All the system status menus in the panel offer a
menu item to jump to a relevant part of the
control-center.

This means each status icon has the same, or nearly the
same bit of code to:

- Add a new "action" menu item and listen for its activation.
- Hide the overview if it's showing when the menu item is activated
- Find the relevant control-center panel from its desktop file
- Launch the control-center to the relevant panel

This commit consolidates all those details in a new method,
addSettingsAction.  This refactoring reduces code duplication and
slight inconsistencies in the code resulting from that duplication.
It will also make it easier in subsequent commits to hide settings menu
items when the shell is used in the login screen.
Comment 23 Ray Strode [halfline] 2011-08-26 04:03:45 UTC
Created attachment 194770 [details] [review]
StBoxLayout: Make adjustment animations work

StAdjustment can optionally change values incrementally
instead of instantaneously to, e.g., implement scrolling
animations.

StBoxLayout doesn't work very well with that feature, though.
It uses  the adjustment "value" getter and setters, which
n't play well with animations. The getter ignores
intermediate values during animations, and the setter stops
canels any pending animations.

This commit changes StBoxLayout to use the adjustment "value"
property directly, instead of its getter, and takes care to
avoid calling the setter during animations.
Comment 24 Ray Strode [halfline] 2011-08-26 04:03:57 UTC
Created attachment 194771 [details] [review]
StAdjustment: Fix elastic property to be elastic

StAdjustment has an elastic property that doesn't seem to do anything.

This commit makes it do what it looks like the code originally intended
it to do. Namely, make the interpolation animation springy.
Comment 25 Ray Strode [halfline] 2011-08-26 04:04:19 UTC
Created attachment 194772 [details] [review]
StAdjustment: Add alpha arg to interpolate method

This commit adds an optional "alpha" argument to
st_adjustment_interpolate that gives the caller
control over how the interpolation should happen.
Comment 26 Ray Strode [halfline] 2011-08-26 04:04:37 UTC
Created attachment 194773 [details] [review]
StAdjustment: Document interpolate method

st_adjustment_interpolate() currently lacks a doc string.

This commit adds one.
Comment 27 Ray Strode [halfline] 2011-08-26 04:04:48 UTC
Created attachment 194774 [details] [review]
ModalDialog: Generalize and rename to Dialog

Most dialogs presented in the shell right now are system modal.
They shade out, and block input from other parts of the OS until
the user addresses the dialog.

There are cases, however, where it would be useful to show a dialog
that doesn't desensitize other parts of the OS chrome (such as a
user list in a future gnome-shell based GDM greeter).

This commit renames ModalDialog to Dialog and adds a modal property,
that determines whether or not to block outside events, shade outside
the dialog with a lightbox, etc.
Comment 28 Ray Strode [halfline] 2011-08-26 04:05:14 UTC
Created attachment 194775 [details] [review]
batch: Add mechanism for doing animation series

In order for transformation animations to look good, they need to be
incremental and have some order to them (e.g., fade out hidden items,
then shrink to close the void left over).

Chaining animations in this way can be error prone and wordy using just
Tweener callbacks.

This commit adds a new set of classes to help:

 - Task.  encapsulates schedulable work to be run in a specific scope.

 - ConsecutiveBatch.  runs a series of tasks in order and completes
                      when the last in the series finishes.

 - ConcurrentBatch.  runs a set of tasks at the same time and completes
                     when the last to finish completes.

 - Hold.  prevents a batch from completing the pending task until
          the hold is released.

The tasks associated with a batch are specified in a list at batch
construction time as either task objects or plain functions.
Batches are task objects, themselves, so they can be nested.

These new APIs aren't specific to doing animations, they are generally
useful.
Comment 29 Ray Strode [halfline] 2011-08-26 04:05:28 UTC
Created attachment 194776 [details] [review]
overview: Make optional

In preparation for using the shell at the login screen, we'll need
to be able to run the shell without an activities overview.

This commit prefaces code that accesses the overview state with a
to check first to see if there is an overview to access.
Comment 30 Ray Strode [halfline] 2011-08-26 04:05:38 UTC
Created attachment 194777 [details] [review]
dateMenu: Force min-width of events area, not whole menu

The theme currently hard codes the minimum size of the calendar
menu to make sure there's a designated area for events
(even if their isn't anything currently scheduled).

A side-effect of the hard coded minimum width is that
if the events area is hidden, the menu ends up much
bigger than the calendar.  We don't currently ever hide
the events area, but we will in the future.

This commit moves the min-width restriction from the menu
specifically to the events area.
Comment 31 Ray Strode [halfline] 2011-08-26 04:06:12 UTC
Created attachment 194778 [details] [review]
dateMenu: Make events list optional

Right now, when a user clicks on the panel clock, a menu pops up with a
calendar and a list of events from the user's schedule.  The list of
events only makes sense from within a user's session, however.

As part of the prep work for making the shell a platform for the login
screen, this commit makes the events list optional.
Comment 32 Ray Strode [halfline] 2011-08-26 04:06:25 UTC
Created attachment 194779 [details] [review]
global: Add concept of "session type"

This commit introduces a "session type" for
gnome-shell.  It essentially defines what
mode of operation the shell runs in
(normal-in-a-users-session mode, or at-the-login-screen mode).

Note this commit only lays the groundwork.  Actually
looking at the key and appropriately differentiating
the UI will happen in a subsequent commit.
Comment 33 Ray Strode [halfline] 2011-08-26 04:07:18 UTC
Created attachment 194780 [details] [review]
main: Factor out user session specific bits

The shell has a number of things are are only relevant for
logged in users (e.g. calendar events, telepathy integration, a
user menu, etc).

This commit moves those user session specific bits into their
own functions in preparation for making the shell code ready
for use at login time.
Comment 34 Ray Strode [halfline] 2011-08-26 04:07:37 UTC
Created attachment 194781 [details] [review]
Add support for login session

This commit adds support for login sessions.

It provides a user list that talks to GDM,
handles authentication via PAM, etc.

It doesn't currently support fingerprint readers
and smartcards.
Comment 35 Ray Strode [halfline] 2011-08-26 04:10:37 UTC
I reposted the whole patchset to make it eaasy to apply, by the only significant change was the addition of a session chooser.

I also changed the boxlayout adjustment changes in attachment 194613 [details] [review] to attachment 194770 [details] [review] which is the same but with a couple more fixes to make it more animation friendly.
Comment 36 Dan Winship 2011-08-26 17:33:06 UTC
Comment on attachment 194767 [details] [review]
st-box-layout: Add methods for fetching sibling actors

>+ * Returns: (transfer full): the requested #ClutterActor or %NULL if
>+ * @actor is the first child in @self.

These should be (transfer none), for consistency with, eg, st_bin_get_child().

otherwise ok
Comment 37 Dan Winship 2011-08-26 17:41:19 UTC
Comment on attachment 194768 [details] [review]
popupMenu: Hide separators when they aren't separating

nice improvement

>+        while (childBefore && !childBefore.visible)
>+            childBefore = this.box.get_actor_before(childBefore);

It would be more efficient to just call this.box.get_children(), find the index of menuItem.actor, and then examine elements before and after it.

>+            this.connect('open-state-changed', Lang.bind(this, function() { this._updateSeparatorVisibility(menuItem); }));
>+            this._updateSeparatorVisibility(menuItem);

don't need the explicit call there since it will get called when the menu is first opened.
Comment 38 Dan Winship 2011-08-26 17:45:54 UTC
Comment on attachment 194769 [details] [review]
popupMenu: Use new convenience method for settings

>+    addSettingsAction: function(title, desktopFile) {
>+        let app = Shell.AppSystem.get_default().lookup_setting(desktopFile);
>+
>+        if (app) {
>+            let menuItem = this.addAction(title, function() {

I think we'd rather have the menu item always be there, and give an error message if the control panel can't be found. Also, since the user may install things while the shell is running, you shouldn't cache the app at startup.
Comment 39 Dan Winship 2011-08-26 18:04:54 UTC
Comment on attachment 194770 [details] [review]
StBoxLayout: Make adjustment animations work

>This commit changes StBoxLayout to use the adjustment "value"
>property directly, instead of its getter, and takes care to
>avoid calling the setter during animations.

get_value() and .value should not give different answers anyway. That's just broken. I'm not sure whether they should both behave like get_value() does now or like .value does now... Either way it won't break anything, since nothing currently uses st_adjustment_interpolate(). (We just use tweener on the adjustment instead.)

(There are also a bunch of typos in the commit message.)

>+  /* update adjustments for scrolling.
>+   * We only update the value if not interpolating,
>+   * since setting the value cancels any pending
>+   * interpolation, and interpolation handles making
>+   * sure the value is up to date anyway.
>+   */

This also seems not quite right. It would be more efficient to have it only change the adjustment if the height/width has actually changed.
Comment 40 Dan Winship 2011-08-26 18:08:17 UTC
Comment on attachment 194771 [details] [review]
StAdjustment: Fix elastic property to be elastic

>+  if (!priv->elastic)
>+    dx = CLAMP (dx, priv->lower,
>+                MAX (priv->lower, priv->upper - priv->page_size));

put {}s around multi-line if body please
Comment 41 Dan Winship 2011-08-26 18:10:52 UTC
Comment on attachment 194772 [details] [review]
StAdjustment: Add alpha arg to interpolate method

So, really, you ought to just be using Tweener rather than adding all this functionality to StAdjustment...

there's nothing obviously wrong with the patch though...
Comment 42 Dan Winship 2011-08-26 18:11:31 UTC
Comment on attachment 194773 [details] [review]
StAdjustment: Document interpolate method

ok, assuming we don't end up deciding to just kill it off...
Comment 43 Dan Winship 2011-08-26 18:27:32 UTC
Comment on attachment 194774 [details] [review]
ModalDialog: Generalize and rename to Dialog

>+const Gettext = imports.gettext.domain('gnome-shell');
>+const _ = Gettext.gettext;

you don't need to do that. _() is defined globally (by Environment.init())

>+        params = Params.parse(params, { modal: null,

false, not null. Although actually, you should make it default to true anyway, since that's the usual case.

>-        this._hasModal = false;

you still use this elsewhere

>+            Main.layoutManager.addChrome(this._group);

Is there any reason to use addChrome rather than putting it in uiGroup like in the modal case? (In the shell, we don't want non-modal dialogs, and I assume you don't have a shaped stage in the greeter, so there's no need to use chrome.)

>+            if (!hadChildren) {
>+                this._buttonLayout.opacity = 0;
>+                Tweener.addTween(this._buttonLayout,

this really belongs as a separate patch

>-        this._eventBlocker.raise_top();
>+        if (this.isModal)
>+          this._eventBlocker.raise_top();

This shouldn't be necessary, since you shouldn't be calling pushModal() or popModal() on a non-modal dialog. The fact that it does means that "non-modal" dialogs are non-modal with respect to other shell UI, but still modal with respect to other apps.
Comment 44 Dan Winship 2011-08-26 18:31:37 UTC
Comment on attachment 194776 [details] [review]
overview: Make optional

>This commit prefaces code that accesses the overview state with a
>to check first to see if there is an overview to access.

Too invasive, and someone will break it 10 minutes after you land these patches. You should just have the greeter have a dummy overview that never displays.
Comment 45 Dan Winship 2011-08-26 18:33:31 UTC
Comment on attachment 194777 [details] [review]
dateMenu: Force min-width of events area, not whole menu

>(even if their isn't anything currently scheduled).

"there"

>-        hbox = new St.BoxLayout({name: 'calendarArea'});
>+        hbox = new St.BoxLayout();

leave the old name anyway; themes might have been using it, and having names makes debugging easier sometimes.

>+        vbox = new St.BoxLayout({ name:     'calendarEventsArea',
>+                                  vertical: true});

we don't generally line up properties
Comment 46 Dan Winship 2011-08-26 18:36:02 UTC
Comment on attachment 194778 [details] [review]
dateMenu: Make events list optional

>+        if (this._eventList) {
>+            this._calendar.connect('selected-date-changed',

probably if we're not showing events, you shouldn't be able to click on dates on the calendar?
Comment 47 Dan Winship 2011-08-26 18:52:02 UTC
Comment on attachment 194779 [details] [review]
global: Add concept of "session type"

>+static gboolean is_login_mode = FALSE;

I don't love "login" as the name, because it makes me think of "login shell" in the command-line sense, which is exactly what this isn't. I don't have a better suggestion though, just bringing this up in case you do...

>   /* Initialize the global object */
>-  shell_global_get ();
>+  global = shell_global_get ();
>+
>+  if (is_login_mode)
>+    _shell_global_set_session_type (global, SHELL_SESSION_LOGIN);

I feel like we should have an explicit ShellGlobal constructor/initializer or something if we're going to be setting properties on it.
Comment 48 Dan Winship 2011-08-26 19:06:06 UTC
Comment on attachment 194780 [details] [review]
main: Factor out user session specific bits

blah. kind of a mess. but we can move things around later...

>+function _initRecorder() {

The recorder is potentially useful in the login screen as well...

>     ctrlAltTabManager = new CtrlAltTab.CtrlAltTabManager();
>-    overview = new Overview.Overview();
>+
>+    if (global.session_type == Shell.SessionType.USER)
>+        _createUserSession();
>+
>     magnifier = new Magnifier.Magnifier();

if we're keeping a dummy overview, then you don't need to call createUserSession in the middle there.

>+        if (global.session_type == Shell.SessionType.USER) {
>+            this._userMenu = new StatusMenu.StatusMenuButton();
>+            this._userMenu.actor.name = 'panelStatus';
>+            this._rightBox.add(this._userMenu.actor);

The CSS is probably not set up right to give you the same padding on the left and right ends of the panel if you don't have the user menu there.

>+            // Synchronize the buttons pseudo classes with its corner
>+            this._userMenu.actor.connect('style-changed', Lang.bind(this,

So, with the user menu gone, the rightmost status area menu now won't have the correct bottom highlight (instead of having a highlight that curves down with the panel, it will have a highlight that gets cut off by the un-highlighted curved panel corner). The easy fix here is to disable the curved corners in login mode.
Comment 49 Ray Strode [halfline] 2011-08-27 19:26:09 UTC
Comment on attachment 194766 [details] [review]
st-box-layout: Document insertion apis

Attachment 194766 [details] pushed as 4e9e6e7 - st-box-layout: Document insertion apis
Comment 50 Ray Strode [halfline] 2011-08-27 19:43:19 UTC
Comment on attachment 194767 [details] [review]
st-box-layout: Add methods for fetching sibling actors

(In reply to comment #37)
> (From update of attachment 194768 [details] [review])
> nice improvement
> 
> >+        while (childBefore && !childBefore.visible)
> >+            childBefore = this.box.get_actor_before(childBefore);
> 
> It would be more efficient to just call this.box.get_children(), find the index
> of menuItem.actor, and then examine elements before and after it.
Ah, well if I'm not going to use get_actor_before, then there's no reason to add the api, so i'll obsolete the patch for.
Comment 51 Ray Strode [halfline] 2011-08-27 19:53:35 UTC
(In reply to comment #38)
> (From update of attachment 194769 [details] [review])
> >+    addSettingsAction: function(title, desktopFile) {
> >+        let app = Shell.AppSystem.get_default().lookup_setting(desktopFile);
> >+
> >+        if (app) {
> >+            let menuItem = this.addAction(title, function() {
> 
> I think we'd rather have the menu item always be there, and give an error
> message if the control panel can't be found. Also, since the user may install
> things while the shell is running, you shouldn't cache the app at startup.
I guess we should just keep the old behavior of crashing instead of a notification.  Showing an item that doesn't ultimately work is the kind of thing that makes users agitated, so I'd advocate either treating this situation as "broken install" (and thus Not Our Problem (tm)), or hiding the item.
Comment 52 Ray Strode [halfline] 2011-08-27 20:27:16 UTC
Comment on attachment 194770 [details] [review]
StBoxLayout: Make adjustment animations work

(In reply to comment #39)
> (From update of attachment 194770 [details] [review])
> >This commit changes StBoxLayout to use the adjustment "value"
> >property directly, instead of its getter, and takes care to
> >avoid calling the setter during animations.
> 
> get_value() and .value should not give different answers anyway. That's just
> broken. I'm not sure whether they should both behave like get_value() does now
> or like .value does now...
Okay, so I guess the right thing here would be to make value behave like get_value() and then add a new "intermediate-value" property or some such, but...

> Either way it won't break anything, since nothing
> currently uses st_adjustment_interpolate(). (We just use tweener on the
> adjustment instead.)
Well, I was fixing it so that I could use it, obviously.  It sounds like you don't think I should use it, so there's no point in fixing it.  In fact, we should probably just drop the function ?

> >+  /* update adjustments for scrolling.
> >+   * We only update the value if not interpolating,
> >+   * since setting the value cancels any pending
> >+   * interpolation, and interpolation handles making
> >+   * sure the value is up to date anyway.
> >+   */
> 
> This also seems not quite right. It would be more efficient to have it only
> change the adjustment if the height/width has actually changed.
I don't think it's substantially more efficient to avoid the check in most cases.  I could imagine a slow down if something is watching the "value" property and does something expensive based on it changing, but spurious property notifications are part of the name of the game with gobject anyway, so they should already be handling them gracefully. Anyway, both min_height and avail_height would have to get monitored, right? It would add complexity to optimize a case that I don't think merits it.  Anyway, 

1) that's all independent of the changes in the patch, so if handled should be covered in a separate earlier patch
2) It looks like I'm not going to use st_adjustment_interpolate, anymore so I'll obsolete the patch (and maybe add one that gets rid of st_adjustment_interpolate and its weird semantics)
Comment 53 Ray Strode [halfline] 2011-08-27 20:29:19 UTC
Comment on attachment 194772 [details] [review]
StAdjustment: Add alpha arg to interpolate method

obsoleting, as per previous comments
Comment 54 Ray Strode [halfline] 2011-08-27 20:29:50 UTC
Comment on attachment 194773 [details] [review]
StAdjustment: Document interpolate method

obsoleting, as per previous comments
Comment 55 Ray Strode [halfline] 2011-08-27 20:30:58 UTC
Comment on attachment 194771 [details] [review]
StAdjustment: Fix elastic property to be elastic

obsoleting, as per previous comments
Comment 56 Ray Strode [halfline] 2011-08-27 20:47:55 UTC
(In reply to comment #43)
> (From update of attachment 194774 [details] [review])
> >+const Gettext = imports.gettext.domain('gnome-shell');
> >+const _ = Gettext.gettext;
> 
> you don't need to do that. _() is defined globally (by Environment.init())
heh, shows how old some of this code is.

> >+            Main.layoutManager.addChrome(this._group);
> 
> Is there any reason to use addChrome rather than putting it in uiGroup like in
> the modal case? (In the shell, we don't want non-modal dialogs, and I assume
> you don't have a shaped stage in the greeter, so there's no need to use
> chrome.)
I think I did that to make sure panel menus always end up stacked above the greeter dialog.  I'll have to look into it more though.
> 
> >+            if (!hadChildren) {
> >+                this._buttonLayout.opacity = 0;
> >+                Tweener.addTween(this._buttonLayout,
> 
> this really belongs as a separate patch
indeed.
 
> >-        this._eventBlocker.raise_top();
> >+        if (this.isModal)
> >+          this._eventBlocker.raise_top();
> 
> This shouldn't be necessary, since you shouldn't be calling pushModal() or
> popModal() on a non-modal dialog. The fact that it does means that "non-modal"
> dialogs are non-modal with respect to other shell UI, but still modal with
> respect to other apps.
Right, that's what I mean by "non-modal" at the moment. Granted, that limits their utility to the login screen, but we don't have and don't want them outside of the login screen anyway.  I guess I should be a little more forthright about that in the commit message/comments
Comment 57 Dan Winship 2011-08-27 21:32:13 UTC
Review of attachment 194781 [details] [review]:

It seems weird to have all the code for the login screen as part of the main gnome-shell code... oh well, we can look at moving it later...

::: data/theme/gnome-shell.css
@@ +1866,3 @@
 }
+
+/* Login Dialog */

however, it may make sense to have a gnome-login.css file, and load that in addition to gnome-shell.css?

::: js/Makefile.am
@@ +35,3 @@
 	ui/lightbox.js		\
 	ui/link.js		\
+	ui/loginDialog.js	\

likewise, I think it makes sense to put this in a new subdirectory of js/

::: js/ui/loginDialog.js
@@ +55,3 @@
+
+    if (actor.opacity == 255 && actor.visible)
+      return null;

there's inconsistent indentation throughout this file. should be 4 spaces

@@ +145,3 @@
+}
+
+function _thawActorSize(actor) {

should probably be called thawActorHeight, since it looks like if the actor's width has changed, that it will *not* be animated back, it will just jump back after the height finishes animating

@@ +187,3 @@
+
+        this._verticalBox.add(layout,
+                                 { y_fill: true,

the "{" should line up with the "l" in layout. (likewise below)

@@ +229,3 @@
+        if (iconFile) {
+            this._iconBin.show();
+            this._iconBin.set_style('background-image: url("' + iconFile + '");');

the standard way to do this would be with st_texture_cache_load_uri_async()

@@ +241,3 @@
+
+        if (iconName != null) {
+            let gicon = new Gio.ThemedIcon({ name: iconName });

likewise, st_texture_cache_load_icon_name()

@@ +309,3 @@
+
+        this._box = new St.BoxLayout({ vertical:    true,
+                                          style_class: 'login-dialog-user-list' });

alignment

@@ +383,3 @@
+    _getExpandedHeight: function() {
+        let hiddenActors = [];
+        for (let userName in this._items) {

for/in with arrays is undefined and we don't use it. use an index variable (like with hiding the hiddenActors below)

::: js/ui/main.js
@@ +27,3 @@
 const Layout = imports.ui.layout;
 const LookingGlass = imports.ui.lookingGlass;
+const LoginDialog = imports.ui.loginDialog;

this means loginDialog.js will be read in and parsed and all of its functions and classes defined, even in user mode.

@@ +48,3 @@
 let placesManager = null;
 let overview = null;
+let loginDialog = null;

you never use this variable outside of _createLoginSession() (let along outside of main.js)

@@ +86,3 @@
 
+function _createLoginSession() {
+    loginDialog = new LoginDialog.LoginDialog();

so... maybe you should just do

  let loginDialog = new imports.ui.loginDialog.LoginDialog();
Comment 58 Ray Strode [halfline] 2011-08-27 21:33:00 UTC
(In reply to comment #47)
> (From update of attachment 194779 [details] [review])
> >+static gboolean is_login_mode = FALSE;
> 
> I don't love "login" as the name, because it makes me think of "login shell" in
> the command-line sense, which is exactly what this isn't. I don't have a better
> suggestion though, just bringing this up in case you do...
--gdm-mode ? --not-logged-in-mode ?

> >   /* Initialize the global object */
> >-  shell_global_get ();
> >+  global = shell_global_get ();
> >+
> >+  if (is_login_mode)
> >+    _shell_global_set_session_type (global, SHELL_SESSION_LOGIN);
> 
> I feel like we should have an explicit ShellGlobal constructor/initializer or
> something if we're going to be setting properties on it.
What do you have in mind?  shell_global_get() would fail until
shell_global_new() is called? a shell_global_init(ShellSessionType type) ? A
shell_global_init(const char *first_property, ...) that takes a list of
constructonly properties?
Comment 59 Dan Winship 2011-08-27 21:35:19 UTC
Comment on attachment 194775 [details] [review]
batch: Add mechanism for doing animation series

so... I don't really love this API. "Hold" is sort of a confusing name (everywhere you use it in loginDialog it's really more like "WaitUntilISayGo"). ConcurrentBatch does not seem to provide any benefits over just making a bunch of concurrent calls yourself.

But anyway, it does simplify the chaining of animations, and I don't have a good suggestion for a better API we can agree on and implement before Monday, so I guess just land this for now. However, please put it in the "js/login/" (or whatever) directory rather than making it a generic part of the shell.
Comment 60 Ray Strode [halfline] 2011-08-27 21:43:01 UTC
(In reply to comment #57)
> Review of attachment 194781 [details] [review]:
> 
> It seems weird to have all the code for the login screen as part of the main
> gnome-shell code... oh well, we can look at moving it later...
Move it where? Owen was very specific that he wanted the greeter shipped with
gnome-shell and not separately.
> 
> ::: data/theme/gnome-shell.css
> @@ +1866,3 @@
>  }
> +
> +/* Login Dialog */
> 
> however, it may make sense to have a gnome-login.css file, and load that in
> addition to gnome-shell.css?
Yea, that may make sense, so it doesn't get loaded in a normal user session.
> 
> ::: js/Makefile.am
> @@ +35,3 @@
>      ui/lightbox.js        \
>      ui/link.js        \
> +    ui/loginDialog.js    \
> 
> likewise, I think it makes sense to put this in a new subdirectory of js/
okay, sounds fine.  Although, it's a little strange, since that new
subdirectory will be referencing stuff in ui/
> 
> ::: js/ui/loginDialog.js
> @@ +55,3 @@
> +
> +    if (actor.opacity == 255 && actor.visible)
> +      return null;
> 
> there's inconsistent indentation throughout this file. should be 4 spaces
This particular case was just an accident.  I think the other indentation
problems are because I forgot to reindent everything after renaming variables
from fooLayout to fooBox to better match other callers of BoxLayout.

I'll fix it.

> so... maybe you should just do
> 
>   let loginDialog = new imports.ui.loginDialog.LoginDialog();
Okay, a little ugly, but does prevent code that will never get run in the
normal case from getting loaded.
Comment 61 Ray Strode [halfline] 2011-08-27 22:05:30 UTC
(In reply to comment #59)
> (From update of attachment 194775 [details] [review])
> so... I don't really love this API. "Hold" is sort of a confusing name
> (everywhere you use it in loginDialog it's really more like "WaitUntilISayGo").
To mean the word hold does mean "wait until I say otherwise".  e.g, "Hold on for a minute" means "stop until I give you the go ahead to continue".  Calling it a wait or something would be weird, since its an object, not a command.
I don't really understand your objection to the name, but some possible alteratives:  Barrier, Block, Pauser
   
> ConcurrentBatch does not seem to provide any benefits over just making a bunch
> of concurrent calls yourself.
well, the point of concurrent batch is to suspend the higher level batch until /all/ the concurrent calls finish.  If you just make the calls yourself, then you would have to track each one individually, and after each one finishes check to see if all the other outstanding ones are done so you know whether or not you can continue to the next step in the chain.

> But anyway, it does simplify the chaining of animations, and I don't have a
> good suggestion for a better API we can agree on and implement before Monday,
> so I guess just land this for now. However, please put it in the "js/login/"
> (or whatever) directory rather than making it a generic part of the shell.
Okay, though, I think getting something like this figured out (after a few rounds of discussion) would clarify other parts of the code (even things that aren't animations)
Comment 62 Dan Winship 2011-08-27 23:03:49 UTC
> > Either way it won't break anything, since nothing
> > currently uses st_adjustment_interpolate(). (We just use tweener on the
> > adjustment instead.)
> Well, I was fixing it so that I could use it, obviously.  It sounds like you
> don't think I should use it, so there's no point in fixing it.  In fact, we
> should probably just drop the function ?

I think Owen and I sort of came to that conclusion on Friday too.

> > Is there any reason to use addChrome rather than putting it in uiGroup like in
> > the modal case? (In the shell, we don't want non-modal dialogs, and I assume
> > you don't have a shaped stage in the greeter, so there's no need to use
> > chrome.)
> I think I did that to make sure panel menus always end up stacked above the
> greeter dialog.  I'll have to look into it more though.

Hm... of course there's really no point in panel menus being in the chrome layer either, since they're only shown when we're pushModal()ed

> --gdm-mode ? --not-logged-in-mode ?

--gdm-mode doesn't suck

> > I feel like we should have an explicit ShellGlobal constructor/initializer or
> > something if we're going to be setting properties on it.
> What do you have in mind?  shell_global_get() would fail until
> shell_global_new() is called? a shell_global_init(ShellSessionType type) ? A
> shell_global_init(const char *first_property, ...) that takes a list of
> constructonly properties?

I didn't have anything in particular in mind. Those sound good. You don't actually need to worry about the "shell_global_get() called before ShellGlobal is initialized" case, because we know a priori that that doesn't happen.

> > It seems weird to have all the code for the login screen as part of the main
> > gnome-shell code... oh well, we can look at moving it later...
> Move it where? Owen was very specific that he wanted the greeter shipped with
> gnome-shell and not separately.

Ah

(In reply to comment #61)
> To me the word hold does mean "wait until I say otherwise".  e.g, "Hold on
> for a minute" means "stop until I give you the go ahead to continue".  Calling
> it a wait or something would be weird, since its an object, not a command.

I don't have a good explanation for why I don't like it. It just feels wrong somehow. :)

> well, the point of concurrent batch is to suspend the higher level batch until
> /all/ the concurrent calls finish.

Ah, indeed
Comment 63 Ray Strode [halfline] 2011-08-28 02:15:32 UTC
(In reply to comment #56)
> > The fact that it does means that "non-modal"
> > dialogs are non-modal with respect to other shell UI, but still modal with
> > respect to other apps.
> Right, that's what I mean by "non-modal" at the moment. Granted, that limits
> their utility to the login screen, but we don't have and don't want them
> outside of the login screen anyway.  I guess I should be a little more
> forthright about that in the commit message/comments

So thinking about this more, it would be lot less intrusive to leave the class called ModalDialog and then name the property shellReactive or some such.  This way we avoid the big rename, don't conflate what the word modal means, and keep the default of the value false instead of true which seems like better practice.
Comment 64 Ray Strode [halfline] 2011-08-28 03:21:13 UTC
(In reply to comment #48)
> (From update of attachment 194780 [details] [review])
> blah. kind of a mess. but we can move things around later...
> 
> >+function _initRecorder() {
> 
> The recorder is potentially useful in the login screen as well...
We probably don't want unauthenticated users to have the ability to eat up disk space in an unbounded way.
Comment 65 Ray Strode [halfline] 2011-08-28 14:41:47 UTC
(In reply to comment #48)
> >     ctrlAltTabManager = new CtrlAltTab.CtrlAltTabManager();
> >-    overview = new Overview.Overview();
> >+
> >+    if (global.session_type == Shell.SessionType.USER)
> >+        _createUserSession();
> >+
> >     magnifier = new Magnifier.Magnifier();
> 
> if we're keeping a dummy overview, then you don't need to call
> createUserSession in the middle there.
Why not? For the user sessionp path, the overview still needs to be created after the layoutManager and before the panel.
Comment 66 Ray Strode [halfline] 2011-08-29 01:05:39 UTC
(In reply to comment #57)
> @@ +229,3 @@
> +        if (iconFile) {
> +            this._iconBin.show();
> +            this._iconBin.set_style('background-image: url("' + iconFile +
> '");');
> 
> the standard way to do this would be with st_texture_cache_load_uri_async()
That wouldn't really be equivalent. The image needs to be a background image so it can be themed with a frame.

> @@ +241,3 @@
> +
> +        if (iconName != null) {
> +            let gicon = new Gio.ThemedIcon({ name: iconName });
> 
> likewise, st_texture_cache_load_icon_name()
Looks like that's not true anymore:

http://git.gnome.org/browse/gnome-shell/commit/?id=0e3431ac47ec3c5f69933ec5df78b922fc7f8976

Looking at other examples in the codebase I can skip the gicon and use an icon_name directly though.
Comment 67 Ray Strode [halfline] 2011-08-29 05:01:53 UTC
(In reply to comment #57)
> @@ +383,3 @@
> +    _getExpandedHeight: function() {
> +        let hiddenActors = [];
> +        for (let userName in this._items) {
> 
> for/in with arrays is undefined and we don't use it. use an index variable
> (like with hiding the hiddenActors below)
this._items is a dictionary not an array
Comment 68 Ray Strode [halfline] 2011-08-29 05:31:04 UTC
Created attachment 195036 [details] [review]
st-adjustment: Drop all animation-y stuff

StAdjustment has some non-functional and unused animation vestiges
like the "elastic" property, st_adjustment_interpolate() and
st_adjustment_clamp().

This commit vacuums that stuff up so it doesn't tempt anyone into
trying to use it.
Comment 69 Ray Strode [halfline] 2011-08-29 05:31:22 UTC
Created attachment 195037 [details] [review]
popupMenu: Hide separators when they aren't separating

A separator only makes sense if there are items on both
sides of it. There is quite a lot of code written
throughout the shell that manages the process of showing
and hiding separators as the items around those separators
change.

This commit drops all that code in favor of changes to the menu
implementation to dynamically hide or show separators as
appropriate, so the callers don't have to deal with it.
Comment 70 Ray Strode [halfline] 2011-08-29 05:31:30 UTC
Created attachment 195038 [details] [review]
popupMenu: Use new convenience method for settings

All the system status menus in the panel offer a
menu item to jump to a relevant part of the
control-center.

This means each status icon has the same, or nearly the
same bit of code to:

- Add a new "action" menu item and listen for its activation.
- Hide the overview if it's showing when the menu item is activated
- Find the relevant control-center panel from its desktop file
- Launch the control-center to the relevant panel

This commit consolidates all those details in a new method,
addSettingsAction.  This refactoring reduces code duplication and
slight inconsistencies in the code resulting from that duplication.
It will also make it easier in subsequent commits to hide settings menu
items when the shell is used in the login screen.
Comment 71 Ray Strode [halfline] 2011-08-29 05:31:42 UTC
Created attachment 195039 [details] [review]
modalDialog: fade in buttons when first showing them

Right now, if buttons get set on a dialog after it is mapped,
they just pop in instantly.

We shouldn't have any harsh transitions like that, though.

This commit changes the buttons to quickly fade in, instead.
Comment 72 Ray Strode [halfline] 2011-08-29 05:31:55 UTC
Created attachment 195040 [details] [review]
modalDialog: add mode that leaves shell reactive

A modal dialog in the shell blocks anything but that dialog from
receiving user input. Applications within the session and other
parts of UI are rendered non-reactive.

When GDM gets changed to use the shell for its greeter, the user
list will be presented as a shell dialog. That dialog shouldn't
block access to the panel menus, etc.

This commit adds a shellReactive property that makes the ModalDialog
class continue to block access to applications, but allow the user
to interact with the shell itself.
Comment 73 Ray Strode [halfline] 2011-08-29 05:32:13 UTC
Created attachment 195041 [details] [review]
panelMenu: Separate from ui chrome layer

The chrome layer contains the user interface elements (e.g.,
the panel) that disappear when fullscreen windows get displayed.

Panel menus are currently put in the chrome layer, but don't need
to be, since they are only displayed when the user is interacting
with the shell and not a fullscreen application.

Putting panel menus in the chrome layer does mean they will get
stacked below shell interface elements that aren't in the chrome layer,
though.

This commit changes panel menus to be on the same layer as most other
shell elements, so they get properly stacked above those elements.
Comment 74 Ray Strode [halfline] 2011-08-29 05:32:22 UTC
Created attachment 195042 [details] [review]
dateMenu: Force min-width of events area, not whole menu

The theme currently hard codes the minimum size of the calendar
menu to make sure there's a designated area for events
(even if there isn't anything currently scheduled).

A side-effect of the hard coded minimum width is that
if the events area is hidden, the menu ends up much
bigger than the calendar.  We don't currently ever hide
the events area, but we will in the future.

This commit moves the min-width restriction from the menu
specifically to the events area.
Comment 75 Ray Strode [halfline] 2011-08-29 05:32:31 UTC
Created attachment 195043 [details] [review]
dateMenu: Make events list optional

Right now, when a user clicks on the panel clock, a menu pops up with a
calendar and a list of events from the user's schedule.  The list of
events only makes sense from within a user's session, however.

As part of the prep work for making the shell a platform for the login
screen, this commit makes the events list optional.
Comment 76 Ray Strode [halfline] 2011-08-29 05:32:43 UTC
Created attachment 195044 [details] [review]
shell-global: require init call before shell_global_get()

shell_global_get() currently implicitly instantiates the shell
global singleton the first time it's called.  This means there's
no opportunity to set construction-time properties on the singleton.

This isn't an issue yet, because there aren't any.  We will need it
in the future, though, when we grow a --gdm-mode that gets exposed as
a property through the global singleton.

This commit adds a new _shell_global_init() function that must be
invoked before shell_global_get() can be called.
Comment 77 Ray Strode [halfline] 2011-08-29 05:32:52 UTC
Created attachment 195045 [details] [review]
global: Add concept of "session type"

This commit introduces a "session type" for
gnome-shell.  It essentially defines what
mode of operation the shell runs in
(normal-in-a-users-session mode, or at-the-login-screen mode).

Note this commit only lays the groundwork.  Actually
looking at the key and appropriately differentiating
the UI will happen in subsequent commits.
Comment 78 Ray Strode [halfline] 2011-08-29 05:33:05 UTC
Created attachment 195046 [details] [review]
overview: Make viewSelector private

It's only used internally by the overview itself,
so don't expose it as a public object.
Comment 79 Ray Strode [halfline] 2011-08-29 05:33:10 UTC
Created attachment 195047 [details] [review]
overview: make dash private

The dash object is currently exposed as a public object.
It's only used outside of the overview for the dash object's
iconSize property though.

This commit makes the dash object private and proxies the dash
iconSize property to the overview.
Comment 80 Ray Strode [halfline] 2011-08-29 05:33:16 UTC
Created attachment 195048 [details] [review]
overview: make shellInfo private

This commit forwards the shellInfo setMessage method
to the overview itself and makes shellInfo private.
Comment 81 Ray Strode [halfline] 2011-08-29 05:33:28 UTC
Created attachment 195049 [details] [review]
overview: Add dummy mode

We're not going to want an overview at the login screen,
but a lot of code in the shell depends on the overview
existing.

This commit adds a new isDummy constructor property to
allow creating the overview as a non-functional, stub object
that doesn't do anything visible.
Comment 82 Ray Strode [halfline] 2011-08-29 05:33:40 UTC
Created attachment 195050 [details] [review]
panel: Dynamically match corner to style of nearest button

Right now the panel code makes the left corner sync up with the
activities button and the right corner sync up with the user menu.
This is fine as long as we have an activities button and a user menu.

The login screen won't have those things, though.

This commit changes the panel corner code to try to figure out which
interface element is the most appropriate to sync up with based on
its position in the panel.
Comment 83 Ray Strode [halfline] 2011-08-29 05:33:54 UTC
Created attachment 195051 [details] [review]
popupMenu: Hide setttings menus outside user session

The control-center contains user-pertinent settings
panels. These panels don't make sense to show outside
of a user's session, so hide them for session types other
than SessionType.USER.
Comment 84 Ray Strode [halfline] 2011-08-29 05:34:09 UTC
Created attachment 195052 [details] [review]
main: Factor out remaining user session specific bits

The shell has a number of things that are only relevant for
logged in users (e.g. calendar events, telepathy integration, a
user menu, etc).

This commit moves those user session specific bits into their
own functions in preparation for making the shell code ready
for use at login time.
Comment 85 Ray Strode [halfline] 2011-08-29 05:34:21 UTC
Created attachment 195053 [details] [review]
batch: Add mechanism for doing animation series

In order for transformation animations to look good, they need to be
incremental and have some order to them (e.g., fade out hidden items,
then shrink to close the void left over).

Chaining animations in this way can be error prone and wordy using just
Tweener callbacks.

This commit adds a new set of classes to help:

 - Task.  encapsulates schedulable work to be run in a specific scope.

 - ConsecutiveBatch.  runs a series of tasks in order and completes
                      when the last in the series finishes.

 - ConcurrentBatch.  runs a set of tasks at the same time and completes
                     when the last to finish completes.

 - Hold.  prevents a batch from completing the pending task until
          the hold is released.

The tasks associated with a batch are specified in a list at batch
construction time as either task objects or plain functions.
Batches are task objects, themselves, so they can be nested.

For now, these APIs are temporarily getting staged in a gdm/ specific
subdirectory so they will be available for use by GDM.  They aren't
specific to GDM, or even to doing animations, though, so the API may eventually
move in some form or another to a more general location. Alternatively, the
APIs may ultimately get dropped entirely and replaced by something else.
Comment 86 Ray Strode [halfline] 2011-08-29 05:34:33 UTC
Created attachment 195054 [details] [review]
Add support for gdm greeter session

This commit adds GDM session support.

It provides a user list that talks to GDM,
handles authentication via PAM, etc.

It doesn't currently support fingerprint readers
and smartcards.
Comment 87 Ray Strode [halfline] 2011-08-29 12:54:25 UTC
Review of attachment 195045 [details] [review]:

::: src/main.c
@@ +473,3 @@
+    "gdm-mode", 0, 0, G_OPTION_ARG_NONE,
+    &is_gdm_mode,
+    N_("Login mode"),

I need to give the argument a better description, like "Mode used by GDM for login screen"
Comment 88 Dan Winship 2011-08-29 13:26:52 UTC
Comment on attachment 195036 [details] [review]
st-adjustment: Drop all animation-y stuff

>-static void
> st_adjustment_dispose (GObject *object)
> {
>-  stop_interpolation (ST_ADJUSTMENT (object));
>-
>   G_OBJECT_CLASS (st_adjustment_parent_class)->dispose (object);
> }

you can just remove st_adjustment_dispose entirely.

good to commit after that
Comment 89 Dan Winship 2011-08-29 13:30:15 UTC
Comment on attachment 195037 [details] [review]
popupMenu: Hide separators when they aren't separating

>+    _findIndexOfActor: function(children, actor) {

you don't need this...

>+        let index = this._findIndexOfActor(children, menuItem.actor);

let index = children.indexOf(menuItem.actor)

ok to commit with that change
Comment 90 Dan Winship 2011-08-29 13:32:59 UTC
Comment on attachment 195038 [details] [review]
popupMenu: Use new convenience method for settings

>+                           if (!app) {
>+                               log('Settings panel for desktop file ' + desktopFile + ' could not be loaded!');

by "give an error message", I meant something user-visible like Main.notify(). But anyway, this can be changed later if we don't like it.
Comment 91 Dan Winship 2011-08-29 13:34:02 UTC
Comment on attachment 195039 [details] [review]
modalDialog: fade in buttons when first showing them

>+        /* Fade in buttons if there weren't any before
>+         */

// use js comment style
Comment 92 Dan Winship 2011-08-29 13:35:54 UTC
Comment on attachment 195040 [details] [review]
modalDialog: add mode that leaves shell reactive

ok
Comment 93 Dan Winship 2011-08-29 13:39:44 UTC
Comment on attachment 195044 [details] [review]
shell-global: require init call before shell_global_get()

>+  _shell_global_init(NULL);

space before (

>+/**â¢
>+ * _shell_global_init: (skip)â¢

bz seems to have mangled it a bit, but there are bullets at the ends of each line here...
Comment 94 Dan Winship 2011-08-29 13:42:08 UTC
Comment on attachment 195045 [details] [review]
global: Add concept of "session type"

>+    &is_gdm_mode,
>+    N_("Login mode"),

"GDM mode" ?
Comment 95 Dan Winship 2011-08-29 13:46:56 UTC
Comment on attachment 195046 [details] [review]
overview: Make viewSelector private

>It's only used internally by the overview itself,

js/perf/core.js refers to it as well, but it's fine if that code refers to private objects, so just fix it there too.
Comment 96 Dan Winship 2011-08-29 13:51:37 UTC
Comment on attachment 195049 [details] [review]
overview: Add dummy mode

I was thinking actually having a different class that just implemented the same methods as no-ops... but this is fine.
Comment 97 Dan Winship 2011-08-29 14:07:17 UTC
Comment on attachment 195050 [details] [review]
panel: Dynamically match corner to style of nearest button

>+        else if (this._side == St.Side.RIGHT)
>+            button = this._findAdjacentButtonOnLeft(children, children.length - 2);

mmm. magic numbers.

This code is too fragile (it assumes things about what order the panel's children were added in), and it doesn't work in RTL locales anyway (note that the leftCorner is always on the left, but the leftBox is on the right side in RTL).

Having panel pass the leftBox or rightBox to the PanelCorner constructor would simplify this a bunch. Then you just need to repeatedly fetch either the first or last child of that container until you get a panelmenu.
Comment 98 Dan Winship 2011-08-29 14:08:14 UTC
Comment on attachment 195051 [details] [review]
popupMenu: Hide setttings menus outside user session

>Subject: [PATCH] popupMenu: Hide setttings menus outside user session

Tttoo many "t"s

>
>The control-center contains user-pertinent settings
>panels. These panels don't make sense to show outside
>of a user's session, so hide them for session types other
>than SessionType.USER.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=657082
>---
> js/ui/dateMenu.js  |   13 +++++++------
> js/ui/popupMenu.js |    4 ++++
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
>diff --git a/js/ui/dateMenu.js b/js/ui/dateMenu.js
>index dbab336..37ce87b 100644
>--- a/js/ui/dateMenu.js
>+++ b/js/ui/dateMenu.js
>@@ -97,13 +97,14 @@ DateMenuButton.prototype = {
>         vbox.add(this._calendar.actor);
> 
>         item = this.menu.addSettingsAction(_("Date and Time Settings"), 'gnome-datetime-panel.desktop');
>+        if (item) {
>+            let separator = new PopupMenu.PopupSeparatorMenuItem();
>+            separator.setColumnWidths(1);
>+            vbox.add(separator.actor, {y_align: St.Align.END, expand: true, y_fill: false});
> 
>-        let separator = new PopupMenu.PopupSeparatorMenuItem();
>-        separator.setColumnWidths(1);
>-        vbox.add(separator.actor, {y_align: St.Align.END, expand: true, y_fill: false});
>-
>-        item.actor.can_focus = false;
>-        item.actor.reparent(vbox);
>+            item.actor.can_focus = false;
>+            item.actor.reparent(vbox);
>+        }
> 
>         if (params.showEvents) {
>             // Add vertical separator
>diff --git a/js/ui/popupMenu.js b/js/ui/popupMenu.js
>index 88b7ee2..dc099f0 100644
>--- a/js/ui/popupMenu.js
>+++ b/js/ui/popupMenu.js
>@@ -813,6 +813,10 @@ PopupMenuBase.prototype = {
>     },
> 
>     addSettingsAction: function(title, desktopFile) {
>+        // Don't allow user settings to get edited unless we're in a user session
>+        if (global.session_type != Shell.SessionType.USER)
>+            return null;
>+
>         let menuItem = this.addAction(title, function() {
>                            let app = Shell.AppSystem.get_default().lookup_setting(desktopFile);
> 
>-- 
>1.7.6
Comment 99 Dan Winship 2011-08-29 14:12:25 UTC
Comment on attachment 195052 [details] [review]
main: Factor out remaining user session specific bits

>-    overview = new Overview.Overview();
>+
>+    if (global.session_type == Shell.SessionType.USER)
>+        _createUserSession();

so what I meant before about not needing to createUserSession here is that you can just change

    overview = new Overview.Overview();

to

    overview = new Overview.Overview({ isDummy: global.session_type != Shell.SessionType.USER });

and then call _createUserSession() at the end, rather than interrupting the block of initializers in the middle.
Comment 100 Dan Winship 2011-08-29 14:17:20 UTC
Comment on attachment 195054 [details] [review]
Add support for gdm greeter session

>+ * Portions adapted from Mx's data/style/default.css
>+ *   Copyright 2009 Intel Corporation

you can remove that. The parts copied from Mx didn't get copied into here.

>+const Lang = imports.lang;
>+const Signals = imports.signals;

these should be alphabetized in with the gi imports

>+const ModalDialog = imports.ui.modalDialog;

need to realphabetize that under "M"

>+let loginDialog = null;

still don't need that as a toplevel variable afaict

>+    // Instantiate a dummy overview to keep code that
>+    // assumes the presence of an overview working
>+    overview = new Overview.Overview();

forgot the isDummy param...
Comment 101 Ray Strode [halfline] 2011-08-29 16:14:49 UTC
Comment on attachment 195036 [details] [review]
st-adjustment: Drop all animation-y stuff

Attachment 195036 [details] pushed as b12967b - st-adjustment: Drop all animation-y stuff
Comment 102 Ray Strode [halfline] 2011-08-29 16:15:38 UTC
Comment on attachment 195037 [details] [review]
popupMenu: Hide separators when they aren't separating

Attachment 195037 [details] pushed as f96b2ee - popupMenu: Hide separators when they aren't separating
Comment 103 Ray Strode [halfline] 2011-08-29 16:16:23 UTC
Comment on attachment 195038 [details] [review]
popupMenu: Use new convenience method for settings

Attachment 195038 [details] pushed as 13bf64a - popupMenu: Use new convenience method for settings
Comment 104 Ray Strode [halfline] 2011-08-29 16:16:50 UTC
Comment on attachment 195039 [details] [review]
modalDialog: fade in buttons when first showing them

Attachment 195039 [details] pushed as e8914c6 - modalDialog: fade in buttons when first showing them
Comment 105 Ray Strode [halfline] 2011-08-29 16:17:23 UTC
Comment on attachment 195041 [details] [review]
panelMenu: Separate from ui chrome layer

Attachment 195041 [details] pushed as 388cfa3 - panelMenu: Separate from ui chrome layer
Comment 106 Ray Strode [halfline] 2011-08-29 16:18:23 UTC
Comment on attachment 195042 [details] [review]
dateMenu: Force min-width of events area, not whole menu

Attachment 195042 [details] pushed as 5be9326 - dateMenu: Force min-width of events area, not whole menu
Comment 107 Ray Strode [halfline] 2011-08-29 16:18:58 UTC
Comment on attachment 195043 [details] [review]
dateMenu: Make events list optional

Attachment 195043 [details] pushed as b6c2399 - dateMenu: Make events list optional
Comment 108 Ray Strode [halfline] 2011-08-29 16:20:56 UTC
Comment on attachment 195044 [details] [review]
shell-global: require init call before shell_global_get()

Attachment 195044 [details] pushed as 4156a4c - shell-global: require init call before shell_global_get()
Comment 109 Ray Strode [halfline] 2011-08-29 16:22:48 UTC
Comment on attachment 195045 [details] [review]
global: Add concept of "session type"

Attachment 195045 [details] pushed as 5088f22 - global: Add concept of "session type"
Comment 110 Ray Strode [halfline] 2011-08-29 16:24:25 UTC
Comment on attachment 195046 [details] [review]
overview: Make viewSelector private

Attachment 195046 [details] pushed as 80a9d2e - overview: Make viewSelector private
Comment 111 Ray Strode [halfline] 2011-08-29 16:25:13 UTC
Comment on attachment 195047 [details] [review]
overview: make dash private

Attachment 195047 [details] pushed as 356e4c0 - overview: make dash private
Comment 112 Ray Strode [halfline] 2011-08-29 16:29:59 UTC
Comment on attachment 195048 [details] [review]
overview: make shellInfo private

Attachment 195048 [details] pushed as 67ae8ed - overview: make shellInfo private
Comment 113 Ray Strode [halfline] 2011-08-29 16:31:07 UTC
Comment on attachment 195049 [details] [review]
overview: Add dummy mode

Attachment 195049 [details] pushed as 35e9926 - overview: Add dummy mode
Comment 114 Ray Strode [halfline] 2011-08-29 16:34:46 UTC
Comment on attachment 195051 [details] [review]
popupMenu: Hide setttings menus outside user session

Attachment 195051 [details] pushed as 239a9e48 - popupMenu: Hide setttings menus outside user session
Comment 115 Ray Strode [halfline] 2011-08-29 16:37:13 UTC
Comment on attachment 195040 [details] [review]
modalDialog: add mode that leaves shell reactive

Attachment 195040 [details] pushed as db39ba3 - modalDialog: add mode that leaves shell reactive
Comment 116 Ray Strode [halfline] 2011-08-29 16:38:54 UTC
Okay, in order to keep things fluid, I've pushed the stuff that was ready to go in.

I'll post updates for other patches that need another look.

Also, I have one more popupMenu patch which I'll post.
Comment 117 Ray Strode [halfline] 2011-08-29 16:44:29 UTC
Comment on attachment 195053 [details] [review]
batch: Add mechanism for doing animation series

Attachment 195053 [details] pushed as 4902a60 - batch: Add mechanism for doing animation series
Comment 118 Ray Strode [halfline] 2011-08-29 16:45:12 UTC
Created attachment 195087 [details] [review]
popupMenu: Raise menu when popping it up

When a menu gets popped up, it should never
pop behind anything else in the shell.
Comment 119 Ray Strode [halfline] 2011-08-29 16:45:27 UTC
Created attachment 195088 [details] [review]
panel: Dynamically match corner to style of nearest button

Right now the panel code makes the left corner sync up with the
activities button and the right corner sync up with the user menu.
This is fine as long as we have an activities button and a user menu.

The login screen won't have those things, though.

This commit changes the panel corner code to try to figure out which
interface element is the most appropriate to sync up with based on
its position in the panel.
Comment 120 Ray Strode [halfline] 2011-08-29 16:45:59 UTC
Created attachment 195089 [details] [review]
main: Factor out remaining user session specific bits

The shell has a number of things that are only relevant for
logged in users (e.g. calendar events, telepathy integration, a
user menu, etc).

This commit moves those user session specific bits into their
own functions in preparation for making the shell code ready
for use at login time.
Comment 121 Ray Strode [halfline] 2011-08-29 16:46:07 UTC
Created attachment 195090 [details] [review]
Add support for gdm greeter session

This commit adds GDM session support.

It provides a user list that talks to GDM,
handles authentication via PAM, etc.

It doesn't currently support fingerprint readers
and smartcards.
Comment 122 Dan Winship 2011-08-29 17:07:27 UTC
Comment on attachment 195088 [details] [review]
panel: Dynamically match corner to style of nearest button

looks right
Comment 123 Ray Strode [halfline] 2011-08-29 18:14:53 UTC
Attachment 195087 [details] pushed as 7f767c4 - popupMenu: Raise menu when popping it up
Attachment 195088 [details] pushed as 1ecbabc - panel: Dynamically match corner to style of nearest button
Attachment 195089 [details] pushed as d4239d5 - main: Factor out remaining user session specific bits
Attachment 195090 [details] pushed as 9f1da20 - Add support for gdm greeter session
Comment 124 Ray Strode [halfline] 2011-08-31 13:50:11 UTC
I've filed bug 657822 and bug 657823 to address the things mentioned in comment 17