Created attachment 138355 [details] [review]
Use one-word names for apps

While preparing this patch for posting, it occured to me that this will not work well in English, where often more than one word is needed. For Dutch systems this algorithm seems to work quite well:

    http://student.science.uva.nl/~sqdh/.tmp/app-well-names.png

but for example 'Tekstverwerker' is 'Word Processor' in English, and gets truncated to 'Word' with this patch. The patch might be useful for a second attempt though.

I think you are right that it's going to cause problems for English. The only real fix here is probably to be able to extract the right thing from the desktop files.

See bug 588781, which has a patch to remove the ellipsization and use a more regular layout.  This improves the situation, though at the cost of more space consumed.

Created attachment 138936 [details] [review]
Avoid ellipsizing app names; Draw glow around running

Corresponding with the design, if an application is in a running
state (has > 0 windows open), draw a glow behind the name.

To make the display look a bit nicer, set the width of each item
to be equal to the longest word among all the items.

Overall the patch looks good and works well. I tested launching an application by dragging it to a workspace and staying in the overlay, and the running applications set gets updated accordingly.

It seems to me that overlay.js parts of the patch need to be removed. 
http://bugzilla.gnome.org/show_bug.cgi?id=587781 should fully take care of these changes.

I mostly just had style comments.

+const GLOW_COLOR = new Clutter.Color();
+GLOW_COLOR.from_pixel(0x4F6BA4FF);

I think we usually use lower case for specifying the color.

+            let ellipse = new Shell.DrawingArea({ });

Don't think we need a space here. Perhaps "glow" would be a better name, "ellipse" makes me think of ellipsizing.

+                                                             GLOW_COLOR.red/255,
+                                                             GLOW_COLOR.green/255,
+                                                             GLOW_COLOR.blue/255,
+                                                             GLOW_COLOR.alpha/255);

Need spaces here.

+            let padding = new Big.Box({ });
+            padding.append(stack, Big.BoxPackFlags.NONE);
+            nameBox.append(padding, Big.BoxPackFlags.EXPAND);

No space in the box constructor. We should be able to set padding on nameBox and not need the padding box. In any case, default padding for the box is 0, so I'm not sure what is the purpose of the padding actor here. If it is actually needed, it should have a different name, more suitable for a box :).

One thing I noticed, is that if I set the padding on this box or on nameBox, it gets applied on the left, but not on the right. You should be able to see this with a large padding, e.g. 8. Do you have any idea what could be causing this?

+guint
+shell_global_get_max_word_width (ShellGlobal *global, ClutterActor *ref, const char *text, const char *font)

+void
+shell_global_clutter_cairo_texture_draw_glow (ClutterCairoTexture *texture,
+                                              double red,
+                                              double green,
+                                              double blue,
+                                              double alpha)

Would like to see description headers for both functions. Arguments should each be on a new line and nicely aligned.


+  cairo_translate (cr, width/2.0, height/2.0);
+  cairo_scale (cr, width/2.0, height/2.0);

Need spaces here.

--------------------------------

Not from this patch, but related - I wasn't sure what the last sentence in the ShellStack description from shell-stack.c actually means:

 * It will be sized in width/height
 * according to the largest such dimension of its children, and
 * all children will be allocated that size.  This differs
 * from #ClutterGroup which allocates its children their natural
 * size, even if that would overflow the size allocated to the stack.

The first sentence seems to imply that the stack will get allocated the size of the largest child. I am not sure what the second sentence is saying about how the stack handles overflow.

Updated for comments and pushed, thanks for review!

Regarding the stack, the second sentence is talking about ClutterGroup, not the stack.

This isn't good enough in the short term, or even really in the long term.  We're going to encounter long names, even after we've fixed most of them.

Tentatively, I say we only show the names on hover for the short term.  To deal with long term, one hack that occurs to me is that if we see a .desktop file of the form:

Name=Firefox Web Browser
GenericName=Web Browser

We can just truncate off the GenericName.  This helps for some cases.

Others, like sound-juicer.desktop:

Name=Audio CD Extractor

(no GenericName).  I'm not sure how to handle ones like that.  It's far worse in some translations:

