Created attachment 236909 [details] [review]
frequentView: Only display as many rows as we can fit

Replace the old fade hack with one that simply removes the icons that
do not fit on inside the allocation.

Review of attachment 236909 [details] [review]:

I did the hack to get the app-picker refresh in in time for UI freeze. I don't consider this change to be bound to the freeze, so we have still some time to do it properly rather than replacing one hack with another one.

::: js/ui/appDisplay.js
@@ +303,3 @@
 
+        // We disable scrolling for this view as we only want to show
+        // a limited number of icons.

The hack is not so much the fade effect, but using a non-scrollable ScrollView in the first place - can we fix this properly then?

@@ +328,3 @@
+
+        // HACK: IconGrid currently lacks API to only display items that match the allocation
+        // so we remove the icons that do not fit afterwards ...

Ugh, that is horrible :-)

IconGrid should get an option to do exactly that - only display items that fit the given allocation.

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

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

Created attachment 237126 [details] [review]
iconGrid: Add limitToParentAllocation and use it for FrequentView

This prevents us from using to many rows causing one being displayed as cut off
at the bottom.


So how about this?
Is it less horrible? ;)

Since it already has a custom xAlign property, maybe this should be called yExpand instead?

Created attachment 237127 [details] [review]
iconGrid: Add limitToParentAllocation property

This property limits the number of displayed icons based on the height of
the parent's allocation. This allows us to display as many icons as we can
fit without overflowing.

Created attachment 237128 [details] [review]
appDisplay: Use limitToParentAllocation for FrequentView

We only want to show as many icons as we can fit.

(In reply to comment #6)
> Since it already has a custom xAlign property, maybe this should be called
> yExpand instead?

Hmm ... don't get this ... the problem has nothing to do with alignment.

(In reply to comment #6)
> Since it already has a custom xAlign property, maybe this should be called
> yExpand instead?

The purpose of the property is to only paint children that fit the parent allocation, e.g. to prevent overallocation in the case that more children are added than fit. That's not what I'd understand by yExpand ...

Maybe instead fix the unnecessary line, to allow scrolling in Frequent?

Mine was just a drive-by comment, really...I see what the code does - the way I thought about it was whether the content makes an effort to expand vertically into the grid regardless of its allocation as opposed to being limited by the parent allocation.
I am fine with whichever name as long as the bug gets fixed, I just thought limitToParentAllocation was a bit weird as a name :)

(In reply to comment #12)
> I am fine with whichever name as long as the bug gets fixed, I just thought
> limitToParentAllocation was a bit weird as a name :)

Yeah, I don't think it's a great name either :-)


(In reply to comment #7)
> iconGrid: Add limitToParentAllocation property
> 
> This property limits the number of displayed icons based on the height of
> the parent's allocation. This allows us to display as many icons as we can
> fit without overflowing.

I wrote a follow-up patch to replace the non-reactive scrollview-with-hidden-scrollbar with a custom widget that underallocates its children as necessary; however, when I was done it finally occurred to me what I didn't like about this approach: for the property to have any effect at all, very specific conditions have to be met (namely: a parent forcing underallocation). I'll attach a proposal for handling that part in IconGrid as well ...

Created attachment 237139 [details] [review]
iconGrid: Add fillParent property

Setting this property will only display as many icons as fit the parent's
allocation, rather than overflowing.

Created attachment 237140 [details] [review]
appDisplay: Remove scroll view hack in FrequentView

The frequent view should not be scrolled, but to work around the
icon grid overflowing, we used a (non-scrolling) scroll view anyway.
Now that IconGrid:fillParent allows us to avoid overflow, we can
remove this hack.

Review of attachment 237139 [details] [review]:

OK this is indeed better.

Review of attachment 237140 [details] [review]:

Looks good and works fine here. (For some reason it does not apply cleanly on master though).

(In reply to comment #17)
> (For some reason it does not apply cleanly on master though).

Ah yes, I did the patches on top of bug 694261, the last one there creates a conflict in the context of the CSS hunk ...

Attachment 237139 [details] pushed as 5f61b57 - iconGrid: Add fillParent property
Attachment 237140 [details] pushed as 7425b38 - appDisplay: Remove scroll view hack in FrequentView