Created attachment 254003 [details] [review]
appDisplay: Use a ScrollView for pages

While we obviously don't want any scrollbars, its fade effect
will give us some extra polish when switching pages.

Review of attachment 254003 [details] [review]:

Looks good.

Created attachment 254042 [details] [review]
Fix for small displays

Review of attachment 254003 [details] [review]:

lgtm if you apply the patch I attached or something like that =)

Review of attachment 254042 [details] [review]:

What exactly does this fix? Also the commit message is a bit vague. And if we want to do that it should probably be squashed into Florian's patch.

::: js/ui/appDisplay.js
@@ +260,3 @@
     _init: function() {
         this.parent({ usePagination: true }, null);
+        this._scrollView = new St.ScrollView({ style_class: 'all-apps all-apps-scrollview',

Why did you introduce a new class for that? 'all-apps' is not used by anything else so could add the offset there.

drago01, yeah, I mean to be squashed with Florian patch of course. Sorry I didn't say anything, I point out Florian about that in the chat.
The fade is visible in  small resolutions, so I put a more little offset, 30px, due to https://git.gnome.org/browse/gnome-shell/tree/data/theme/gnome-shell.css#n910

Created attachment 254043 [details] [review]
theme: Don't show vfade effect on small displays

Created attachment 254044 [details] [review]
appDisplay: Use a ScrollView for pages

How about this instead?

(In reply to comment #6)
> The fade is visible in  small resolutions [..]

I still have no idea what that means. Why shouldn't it be visible on small resolutions? And how does the offset have to do with that?

Can you tell me what the actual problem is and not just provide solutions? (Maybe a screenshot showing it would help).

Review of attachment 254044 [details] [review]:

That's "more" correct than my patch. With your comment in the code about that seems enough readable to understand what we are doing.

::: js/ui/appDisplay.js
@@ +269,3 @@
+        // Set vfade style class so ScrollView creates the effect; the
+        // actual offset is handled programmatically in adaptToSize().
+        this._scrollView.style_class += ' vfade';

why that and not style_class: 'all-apps vfade' ?
Feel free to ignore my comment if you think it's ok

(In reply to comment #9)
> (In reply to comment #6)
> > The fade is visible in  small resolutions [..]
> 
> I still have no idea what that means. Why shouldn't it be visible on small
> resolutions? And how does the offset have to do with that?
> 
> Can you tell me what the actual problem is and not just provide solutions?
> (Maybe a screenshot showing it would help).

Of course.
The problem is that vfade offset is too big, so in small resolutions where the spacing between the corner of the view and the grid itself is the minimum (30px), due that the default vfade offset is 68px, it becomes visible.
So we have to reduce that offset to 30px, to ensure that the vfade effect is never visible (if we are not changing pages of course)
http://postimg.org/image/98gqa3k6n/full/
You can see it on the folder icons for example.

Hope I explained well now

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > The fade is visible in  small resolutions [..]
> > 
> > I still have no idea what that means. Why shouldn't it be visible on small
> > resolutions? And how does the offset have to do with that?
> > 
> > Can you tell me what the actual problem is and not just provide solutions?
> > (Maybe a screenshot showing it would help).
> 
> Of course.
> The problem is that vfade offset is too big, so in small resolutions where the
> spacing between the corner of the view and the grid itself is the minimum
> (30px), due that the default vfade offset is 68px, it becomes visible.
> So we have to reduce that offset to 30px, to ensure that the vfade effect is
> never visible (if we are not changing pages of course)
> http://postimg.org/image/98gqa3k6n/full/
> You can see it on the folder icons for example.
> 
> Hope I explained well now

Oh ok ... then Florian's approach of using a dynamic instead of a hard coded value indeed makes sense.

Review of attachment 254044 [details] [review]:

::: js/ui/appDisplay.js
@@ +269,3 @@
+        // Set vfade style class so ScrollView creates the effect; the
+        // actual offset is handled programmatically in adaptToSize().
+        this._scrollView.style_class += ' vfade';

Well you can just use scrollview.update_fade_effect(offset, 0); i.e setting the offset will create the effect.

@@ +547,3 @@
+        let fadeOffset = Math.min(this._grid.topPadding,
+                                  this._grid.bottomPadding);
+        this._scrollView.get_effect('fade').vfade_offset = fadeOffset;

this._scrollView.update_fade_effect(fadeOffset, 0)

Created attachment 254056 [details] [review]
st: Make st_scroll_view_update_fade_effect() public

Using fixed fade offsets is not always appropriate, this will allow
to set them from code instead.


(In reply to comment #13)
> Review of attachment 254044 [details] [review]:
> 
> Well you can just use scrollview.update_fade_effect(offset, 0); i.e setting the
> offset will create the effect.

... except that update_fade_effect() is not public :-)

But yes, considering that we won't need to worry about StWidget::style-changed and AllView.adaptToSize() fighting over the offsets, I prefer this approach.

Created attachment 254057 [details] [review]
appDisplay: Use a ScrollView for pages

(In reply to comment #10)
> +        // Set vfade style class so ScrollView creates the effect; the
> +        // actual offset is handled programmatically in adaptToSize().
> +        this._scrollView.style_class += ' vfade';
> 
> why that and not style_class: 'all-apps vfade' ?

Basically just because it allows me to add the comment in context without exceeding the 80-character limit (e.g. the comment has a lot less indentation here than it would have in the property list).
It's not a good reason though, so I would have changed that back if it was still present in the current version ...

Review of attachment 254056 [details] [review]:

Oh I though this was already public...  looks good.

Review of attachment 254057 [details] [review]:

LG.

Created attachment 254059 [details] [review]
appDisplay: Fix return value for _onScroll

ClutterActor::scroll-event has a boolean return value to indicate
whether the event has been handled, or event emission should continue.
Now that we are using an StScrollView, we depend on this to avoid
propagating the event to the view's own handler.

Review of attachment 254059 [details] [review]:

OK.

::: js/ui/appDisplay.js
@@ +403,2 @@
     _onScroll: function(actor, event) {
+        if(this._displayingPopup)

Missing space (has been wrong before but fix while you are touching it)

Created attachment 254127 [details] [review]
st: Add StScrollViewFade:fade-edges

Add a new property which controls whether edge areas are excluded
from the effect (the default and current behavior), or not.


This is my shot at fixing the fade effect for translated children as well (e.g. when moving out of the way to make space for an opening folder).
It's on top of bug 707508, though the patch could be rewritten to not depend on this.

Created attachment 254128 [details] [review]
appDisplay: Extend fade effect to edges

We want the fade effect to apply to icons that are translated to
make space for a folder popup, so we have to make sure it does not
exclude the edges.

(meant to be squashed with the "Use a ScrollView for pages" patch)

Review of attachment 254127 [details] [review]:

::: src/st/st-scroll-view-fade.glsl
@@ +46,3 @@
         float fade_bottom_start = fade_area_bottomright[1] - vfade_offset;
         float fade_right_start = fade_area_bottomright[0] - hfade_offset;
+        bool fade_top = y < vfade_offset && (fade_edges ? vvalue >= 0.0

Hmm ... this might reintroduce bug 696404 ... unfortunately I don't have hardware with such low instruction count limits to test it.

Review of attachment 254128 [details] [review]:

OK if the other patch does not hit the instruction limit issue.

Created attachment 254131 [details] [review]
st: Add StScrollViewFade:fade-edges

Whoops, messed up when cleaning up the fader code.

Review of attachment 254131 [details] [review]:

<airlied> drago01: not really sure there is a nice way, since drivers enable different thinsg
<airlied> that cause different results
<drago01> airlied: hmm ok
<drago01> airlied: ok lets ask differently then how "bad" in terms of instruction count is something like "(fade_edges ? vvalue >= 0.0 : vvalue > 0.0)" (4 times in the code)
<airlied> find someone with a pineview :-)
<airlied> drago01: again it really depends on the hw!
<airlied> since there are no generic instructions
<drago01> airlied: ok
<drago01> airlied: ok maybe "lets see if people file bugs" is the best option then ;)
<airlied> submit a piglit test with the shader :)


---

So if you are fine with the trial and error approach go ahead ;)

(In reply to comment #25)
> So if you are fine with the trial and error approach go ahead ;)

I am, but would we fair better with:

  if (fade_edge) {
      fade_top = ...;
      fade_bottom = ...;
      ...
  } else {
      fade_top = ...;
      fade_bottom = ...;
      ...
  }

(In reply to comment #26)
> (In reply to comment #25)
> > So if you are fine with the trial and error approach go ahead ;)
> 
> I am, but would we fair better with:
> 
>   if (fade_edge) {
>       fade_top = ...;
>       fade_bottom = ...;
>       ...
>   } else {
>       fade_top = ...;
>       fade_bottom = ...;
>       ...
>   }

Not sure I'd assume the compiler to be smart enough to stuff like that for us.

Attachment 254056 [details] pushed as 6fb044f - st: Make st_scroll_view_update_fade_effect() public
Attachment 254057 [details] pushed as 36bee16 - appDisplay: Use a ScrollView for pages
Attachment 254059 [details] pushed as 2980515 - appDisplay: Fix return value for _onScroll
Attachment 254131 [details] pushed as 4f5d3e0 - st: Add StScrollViewFade:fade-edges

OK, let's find out the next test day then ...