Fix: WrapLayout

[Poker-sang]

Description

• Fix: When all the items in WrapLayout are Collapsed, the method WrapLayoutState.GetHeight() would throw "Nullable object must have a value."
• Fix: Once an item is collapsed in the WrapLayout(such as a Image that is loaded later), it would not be visible without refreshing.
• Other: Some code efficiency optimizations.

closes #70

[Poker-sang]

It seems that this feature has been implemented by RiverFlowLayout. Maybe we can just make WrapLayout Obsoleted.

<RiverFlowLayout
    ItemsJustification="Start"
    ItemsStretch="None"
    LineSize="200"
    LineSpacing="5"
    MinItemSpacing="5" />

[Arlodotexe]

It seems that this feature has been implemented by RiverFlowLayout. Maybe we can just make WrapLayout Obsoleted.

<RiverFlowLayout
    ItemsJustification="Start"
    ItemsStretch="None"
    LineSize="200"
    LineSpacing="5"
    MinItemSpacing="5" />

It looks like RiverFlowLayout is only available in the prerelease versions of WinUI 😕

[Arlodotexe]

@Poker-sang We need you to complete the Contributor License Agreement before we can approve this PR.

[niels9001]

@Arlodotexe @Poker-sang Yeah, and not sure if this will layout will be coming to UWP as well.

[Poker-sang]

@dotnet-policy-service agree

[Poker-sang]

It looks like RiverFlowLayout is only available in the prerelease versions of WinUI 😕

Yes, and I think it will be available in next release, soon. Maybe use #if wsapp to make it obsoleted?

[michael-hawker]

CI issue is due to a problem with the build setup, adding a fix in #178

[Arlodotexe]

@Poker-sang Noticed you submitted this PR directly from the main branch of your fork. That causes a lot of issues, typically it's best to create a branch for every new feature/issue you work on.

When this gets approval, we'll need to cherry-pick the changes into a new branch so we can get this merged in. You'll still receive authorship, but then we'll be able to close this PR out.

[Poker-sang]

@Arlodotexe Thanks for your review! I'm sorry, I probably didn't quite understand what you mean. Do you mean to create a new branch in my fork for each of my following fixes?

• Fix: When all the items in WrapLayout are Collapsed, the method WrapLayoutState.GetHeight() would throw "Nullable object must have a value."
• Fix: Once an item is collapsed in the WrapLayout(such as a Image that is loaded later), it would not be visible without refreshing.
• Other: Some code efficiency optimizations.

If so, I'm concerned that these fixes are more coupled. So it's not easy to separate them.

[niels9001]

@Arlodotexe Thanks for your review! I'm sorry, I probably didn't quite understand what you mean. Do you mean to create a new branch in my fork for each of my following fixes?

• Fix: When all the items in WrapLayout are Collapsed, the method WrapLayoutState.GetHeight() would throw "Nullable object must have a value."
• Fix: Once an item is collapsed in the WrapLayout(such as a Image that is loaded later), it would not be visible without refreshing.
• Other: Some code efficiency optimizations.

If so, I'm concerned that these fixes are more coupled. So it's not easy to separate them.

I think what @Arlodotexe is saying is to just move all the changes into a single branch :).. so instead of commiting from Poker-sang:main you'd have a separate branch in your repo (e.g. Poker-sang/wraplayout-fixes) and you'd use that branch instead to commit from.

[Poker-sang]

I think what Arlodotexe is saying is to just move all the changes into a single branch :).. so instead of commiting from Poker-sang:main you'd have a separate branch in your repo (e.g. Poker-sang/wraplayout-fixes) and you'd use that branch instead to commit from.

Thank you for telling me! I'll create a new branch right away. But I'm not very clear about the benefits of that. Can you explain if it's convenient for you?

[Poker-sang]

Looks like I cannot rename a branch without closing this PR. Should I open a new PR instead?

[niels9001]

Looks like I cannot rename a branch without closing this PR. Should I open a new PR instead?

Yeah let's do that! If you have any review/testing recommendations please add that to the description and I'll look at it once it's up 👍 .

[michael-hawker]

Thank you for telling me! I'll create a new branch right away. But I'm not very clear about the benefits of that. Can you explain if it's convenient for you?

To answer your question here, main is a special branch for a fork, it should really just be used to keep your fork in sync with our main. If you use your main for PRs then it's like re-using a branch over and over again for multiple PRs (which isn't something you'd do). That's the best way to think about it.

So by re-using main to make PRs, GitHub can't easily differentiate what's changed between the repos, so if you reverted something or removed files somewhere, even if they're in sync now, merging a PR across mains can cause those to be reapplied and delete code from our source instead (which is very bad) and doesn't always show up in the diff for the PR. We've had this happen a couple of times in the past, so we're very cognizant of it now. 🙂

[Poker-sang]

@michael-hawker It was the first time I knew there would be these problems! I learnt a lot. Thank you so much for the long reply. I'll be on the lookout for it in all PRs from now on! 😀

[Poker-sang]

LinedFlowLayout is now available in WindowsAppSDK 1.4, not sure if it is coming to UWP as well.