Added MaintainAspectRatio and optimized item width calculation.

[lukasf]

The MaintainAspectRatio property will only be used in cases where the number of items does not fill one row. The name of the property could be changed if someone has a better idea, this is the best I came up with.

I made sure in the calculation that there are no gaps or size jumps when resizing the control. You can check the behavior by setting the property in the sample page and restricting the number of items in the OnNavigatedTo() function (pass a number to PhotoDataSource.GetItemsAsync() overload).

I also optimized the size calculation code to be closer to the desired width by using Math.Round().

[dnfclas]

Hi @lukasf, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@lukasf, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[skendrot]

Wasn't this address with issue #511? If you need the images to maintain aspect ratio why not use a regular GridView? The AdaptiveGridView is intended to fill the area it's given

[lukasf]

In normal cases when using the AdaptiveGridView, items will only be slightly adjusted in width, to fill the available space. The dev specifies the DesiredWidth and the control will try to be close to this width. Only in the special case where there are not enough items, the items are suddenly stretched way beyond the DesiredWidth. In the demo this looks cool, but if you actually want to use this in an app (select an image based on the thumbnail), you will find out that this does not work so well anymore. Identifying the image by looking at the thumbnail does not work anymore if aspect is suddenly 10:1 and 90% of the image is cropped out. I think there are a lot of cases where the normal behavior will be very appreciated, only the extreme stretching does not work well.

With this PR, devs could opt into a different sizing method for this special case. #511 was to always change this behavior, it was closed so fast I could not even respond. So I created a new one to at least make this configurable. After working with the control in two actual apps, I am pretty sure that this would be used a lot.

[deltakosh]

As this is optional and off by default I'm ok to merge it

[ScottIsAFool]

Sample needs updating to reflect this change too.

[ScottIsAFool]

Use nameof() for registering the property

[lukasf]

done

[lukasf]

The effects of setting this property can only be seen with a small number of items. The control panel of the samples gets auto generated somehow. I have not yet found a way how I could add custom controls into there, to change the number of items. The OneRowMode is also not shown in the sample, probably for similar reasons. Can someone point me in the right direction? Alternatively (or additionally) I can add some info on the property into the docs.

[deltakosh]

We cannot demonstrate all features in the sample app. But if you want to demonstrate this feature we need to introduce a button directly in the sample page. (In order to switch between current mode and a OnlyFewObjects mode with MaintainAspectratio on )

[lukasf]

I added documentation and fixed the DP name. I also tried with the sample but failed. Tried adding a new DP on the page for NumberOfItems and binding a slider to it in the .bind file, nothing worked. If someone knows how to do this, please let me know. Otherwise, docs must be sufficient.

[lukasf]

@deltakosh Ah, only saw your comment now. I don't think that the feature is so important that we need to show it on the page (stealing space showcasing the control). If people use the control and find that they have problem with the sizing, they will probably check the docs or properties and then they will find out how to do it.

[dotMorten]

I find the name of this property quite misleading. It's not maintaining the aspect ratio of the items. It's avoiding over-stretching items if there's not enough to fill the first row. Is there a better name to convey this?

[lukasf]

I also not overly happy with the name, but it was the best I could come up with. Maybe we could invert the property and name it something like "StretchItemsToFill" (with a default of 'true')? Any other ideas?

[skendrot]

Why does this property only affect when there is one row? The "problem" of items stretching is still present when there is more than one row. Example: ItemHeight and DesiredWidth is 400 and 533 respectively (4x3 ratio). Width of control is anywhere between 1070 and 1590. The two items on each row stretch very far.
Having a property that only applies during an edge cases adds confusion.

[lukasf]

@skendrot I don't understand why you are so negative on this. The current behavior is inconsistent, and there are a lot of scenarios where the current behavior makes problems.

The docs of AdaptiveGridView don't mention this behavior at all. They read: "The AdaptiveGridView Control presents items in a evenly-spaced set of columns to fill the total available display space. It reacts to changes in the layout as well as the content so it can adapt to different form factors automatically." Nothing in the docs says that all columns must be filled with items or that number of colums will be reduced if there are not enough items. My expectation when reading the docs was: I set a DesiredWidth and then I expect an evenly-spaced set of columns, close to my DesiredWidth, nothing more. I would expect that if there are less items, some of the columns are empty, of course. And this is actually what I get, most of the time. But when I have only one row, the behavior is totally different, and it is not described or documented anywhere. And it is pretty inconsistent: If I have two rows, and there is only one item in the second row, the item is not stretched. But if there is only one item in the first row, it is suddenly stretched, and not only a bit like normal, but extremely. Why is the item extremely stretched when it is the only item in the first row, but not when it is the only item in the second row? Very very inconsistent. Also, when items are added and removed, size pops and changes around. This is not a good UX experience for the end users.

You say adding a property for a corner case adds confusion. But actually, the control adds a whole new sizing behavior for a corner case, and does not even allow users to opt-out of this special behavior. When looking at the code, you can even see that there is this "if" statement, which artificially adds all that behavior for this corner case. In my opinion, this should not have been there in the first place, or if at all, it should have been opt-in from the start, because it adds so much inconsistency. Now I would like to add a property to at least be able to opt-out of this special corner case behavior, and make the control more consistent again.

And for the stretching: My PR also optimizes the normal item sizing, to stay a bit closer to the DesiredWidth. With a usual 4-5 column layout in an app, item width will be +/- 10%. But if there is only one item, width will suddenly be +400%. If there are 10 rows, width is normally +/- 5%, but if there is only one item, the width is +900% (which means in a photo browser that 90% of the thumbnail are cropped away and it is impossible to identify the image anymore). Please allow us to at least add a way to disable this extreme inconsistency.

