Created attachment 154486 [details] [review]
[windowAttention] Add clickable window previews

Created attachment 154697 [details] [review]
[windowAttention] Add clickable window previews

Don't remove previews on update.

Created attachment 154746 [details] [review]
[windowAttention] Add clickable window previews

Some fixes.

Review of attachment 154746 [details] [review]:

I'm still not convinced that the preview is a good idea from a design POV, but then this is a code review, right? Code looks good to me - just one small comment:

::: js/ui/windowAttentionHandler.js
@@ +77,3 @@
         source.notify(notification);
         let notification = new MessageTray.Notification(window.get_startup_id(), source, this._getTitle(app, window), this._getBanner(app, window), true);
+        notification.addActor(source.createPreview(), { row: 2 });

I don't think that the row parameter should be needed ("should" in the sense of "we should fix messageTray") - I think you should check with Dan on the supposed behavior of addActor(), IMO it would be much better to handle the _bannerText case there ...

(In reply to comment #4)
> Review of attachment 154746 [details] [review]:
> 
> I'm still not convinced that the preview is a good idea from a design POV, but
> then this is a code review, right? 

I am interested in hearing why though ... I though your only concern was not being able to click on them.

The idea is that we represent windows using previews all over the place (overview, altTab) ... so having it here too would just be consistent.

>Code looks good to me - just one small
> comment:
> 
> ::: js/ui/windowAttentionHandler.js
> @@ +77,3 @@
>          source.notify(notification);
>          let notification = new
> MessageTray.Notification(window.get_startup_id(), source, this._getTitle(app,
> window), this._getBanner(app, window), true);
> +        notification.addActor(source.createPreview(), { row: 2 });
> 
> I don't think that the row parameter should be needed ("should" in the sense of
> "we should fix messageTray") - I think you should check with Dan on the
> supposed behavior of addActor(), IMO it would be much better to handle the
> _bannerText case there ...

Yes should indeed talk with him about that.

(In reply to comment #5)
> I am interested in hearing why though ...

Fair enough - I'm not quite sure how much of that is rational and what is just some "me not like buaaaah"-feeling ... actually, I'll have to figure out much of it myself.


> The idea is that we represent windows using previews all over the place
> (overview, altTab) ... so having it here too would just be consistent.

Sounds like a good point, so I'm not really sure why it feels that weird to me.

Maybe it's because both in the overview and in alt-tab we use the previews to help differentiate between windows, while there's only one window in the notification - we don't show previews in alt-tab for apps with just one window either ...

Maybe it's because it doesn't seem to add useful information - it shows an empty tab loading a web page after clicking a link outside the browser, a well-known "new document"/empty window for newly started apps (gimp), the main application window when the "interesting" stuff is actually happening in a dialog ...

Maybe it reminds me a little of the old attention-grabbing "oh look at me i'm so important" flash thingy the old panel does - to be fair: to a much, much lesser extend ...

Maybe it's because often the windows update their title while showing the notification, which makes the preview feel somewhat "jumpy" ...


Of course, all of the above is what Jon would call "handwavy" - thumbnails on notifications are not awful, evil or anything alike - it's just that I consider the current text-only ones slightly cleaner; it's a preference, and not a very strong one at that (just in case someone was going to suggest it: an option is not an option - I prefer previews then)

I just hope that Jon does have a stronger opinion (either way) and doesn't join me in handwaving ;)

(In reply to comment #6)
> (In reply to comment #5)
> > I am interested in hearing why though ...
> 
> Fair enough - I'm not quite sure how much of that is rational and what is just
> some "me not like buaaaah"-feeling ... actually, I'll have to figure out much
> of it myself.

OK ;)

> 
> > The idea is that we represent windows using previews all over the place
> > (overview, altTab) ... so having it here too would just be consistent.
> 
> Sounds like a good point, so I'm not really sure why it feels that weird to me.
> 
> Maybe it's because both in the overview and in alt-tab we use the previews to
> help differentiate between windows, while there's only one window in the
> notification 

This is indeed a good point.

>- we don't show previews in alt-tab for apps with just one window
> either ...

Yeah but we do when you press "S" or the down arrow which can be interpreted as the same as "we only show them for expanded notifications" (but this might be a bit far fetched)

> Maybe it's because it doesn't seem to add useful information - it shows an
> empty tab loading a web page after clicking a link outside the browser, a
> well-known "new document"/empty window for newly started apps (gimp), the main
> application window when the "interesting" stuff is actually happening in a
> dialog ...
> 
> Maybe it reminds me a little of the old attention-grabbing "oh look at me i'm
> so important" flash thingy the old panel does - to be fair: to a much, much
> lesser extend ...

Yeah but it makes it easier to decide "do I care now or not", especially when the window has a long title.

> Maybe it's because often the windows update their title while showing the
> notification, which makes the preview feel somewhat "jumpy" ...

This should be fixed if possible ...

> Of course, all of the above is what Jon would call "handwavy" - thumbnails on
> notifications are not awful, evil or anything alike - it's just that I consider
> the current text-only ones slightly cleaner; it's a preference, and not a very
> strong one at that (just in case someone was going to suggest it: an option is
> not an option - I prefer previews then)
> I just hope that Jon does have a stronger opinion (either way) and doesn't join
> me in handwaving ;)

OK, thanks for the input, lets wait for the designer's to opinions ;)

OK, after trying this out...  it is unclear to me how this actually helps.  I think in practice it won't really help with disambiguation and just by being there and popping up every time I try to interact with the banner it will significantly slow down switching to the window.

If we find that the app icon and window name are insufficient or find some other motivation for this we may want to revisit it.  Otherwise, I think adding this is counterproductive.

Thanks for your efforts in bringing it to a state that allowed us to evaluate the ideas.

(In reply to comment #8)
> OK, after trying this out...  it is unclear to me how this actually helps.  I
> think in practice it won't really help with disambiguation and just by being
> there and popping up every time I try to interact with the banner it will
> significantly slow down switching to the window.
> 
> If we find that the app icon and window name are insufficient or find some
> other motivation for this we may want to revisit it.  Otherwise, I think adding
> this is counterproductive.
> 
> Thanks for your efforts in bringing it to a state that allowed us to evaluate
> the ideas.

OK, as noted on IRC one other point is that the preview itself is easier to hit than the icon, but this could probably solved by making the whole banner click able.

Comment on attachment 154746 [details] [review]
[windowAttention] Add clickable window previews

Marking as rejected for now, as per ui-review.

(In reply to comment #9)
> OK, as noted on IRC one other point is that the preview itself is easier to hit
> than the icon, but this could probably solved by making the whole banner click
> able.

which happened. I think the rest of this bug can go away now.