Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Upgrading OnIdiom<T>#4025

Closed
MartinZikmund wants to merge 4 commits intoxamarin:masterfrom
MartinZikmund:onidiom-default
Closed

Upgrading OnIdiom<T>#4025
MartinZikmund wants to merge 4 commits intoxamarin:masterfrom
MartinZikmund:onidiom-default

Conversation

@MartinZikmund
Copy link
Copy Markdown
Contributor

@MartinZikmund MartinZikmund commented Oct 7, 2018 •
edited
Loading

Description of Change

The OnIdiom<T> was not matching the format of OnPlatform<T>. This PR attempts to upgrade OnIdiom to include matching functionality and more friendly XAML interface.

Issues Resolved

API Changes

Added:

  • T OnIdiom<T>.Default { get; set; }
  • IList<OnIdiomSetting> OnIdiom.Idioms { get; private set; }

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

The legacy syntax for OnIdiom is still supported and there should be no change required for existing apps.

PR adds support for Default value, which is on par with {OnIdiom} markup extension and supports even the legacy syntax.

The new syntax is very similar to the one provided by OnPlatform<T>:

<BoxView WidthRequest="100" HeightRequest="100">
	<BoxView.BackgroundColor>
		<OnIdiom x:TypeArguments="Color" Default="Red">
			<OnIdiomSetting Idiom="Phone" Value="Blue" />
			<OnIdiomSetting Idiom="TV, Tablet" Value="Green" />
		</OnIdiom>
	</BoxView.BackgroundColor>
</BoxView>

The only disadvantage is that it is not possible to reuse <On> as in OnPlatform<T>, because both reside in the same namespace. It would be possible to add Idiom property to On but that could be confusing to users.

Before/After Screenshots

Not applicable

Testing Procedure

I have created demonstration of the Default value on Issue4006 page in controls gallery.

Functionality of OnIdiom<T> has been covered by tests in Xamarin.Forms.Core.UnitTests.OnIdiomTests.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@MartinZikmund MartinZikmund changed the title Upgrading OnIdiom Upgrading OnIdiom<T> Oct 7, 2018
StephaneDelcroix
StephaneDelcroix suggested changes Oct 8, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep this simple, and do

case TargetIdiom.Phone:
    return onIdiom.Phone ?? onIdiom.Default;

@MartinZikmund
Copy link
Copy Markdown
Contributor Author

MartinZikmund commented Oct 8, 2018

@StephaneDelcroix Is there a particular reason for this? The original syntax with explicit property per idiom is still supported, but I thought it would be meaningful to have both OnX expressions have matching features - including specifying same value for multiple platforms.

The use of Dictionary for storing the value for "legacy" syntax is useful to simplify the code - I can quickly check if any of the "idiom-specific" properties has been set, while keeping a common setter code.

@StephaneDelcroix StephaneDelcroix mentioned this pull request Oct 16, 2018
Merged
3 tasks
@PureWeen PureWeen requested a review from hartez October 16, 2018 17:49
@PureWeen PureWeen assigned hartez and unassigned jassmith Oct 16, 2018
@PureWeen PureWeen requested review from samhouts and removed request for hartez and jassmith October 16, 2018 17:49
@PureWeen PureWeen assigned samhouts and unassigned hartez Oct 16, 2018
samhouts
samhouts suggested changes Oct 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@samhouts samhouts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rebase. Thank you!

Comment thread Xamarin.Forms.Core/OnIdiom.cs Outdated
Comment thread Xamarin.Forms.Core/OnIdiom.cs
@xamarin-release-manager xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label Oct 17, 2018
@MartinZikmund
Copy link
Copy Markdown
Contributor Author

MartinZikmund commented Oct 24, 2018

@samhouts In that case I will make it a for, but - could you please send me some resource for this? I have never heard about performance issues of foreach and LINQ so I am really curious, and it seems there are quite a few foreach statements in the repo as well

StephaneDelcroix
StephaneDelcroix suggested changes Oct 24, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing feature, as described in #4006, is to add a default Value. There's no point in doing the changes we did for OnPlatform (moving from an enum to a string) as the OnIdiom enum hasn't been deprecated.

@MartinZikmund
Copy link
Copy Markdown
Contributor Author

MartinZikmund commented Oct 24, 2018

I understand this, but this syntax is much more powerful as it allows to define the same value for multiple idioms at the same time, without possible unnecessary duplication, as shown in the sample. With the existing syntax (which is still available) we can only set the values one by one. I find this more convenient and valuable. But if you don't share this view, I can revert it back.

StephaneDelcroix added a commit that referenced this pull request Oct 26, 2018
@StephaneDelcroix
[C] Add Default value to OnIdiom
b315a1e 
- fixes #4006
- closes #4025
@StephaneDelcroix StephaneDelcroix mentioned this pull request Oct 26, 2018
Merged
3 tasks
@samhouts
Copy link
Copy Markdown
Contributor

samhouts commented Oct 26, 2018

@MartinZikmund Thank you for this PR! Please take a look at #4225 and let us know if it provides the same flexibility you're going for here.

@MartinZikmund
Copy link
Copy Markdown
Contributor Author

MartinZikmund commented Oct 26, 2018 •
edited
Loading

@samhouts The reasons for the new syntax still hold, as it closely mirrors how OnPlatform has evolved and offers more flexibility beyond just offering Default value, see the previous comment I have posted. I think the unit test I have added in the PR shows the usefulness as well. But anyway, if you don't see the new feature as useful, it is okay too... I just didn't get an explanation on why I should remove the new syntax from in the conversation so far so that is why I was trying to explain it in the comments :-)

@samhouts samhouts added the e/1 🕐 1 label Nov 2, 2018
@PureWeen PureWeen assigned hartez and unassigned samhouts Nov 2, 2018
@PureWeen PureWeen requested a review from hartez November 2, 2018 22:24
StephaneDelcroix added a commit that referenced this pull request Nov 5, 2018
@StephaneDelcroix
[C] Add Default value to OnIdiom
a363ced 
- fixes #4006
- closes #4025
@PureWeen PureWeen closed this in 885fa9a Nov 6, 2018
@samhouts samhouts added hacktoberfest 🍻 in-progress This issue has an associated pull request that may resolve it! labels Oct 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hartez hartez Awaiting requested review from hartez

2 more reviewers

@samhouts samhouts samhouts requested changes

@StephaneDelcroix StephaneDelcroix StephaneDelcroix requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephaneDelcroix StephaneDelcroix

@hartez hartez

Labels

a/Xaml </> API-change Heads-up to reviewers that this PR may contain an API change e/1 🕐 1 hacktoberfest 🍻 in-progress This issue has an associated pull request that may resolve it! t/enhancement ➕

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OnIdiom<T> and {OnIdiom} discrepancies.

7 participants

@MartinZikmund @samhouts @StephaneDelcroix @jassmith @hartez @xamarin-release-manager @PureWeen