*** Bug 598351 has been marked as a duplicate of this bug. ***

Looks really good already.

Some comments:
 * (from bug #598351) the bubble should stay up for 30 seconds or until I leave the overview - whichever is longer
 * We need more space between the text and the "undo" action.  Maybe 2 spaces?
 * It seems like the undo action is removed before the rest of the text which causes the thing to jump around as it recenters.  This is just as it fades out.

 * When hovering over the undo action we should probably make it prelight.
 * We should probably put up an infobar when the action is undone and keep it there until another action is performed.

Nice work.

Created attachment 152984 [details] [review]
InfoBar in overview.

corrected patch.

Ok, so things are looking pretty good.

The spacing and the bar look great.  I think I'm not totally sold on the inverse for prelighting but it is understandable considering that we haven't figured out what prelighting means in general yet.  I think I'd prefer if it made the text brighter or changed the hue slightly.  Its seems that is all in the css anyway so I think I should just play with it and figure out what we want.

Sorry, I think I may have also steered you wrong with the advice to keep the box around after undoing the action.  It seems kind strange now that I try it.  Maybe we should just hide the bar after the action is undone after all.

Review of attachment 152984 [details] [review]:

Overall, the patch looks good and works as advertised - so the comments are mostly stylistic. Two issues though:

(1) I agree with Mccann: Leaving the bar struck out feels weird
(2) I don't think switching the workspace view should trigger the bar - the user can easily switch back by moving the mouse about 20px. More importantly, the undo message might replace a previous, more important one

OK, the rest is pretty minor:

::: data/theme/gnome-shell.css
@@ +140,3 @@
+    border: 1px solid #5c5c5c;
+    border-radius: 3px;
+.info-bar {

You could instead use
.info-bar StLabel { ... };

That way
(1) the CSS reads explicityly "an StLabel inside .info-bar"
(2) you can create the label simply with new St.Label();

@@ +147,3 @@
+    border: 1px solid #5c5c5c;
+    border-radius: 3px;
+.info-bar {

Color and font-size are redundant and should be removed (if we kept this feature - I agree with Mccann that it did not work out too well)

@@ +151,3 @@
+    border: 1px solid #5c5c5c;
+    border-radius: 3px;
+.info-bar {

Again, I'd slightly prefer
.info-bar StButton
and
.info-bar StButton:hover

::: js/ui/appFavorites.js
@@ +66,2 @@
     },
+    addFavorite: function(appId, dontShowInfoMessage) {

Mmmmh, I don't quite like it:
doSomething(true);
does not exactly look like _dis_abling something.

I understand that by doing this you don't have to modify appDisplay.js, but still, function(appId, enableUndo) makes more sense to me, so I'd rather modify appDisplay ...

Another solution would be to move the original function to a private one (e.g. _add()), and rewrite addFavorite more or less like that:

addFavorite: function(appId) {
    app = Shell.AppSystem.get_default().get_app(appId);
    this._add(app);
    Main.overview.infoBar.setMessage(...,
        Lang.bind(this, function() {
            this._remove(app);
        }));
}

That way you can skip the info bar from the callbacks without modifying the public interface (which is of course utterly important, given the stability of the current API ;)

@@ +77,3 @@
+            Main.overview.infoBar.setMessage(app.get_name() + _(' has been added to your favorites.'), Lang.bind(this, function () {
+        if (!dontShowInfoMessage)
+

That string cannot be translated properly into some languages (they might use something like "Has been removed " + app.getName()). You should use _("%s has been removed").format(app.get_name()) instead.

::: js/ui/overview.js
@@ +99,3 @@
+}
+    this._init();
+function InfoBar() {

I don't see the point of passing an empty object parameter. box.add(this._label) works just fine.

@@ +101,3 @@
+}
+    this._init();
+function InfoBar() {

IIRC we are smart enough to not map actors with 0 opacity, but then I'm not sure ... this.actor.hide() feels somewhat nicer anyway (just make sure to call this.actor.show() before the Tweener animation)

@@ +105,3 @@
+        this._undoCallback = null;
+        this._undo.connect('clicked', Lang.bind(this, this._onUndoClicked));
+        this._undo.hide();

Should not be necessary when hiding the parent container.

@@ +126,3 @@
+    _hideDone: function() {
+        if (this._undo.visible)
+            this._undo.hide();

Again, why not this.actor.hide()?

@@ +176,3 @@
+                         });
+
+        this._timeoutId = Mainloop.timeout_add_seconds(INFO_BAR_HIDE_TIMEOUT, Lang.bind(this, this._onTimeout));

We might consider the case of setMessage() being called while _not_ in the overview. In this case, we should probably queue the message and show it when entering the overlay.

@@ +183,3 @@
+        } else {
+            this._undo.hide();
+            this.undoCallback = null;

Careful: you have
this._undoCallback = undoCallback;
vs.
this.undoCallback = null;
    ^____missing underscore

Of course, this._undoCallback = undoCallback is correct in both cases ...

Created attachment 153029 [details] [review]
InfoBar in overview.

Corrected patch.

> the undo message might replace a previous, more important one

Maybe add button Skip or Ignore?

> We might consider the case of setMessage() being called while _not_ in the
> overview. In this case, we should probably queue the message and show it when
> entering the overlay.

should stay up for 30 seconds or until I leave the overview - whichever is longer
(So already done)

Looks good.

Review of attachment 153029 [details] [review]:

OK, I like it ;)

I found just another tiny little thing though:

::: js/ui/overview.js
@@ +162,3 @@
+        this._overviewWasHide = false;
+
+        this._label.text = text + '  ';

You can just do

this._label.text = text;

and handle the spacing in the CSS:

.info-box StBoxLayout {
    spacing: 10px;
}

(In reply to comment #6)
> > the undo message might replace a previous, more important one
> 
> Maybe add button Skip or Ignore?

Nope, that does not help at all - the previous message will be gone. Simple use case:

(1) Remove favorite
(2) Have second thoughts: wasn't there something on another workspace, so it
    makes sense to keep that favorite a little longer?
(3) Switch to mosaic view - yup, right, undo the removal of the favorite ...
(4) ... only that the undo action now will reverse the view switch.

Discarding the latter message does not bring back the former.


> > We might consider the case of setMessage() being called while _not_ in the
> > overview. In this case, we should probably queue the message and show it when
> > entering the overlay.
> 
> should stay up for 30 seconds or until I leave the overview - whichever is
> longer
> (So already done)

Well, I meant that some undo is triggered before entering the overview - on second thought it should mostly work (except for the timeout) ...
Until something outside the overlay makes use of the info bar we probably shouldn't bother though.


Nevertheless another pair of eyes on the code would be appreciated ;)

(In reply to comment #9)
> (In reply to comment #6)
> > > the undo message might replace a previous, more important one
> > 
> > Maybe add button Skip or Ignore?
> 
> Nope, that does not help at all - the previous message will be gone.

We can store history.

> undo is triggered before entering the overview 

undo can be triggered only by user(other should leave a message in InfoBar).

Review of attachment 153029 [details] [review]:

This looks good and works really well. Some comments on the details of the patch below. For the subject of the commit message, I'd probably use

 Add "InfoBar" and undo capability to overview

::: data/theme/gnome-shell.css
@@ +137,3 @@
+    border-radius: 3px;
+    border: 1px solid #5c5c5c;
+    background: rgba(30,30,30,255);

It's strang to me to use rgba() for an opaque color. I'd probalby just write this as #1e1e1e ,since you use hex colors for everything else

@@ +140,3 @@
+}
+
+.info-bar StLabel {

'.info-bar StLabel' is actually a really expensive selector. For every StLabel in the stage, we have to check all parents to see if any of them have the .info-bar class. You should generally add classes as necessary so that selectors end in specific classes and not in generic actor types.

@@ +142,3 @@
+.info-bar StLabel {
+    color: #cccccc;
+    font-size: 14px;

Looks to me this could juset have been:

.info-bar {
    color: #cccccc;
    font-size: 14px;
}

Since color and font-size will be inherited by all children

@@ +145,3 @@
+}
+
+.info-bar StButton {

Then this becomes:

 .info-bar-link-button {
     text-decoration: underline;
 }

::: js/ui/appFavorites.js
@@ +65,3 @@
     },
 
+    _addFavorite: function(app) {

I don't like having _addFavorite() and then below addFavorite() that takes a different argument. This maybe should be _addFavoriteByApp(). This function needs protection against the app already being in the favorites, otherwise if the GConf key was changed by other means while the undo message was up, you could get duplication.

@@ +86,3 @@
+    },
+
+    _removeFavorite: function(appId) {

See, it gets really confusing here because you have _removeFavorite() with the underscore, but taking the appId. (You probably should make this take an app and be _removeFavoriteByAppp() for consistency)

@@ +99,3 @@
+            let app = Shell.AppSystem.get_default().get_app(appId);
+            if (!app)
+                return;

Hmm, actually, the only time you call _addFavorite() you really wanted to pass in an appId. So, maybe the right split is addFavorite() vs. addFavoriteNonInteractive() with both taking an appId.

::: js/ui/overview.js
@@ +106,3 @@
+
+        this._overviewWasHidden = false;
+        this._timeout = false;

Rather than a separate variable, i think it's simpler to do this._timeoutId != 0

@@ +152,3 @@
+            if (!Main.overview)
+                return;
+            this._hidingOverviewId = Main.overview.connect('hiding', Lang.bind(this, function() {

Just connect this up at the beginning and leave it connected. The tiny performance gain isn't worth complexity.

Since onTimeout is a standalone callback, I'd make this a standalone callback too for symmetry, since they are closely related functions.

@@ +160,3 @@
+        }
+        this._timeout = false;
+        this._overviewWasHide = false;

It's (correctly) overviewWasHidden elsewhere

@@ +335,3 @@
         this._dash.searchResults.actor.height = this._workspacesHeight;
 
+        this.infoBar.actor.set_position(this._workspacesX, this._workspacesY - 2 * contentY);

I don't see what contentY has to do with the positioning here - you seem to be counting on the fact that contentY == Panel.PANEL_HEIGHT and that ends up looking good. My suggestion here is to set the size of the infoBar here to be the entire space above the workspaces beneath the panel - no vertical padding, no vertical alignment. Then use the facilities of StBin/StBox and CSS to align the content at the bottom and add an appropriate amount of padding below.

::: js/ui/workspacesView.js
@@ +849,3 @@
+        if (showUndoMessage)
+            Main.overview.infoBar.setMessage(_('Workspaces view has been changed.'),
+                                             Lang.bind(this, function() {

This is a good demonstration of the generality of your framework, but I don't really think it adds much for the user. Removing a favorite is something that needs an undo, because otherwise you'd have to remember what you removed and find it in the applications browse. But for this case you can just move your mouse a few pixels over and click on the other icon. So, I think this part of the patch should be removed... it's unnecessary visual noise - we possible we want the user to focus on what happened, not on text describing what happened.

Created attachment 153049 [details] [review]
Add "InfoBar" and undo capability to overview

Corrected patch

Review of attachment 153049 [details] [review]:

Looks good except for cosmetics noted below. Feel free to commit once you fix those.

::: js/ui/overview.js
@@ +92,3 @@
+        let bin = new St.Bin({ style_class: 'info-bar' });
+
+        this.actor = new St.Bin({ style_class: 'info-bar-panel' });

The ordering here makes this function a bit hard to read - I'd suggest creating the actors from outermost to innermost and packing them right after you create and configure them.

  this.actor = new St.Bin({ style_class: 'info-bar-panel' });
  this.actor.set_fill(true, false);
  
  let bin = new St.Bin({ style_class: 'info-bar' });
  bin.set_fill(false, false);
  bin.set_alignment(St.Align.MIDDLE, St.Align.MIDDLE);
  this.actor.set_child(bin);

  let box = new St.BoxLayout();
  bin.set_child(box);

  this._label = new St.Label();
  [...]

Actually, 

  let bin = new St.Bin({ style_class: 'info-bar' });
  bin.set_fill(false, false);
  bin.set_alignment(St.Align.MIDDLE, St.Align.MIDDLE);

is better as:

  bin = new St.Bin({ style_class: 'info-bar',
                     x-fill: true,
                     y-fill: false,
                     x-align: St.Align.MIDDLE,
                     y-align: St.Align.MIDDLE });

In my opinion.

@@ +102,3 @@
+        bin.set_fill(false, false);
+        bin.set_alignment(St.Align.MIDDLE, St.Align.MIDDLE);
+        this._timeoutId = 0;

This shouldn't be mixed in with packing, it should be down further.

@@ +160,3 @@
+
+        if (this._hidingOverviewId == 0) {
+            // Set here, because when constructor is call, overview is null.

nitpick: 'is called'

@@ +163,3 @@
+            if (!Main.overview)
+                return;
+            this._hidingOverviewId = Main.overview.connect('hiding', Lang.bind(this, this._onOverviewHiding));

The use of '_hidingOverviewId' sends a message that we're going to disconnect it somewhere, since we aren't, this needs to be commented.

  // We don't actually use the ID, it's just a way of tracking whether we've hooked up the signal

@@ +180,3 @@
+        this._undoCallback = undoCallback;
+        if (undoCallback) {
+            this._undo.show();

I don't think we have a firm style, but I find it ugly to have the excess braces here on an if/else where both the if and the else are single line.