Name[bg]=Извличане на музика от музикални дискове

virt-manager.desktop is similar.  Any ideas on those?

Created attachment 139410 [details] [review]
Always use 4 columns for application well

Add a class ShellHGrid which takes a fixed item width, and centers
its content.  I chose not to modify TidyGrid since how exactly
centering would interact with its myriad modes seemed unwieldy.

Use ShellHGrid, take the given width, divide by four, and use that
as the item width.

Created attachment 139437 [details] [review]
Display full name on hover over application in favorites well

This second patch is not fully working yet.

I'd like to rebase this patch on top of 590778 rather than adding a new C container.

Created attachment 140067 [details] [review]
reimplement layout for app well

patch is WIP

Created attachment 140077 [details] [review]
appDisplay: Reimplement well layout to be width-independent

Use ShellGenericContainer to implement a fully dynamic layout
for the application well.  It's still fixed to 4 columns by default,
but no longer requires a fixed width to be passed in on start.

With another chunk of work, it could likely try to adjust to
the case where we can only fit fewer than 4 items in the well.

Created attachment 140078 [details] [review]
dnd: Fix used of undefined variables

The variables this._yOffset and this._xOffset are included in the
drop coordinates, but as far as I can tell never defined.  Looking
back on the commit that introduced this code, they weren't removed
from anywhere else either.

The drop coordinates seem correct without them, so just delete them.

Generally, like how GenericContainer works out here, we get exactly the behavior we want without having to write a lot of C code.

Seems to work pretty well in practice too once I did a couple of bug fix mentioned below.

I'd like to see some mention in the commit message about the removed code; especially the enter/leave code you removed - not immmediately obvious the connection between that and this patch.

It actually works out for me in nicely testing to have WELL_ITEM_SPACE = 0 rather than 6. Because :

 A) It's not that likely to get two full length names next to each other
 B) It's likely the first name starts with ...

So you almost never have words immediately next to each other even with a spacing of zero.

WHen I dragged a window to the favorites well, the window never re-appeared where I dragged it from, it just vanished. I got a warning of:

(mutter:15604): Clutter-WARNING **: Actor of type ClutterClone is not inside a container

Not sure if that is related.