[skendrot]

@lukasf, my comments are not negative and I'm sorry if they are coming across that way. I am bringing up discussion points. I agree that stretching well beyond the aspect ratio of an image is not ideal. I even pointed out another case where this happens and it will happen in more cases as well.

The items are laid out in a row/column layout, so when there is one item on the second row it fits into the first column of the second row. The item in the second row does not get a "column span" of all of the columns.

I like the idea of this change, but I'd rather see it be the default rather than an opt-in. But I would probably still allow a little stretching so the behavior stays close to that of having multiple rows.

[lukasf]

@skendrot Okay, maybe I got the wrong impression, sorry. I also agree that this behavior should be the new default. In my opinion, it is the more logical, more consistent behavior.

My implementation actually does some amount of stretching. It will calculate the maximum width that this number of items could normally have and will lock them to this width. So for two items, they will be stretched normally up to the exact point where the control would normally start to insert a third row. From there on, their size is locked to this value. So when the size of the control is further increased beyond this point, item width will not "wobble", as it would be the case when strictly following the normal sizing. This makes resizing smooth, and it also makes some more use of the available space due to the mild stretching of items (item width will be a bit wider than with strictly standard sizing).

[deltakosh]

I agree on everything but the default behavior:

• No breaking change
• Some users (Including me) use this control and likes the way it stretches content

[skendrot]

Should it stretch content when there is one item? If yes, then I would say no need for this PR. I'm not a fan of the MaintainAspectRatio property name because it only maintains aspect ratio under one condition. If a better name could be brought in it would be better. Maybe StrecthContentForSingleRow.

[skendrot]

What about a MaxStretchPercentage. The user could decide how much they wish the control to stretch the content. The decision would then be on the user to change this. Example: Have an adaptive trigger that checks if the height of the control is greater than the ItemHeight property value, if it is then MaxStretchPercentage could be 90%, if it is equal to the ItemHeight, the user wold change the value to be 0%

[lukasf]

@deltakosh Agreed, we should not change the UX for existing users of the control.

@skendrot StretchContentForSingleRow sounds good to me. Adding MaxStretchPercentage might be going too far. The control needs a certain amount of stretching also for the normal use. If we add a max percentage, we would need to respect it even for the normal case. This would complicate stuff and add even more corner cases instead of simplifying things. If such precise control is required, it would perhaps be better to make RecalculateLayout virtual (plus ItemWidth protected, _columns can be made local). Then people could inherit and roll their own, customized sizing calculation.

[deltakosh]

Flagging this as 1.2 as I'm sure we will finish it soon

[skendrot]

Started playing with having one item in the sample app and I might be missing something. Having one item in the grid view only stretches to fill the width of the given space IFF DesiredWidth is around half of the given space. So if the DesiredWidth is 200, and the width of the control is 500, the single item is not stretched super far (b/c the column count is two). It does become stretched if the control width is between 200-400. @lukasf, you said items would grow between 400-900%, but I only saw growth less than 100%.

I added the change to use Math.Round when calculating the number of columns and saw much less stretching. The single item would stretch if the control width was between 200-300. I thought this gave a better effect and still allowed for the columns to grow and shrink.

What are thoughts for just leaving the rounding change? Items still grow/shrink as they would when there are multiple rows, but not as excessive. I think having items not grow/shrink when there is one row and then grow when there are two or more rows would be an odd experience. Changing rounding of column count would keep the same user experience, but not stretch items as much (50% at most).

[lukasf]

Are you sure you are looking at the dev branch? Have you perhaps added some MaxWidth somewhere in the ItemTemplate? The control in dev branch will definitely reduce number of columns to 1 if there is only one item, and then the ItemWidth will be set to (available space - 5). Check RecalculateLayout method.

[skendrot]

Sorry. You are right. I had copied your changes locally and completely removed an if block I thought was added:

    // If there's less items than there's columns, reduce the column count;
    if (Items != null && Items.Count > 0 && Items.Count < _columns)
    {
        _columns = Items.Count;
        fillsAtLeastOneRow = false;
    }

I should have removed just the fillsAtLeastOneRow = false; line.

Removing this if statement created the functionality I was describing. I think this is desired behavior rather than keeping items static. So maybe change to:

    if ((MaintainAspectRatio == false) && Items != null && Items.Count > 0 && Items.Count < _columns)
    {
        _columns = Items.Count;
    }

How about MaintainColumnCount or something along the lines of ColumnCount.

I do think this should be the behavior always instead of stretching one item the entire width of the control. This however would introduce a breaking change.

[skendrot]

Rather than checking if there's only one row, enforce the column count by checking your property:

    if ((MaintainAspectRatio == false) && Items != null && Items.Count > 0 && Items.Count < _columns)
    {
        _columns = Items.Count;
    }

[lukasf]

Agreed, done

[skendrot]

Revert these lines in favor of the if check proposed above. This will keep the "stretchy" behavior of the items when there is only one row and maintains the same experience as when there are multiple rows

[lukasf]

Agreed, done

[deltakosh]

up? We are code freezing this friday

[lukasf]

@deltakosh @skendrot Thanks for the latest feedback. I have updated the PR with the requested changes. I have opted for the StretchContentForSingleRow property name, since I think it describes best what actually happens. Docs have been updated.

Since there has been so much debate on the most appropriate sizing behavior, I have also made the size calculation method virtual. I hope that is ok. This way, devs can implement their own sizing strategy if required, and add a "MaxItemStretchFactor" or whatever. Please review the last changes, I hope we can get this into 1.2.

[deltakosh]

Congrats! :)

[lukasf]

Cool, my first PR made it, thank you guys! :)

[skendrot]

Thank you! Great change