Stylewise, I don't like:

        this.actor.connect('get-preferred-width', Lang.bind(this, function (grid, forHeight, alloc) {
             /* Bunch of inline code that calls methods on this */
        });

        this.actor.connect('get-preferred-height', Lang.bind(this, function (grid, forWidth, alloc) {
             /* Bunch of inline code that calls methods on this */
        });

+        this.actor.connect('allocate', Lang.bind(this, function (grid, box, flags) {
+            this._doAllocate(box, flags);
+        }));

If you are going to be writing lots of code and calling delegate methods, and the allocate function is already thunked to a single method on the delegate with minor parameter adjustment, then I think it's better to just do:

        this.actor.connect('get-preferred-width', Lang.bind(this, this._getPreferredWidth));
        this.actor.connect('get-preferred-height', Lang.bind(this, this._getPreferredWidth));
        this.actor.connect('allocate', Lang.bind(this, this._allocate));

I think what you have is mixing styles together.

+            let totalVerticalSpacing = Math.max(rows-1, 0) * DEFAULT_PADDING;

Missing space around '-'
Don't think you should be using a constant called DEFAULT_PADDING for the vertical *spacing* in the well box. Padding something that goes on both sides. (Constant isn't commented either.) 
You need some space underneath the separator - right now it has DEFAULT_PADDING above and no space below.

+                columnIndex = columns; /* force a column change */

Think you meant "force a row change", but you already handle this in a different way immediately below.

+            if (i != 0 && (columnIndex == columns || atSeparator)) {
+                rowIndex++;
+                columnIndex = 0;
+            } else {
+                columnIndex++;
+            }
+
+            if (i != 0) {
+                if (columnIndex == 0) {
+                    y += itemHeight + DEFAULT_PADDING;
+                    x = box.x1;
+                } else {
+                    x += itemWidth + WELL_ITEM_SPACING;
+                }
+            }

Clearer to combine the two if (i != 0) - this is the "advance to the next" case, or just move the whole thing to the bottom of the loop where you expect the "advance to the next" to be and don't conditionalize. Moving it to the end would make it more natural to fix the columnIndex computations which are wrong on anything but the first line you count:

 0 1 2 3 4

Start more than 4 non-default apps to see this in action - the 5th won't be properly wrapped down and will overflow.

+            let [childMinWidth, childNaturalWidth, childMinHeight, childNaturalHeight] = children[i].get_preferred_size();

Ordering here is wrong, should be:

+            let [childMinWidth, childMinHeight, childNaturalWidth, childNaturalHeight] = children[i].get_preferred_size();

Which makes the items fill out the space much better.

+        childBox.y2 = childBox.y1+separatorNatural;

Space around operator

+        let horizSpacingTotal = Math.max(nColumns - 1, 0) * WELL_ITEM_SPACING

Missing trailing semicolon
+        let separatorColumns = this._separatorIndex % nColumns;

IUf separatorIndex was 1 we'd want to add 3 columns not 1 column. Should be:

+        let separatorColumns = (nColumns - 1) - (nColumns - 1 + this._separatorIndex) % nColumns;

(A little bit obscure but you can see that it works by checking a few examples.)

+           log("trying to allocate for width " + forWidth + " but min is " + minWidth);

If you leave this in:

 log("WellGrid: FIXME: trying to allocate ...")

So we have some idea where the message is coming from when we see it.


+            let [childMin, childNatural] = children[i].get_preferred_height(forWidth);

Shouldn't be forWidth but rather the width you rae actually going to allocate that item at - which is the min of the itemWidth you compute below and the items naturalWidth. Calling get_preferred_height() with a different width than the allocation a) may cause misbehavior b) may cause inefficiency - for example, layout being multiple times for a PangoLayout.

-        let favoriteIdsObject = arrayToObject(favoriteIds);
+        let favoriteIdsObject = this._arrayToObject(favoriteIds);

Don't really like using "object" to refer to an Object used as a map. 

+        /* hardcode here pending some design about how exactly activities behave */

Existing problem that you are just moving around but the comment should say "contexts" not activities>

+            if (id == null)
+                return false;
+        } else {
+            return false;
+        }

Suggest moving:

 if (id == null)
     return false;

after everything, in case appDisplayItem.getId() starts returning null or something and to simplify.

+            Mainloop.idle_add(function () {
+                appSystem.remove_favorite(id);
+            });

I tend to feel that idle functions should have an explicit return false. (Looking at it, all the places where we have idles do that except for a few that you added recently.) I understand that a Javascript function reliably returns null (or undef?) when it has no return and that's false, and that menas run-once, but I think it's best to be explicit.

Will review the dnd patch separately.

(Don't know how this got NEEDINFO'ed, reopening)

(In reply to comment #15)
> Created an attachment (id=140078) [edit]
> dnd: Fix used of undefined variables
> 
> The variables this._yOffset and this._xOffset are included in the
> drop coordinates, but as far as I can tell never defined.  Looking
> back on the commit that introduced this code, they weren't removed
> from anywhere else either.
> 
> The drop coordinates seem correct without them, so just delete them.

This patch looks OK - the coordinates that should be passed in are the mouse pointer coordinates since that is what we use to determine the drag target. But xOffset and yOffset are also used in:

                    target._delegate.handleDragOver(this.actor._delegate, actor,
                                                    (stageX + this._dragOffsetX + this._xOffset - targX) / target.scale_x,
                                                    (stageY + this._dragOffsetY + this._yOffset - targY) / target.scale_y,
                                                    event.get_time());

So you need to fix that as well to correspond to what you have for the drop.

One final comment about the big patch - I think you should correct for coordinate differences between the outer box and the WellGrid in:

        let dropIsFavorite = this._grid.isBeforeSeparator(x, y);

Even if you only use the y and there is no y offset - otherwise, it's going to be a hard bug to track down later when some padding gets added.

Created attachment 140199 [details] [review]
Define stable ordering for application favorites and running apps

For both of these, because of optimizations a few patches ago, we
ended up relying on hash table ordering which caused instability
in the application well among other things. Define an ordering
for both.

The favorites is just the order of the GConf keys, and new items
get appended.  In the future we should allow insertion at any
point which the grid could use.

For running applications order, define a new "initially_seen_sequence"
transient variable which is just an monotonically incrementing
integer assigned to an application for the first time we saw it
running in this session.  When an application is closed, it's reset.

Created attachment 140200 [details] [review]
appDisplay: Reimplement well layout to be width-independent

Use ShellGenericContainer to implement a fully dynamic layout
for the application well.  It's still fixed to 4 columns by default,
but no longer requires a fixed width to be passed in on start.

With another chunk of work, it could likely try to adjust to
the case where we can only fit fewer than 4 items in the well.

Remove the border highlighting on mouseover, since that caused
reallocations, and the grid layout isn't trivial.

Delete the unused shell_global_get_word_with function.

(In reply to comment #16)
> Generally, like how GenericContainer works out here, we get exactly the
> behavior we want without having to write a lot of C code.

Yeah, it's the new shiny tool.

> It actually works out for me in nicely testing to have WELL_ITEM_SPACE = 0
> rather than 6. Because :

Ok, done for now.

> WHen I dragged a window to the favorites well, the window never re-appeared
> where I dragged it from, it just vanished. I got a warning of:
> 
> (mutter:15604): Clutter-WARNING **: Actor of type ClutterClone is not inside a
> container

Oops, forgot to address this one.  
 
> Don't think you should be using a constant called DEFAULT_PADDING for the
> vertical *spacing* in the well box. Padding something that goes on both sides.

Well, padding can be vertical too.  But fair enough.

> Clearer to combine the two if (i != 0) - this is the "advance to the next"
> case, or just move the whole thing to the bottom of the loop where you expect
> the "advance to the next" to be and don't conditionalize. Moving it to the end
> would make it more natural to fix the columnIndex computations which are wrong
> on anything but the first line you count:

Yeah, I'd fixed this immediately after I submitted.

> +            let [childMinWidth, childNaturalWidth, childMinHeight,
> childNaturalHeight] = children[i].get_preferred_size();
> 
> Ordering here is wrong, should be:

OH!  That would have taken me a while to debug...it had seemed wrong but I was looking at other parts of the code.  Nice catch.

> IUf separatorIndex was 1 we'd want to add 3 columns not 1 column. Should be:
> 
> +        let separatorColumns = (nColumns - 1) - (nColumns - 1 +
> this._separatorIndex) % nColumns;
> 
> (A little bit obscure but you can see that it works by checking a few
> examples.)

OK, I had to add parenthesis here so my brain parsed it correctly, and it's a bit clearer if we name nColumns - 1 to say lastColumnIndex.

> -        let favoriteIdsObject = arrayToObject(favoriteIds);
> +        let favoriteIdsObject = this._arrayToObject(favoriteIds);
> 
> Don't really like using "object" to refer to an Object used as a map. 

Fair enough...I wish we had maps though.
 

Created attachment 140208 [details] [review]
appDisplay: Reimplement well layout to be width-independent

Use ShellGenericContainer to implement a fully dynamic layout
for the application well.  It's still fixed to 4 columns by default,
but no longer requires a fixed width to be passed in on start.

With another chunk of work, it could likely try to adjust to
the case where we can only fit fewer than 4 items in the well.

Remove the border highlighting on mouseover, since that caused
reallocations, and the grid layout isn't trivial.

Delete the unused shell_global_get_word_with function.

Created attachment 140210 [details] [review]
appDisplay: Reimplement well layout to be width-independent

Use ShellGenericContainer to implement a fully dynamic layout
for the application well.  It's still fixed to 4 columns by default,
but no longer requires a fixed width to be passed in on start.

With another chunk of work, it could likely try to adjust to
the case where we can only fit fewer than 4 items in the well.

Remove the border highlighting on mouseover, since that caused
reallocations, and the grid layout isn't trivial.

Delete the unused shell_global_get_word_with function.

(In reply to comment #16)

> (mutter:15604): Clutter-WARNING **: Actor of type ClutterClone is not inside a
> container

I think this is because when we accept a drop, dnd.js does:

                    // If it accepted the drop without taking the actor,
                    // destroy it.
                    if (actor.get_parent() == actor.get_stage())
                        actor.destroy();

How exactly this should all work if you haven't created a specific drag clone I'm not sure, but I think it's better to create one anyways so that we can make it partially transparent, which lets you actually see where you're dropping it.  We should probably scale it down too.  That's in bug 591158.

+const DEFAULT_PADDING = 4;
+const DEFAULT_SPACING = 4;
+
 const APP_ICON_SIZE = 48;
-const APP_PADDING = 18;
+const WELL_DEFAULT_COLUMNS = 4;
+const WELL_ITEM_SPACING = 0;
+const WELL_ITEM_PADDING = APP_ICON_SIZE/4;

You don't use either DEFAULT_PADDING or WELL_ITEM_PADDING. Not sure why DEFAULT_SPACING is the best name for the vertical spacing between rows of items in well.

+            if (columnIndex == columns) {
+                columnIndex = 0;
+                rowIndex++;
+            }
+            if (atSeparator) {
+                columnIndex = 0;
+                rowIndex++;
+            }

Doens't this give you a double rowIndex bump of the separator falls exactly on a column multiple? Which points out that rowIndex is completely unused.

+            let [childMin, childNatural] = children[i].get_preferred_height(itemWidth);
+            if (childNatural > itemNaturalHeight)
+                itemNaturalHeight = childNatural;
+        }

I really think you should use exactly the width that you are going to allocate the item. Calling child.get_preferred_width() here should be pretty cheap since it will be cached. (Oh, on making caching work well, suggest passing -1 always for forHeight to get_preferred_width() - I don't think passing in the forHeight of the container as you do in getItemPreferredWidth() is meaningful - since you are wrapping in rows, only height-for-width is meaningful, not width-for-height.)

Otherwise, looks good to commit.

(In reply to comment #19)
> Created an attachment (id=140199) [edit]
> Define stable ordering for application favorites and running apps
> 
> For both of these, because of optimizations a few patches ago, we
> ended up relying on hash table ordering which caused instability
> in the application well among other things. Define an ordering
> for both.
> 
> The favorites is just the order of the GConf keys, and new items
> get appended.  In the future we should allow insertion at any
> point which the grid could use.
> 
> For running applications order, define a new "initially_seen_sequence"
> transient variable which is just an monotonically incrementing
> integer assigned to an application for the first time we saw it
> running in this session.  When an application is closed, it's reset.

 
+static int
+sort_apps_by_open_sequence (gconstpointer a,
+                            gconstpointer b,
+                            gpointer data)
+{
+  ShellAppMonitor *self = data;
+  const char *id_a = a;
+  const char *id_b = b;
+  AppUsage *usage_a;
+  AppUsage *usage_b;
+
+  usage_a = get_app_usage_for_id (self, id_a);
+  usage_b = get_app_usage_for_id (self, id_b);
+  if (usage_a->initially_seen_sequence == usage_b->initially_seen_sequence)
+    return 0;
+  if (usage_a->initially_seen_sequence < usage_b->initially_seen_sequence)
+    return -1;
+  return 1;
+}

This is going to segfault if shell_app_monitor_get_running_app_ids() for any context other than "" since the apps may not have usages for the context "" which is what get_app_usage_for_id (self, id_a) does. Since we are supporting contexts, I think we have to make them work - so create a little closure structure and pass it in here.

(Acceptable alternative: remove the context from shell_app_monitor_get_running_app_ids() and document as only working for the default context "". Think then get_app_usage_for_id() should be called get_default_context_app_usage_for_id())

Patch otherwise looks good to commit.

(In reply to comment #26)
> 
> This is going to segfault if shell_app_monitor_get_running_app_ids() for any
> context other than "" since the apps may not have usages for the context ""
> which is what get_app_usage_for_id (self, id_a) does.

Well we auto-create usages for apps not seen before in a context (see get_app_usage_for_context_and_id), so it wouldn't segfault, but yeah.

Fixed for all comments and pushed.