Xamarin.forms: [Enhancement] Color extension or AppThemeColor

Created on 5 Jun 2020  Â·  13Comments  Â·  Source: xamarin/Xamarin.Forms

Summary

This commit (https://github.com/xamarin/Xamarin.Forms/commit/b6e323d1576c0f6746ad9f4c7b77fe3f5af8e44d) removed the way to set color depends on theme

API Changes

<Color x:Key="ButtonBorderColor" Value="{AppThemeBinding Dark=#e6e6e6, Light=#191919}"/>

or

<Color x:Key="BackgroundColor1">
        <AppThemeBinding Dark="#232323" Light="#dcdcdc" />
 </Color>

or 

<AppThemeColor x:Key="MyColor" Dark="Red" Light="Blue" />

Intended Use Case

<AppThemeColor x:Key="MyColor" Dark="..." Light=".." />
<Style
        x:Key="Style1"
        TargetType="{x:Type Label}">
        <Setter Property="BackgroundColor" Value="{DynamicResource MyColor}" />
    </Style>
<Style
        x:Key="Style2"
        TargetType="{x:Type Button}">
        <Setter Property="BackgroundColor" Value="{DynamicResource MyColor}" />
    </Style>

instead of code duplication:

<Style
        x:Key="Style1"
        TargetType="{x:Type Label}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark="Red", Light="Green"}" />
    </Style>
<Style
        x:Key="Style2"
        TargetType="{x:Type Button}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark="Red", Light="Green"}" />

    </Style>
darkmode in-progress proposal-open enhancement âž•

Most helpful comment

@CZEMacLeod original naming was wrong and caused unrealistic expectations and a wrong mental representation of what this is, and what this does.

Using a style is the correct way to handle this. We plan to bring that capability to our CSS support as well.

But there's definitely a wrong way of doing this. It is possible to define the AppThemeBinding as non-shared resource, and reuse it. If someone asks, I've never told you about it, and never wrote the following snippet of code:

<ContentPage.Resources>
  <AppThemeBinding x:Key="FancyColor" x:Shared="false">
    <AppThemeBinding.Light><Color>HotPink</Color></AppThemeBinding.Light>
    <AppThemeBinding.Light><Color>LimeGreen</Color></AppThemeBinding.Light>
  </AppThemeBinding>
</ContentPage.Resources>

<StackLayout>
  <Label BackgroundColor="{StaticResource FancyColor}"/>
  <Label BackgroundColor="{StaticResource FancyColor}"/>
</StackLayout>

This is supposed to work, but I haven't tested it (please report if it doesn't in a separate issue, and I'll fix it). Why isn't it recommended ? Because the mental model makes you think you're sharing a sort of magical color, while you're sharing a binding.

All 13 comments

The simplest solution: make OnAppTheme<T> public instead of internal

none of the proposal is viable or will achieve the expected behaviour

<Color x:Key="ButtonBorderColor" Value="{AppThemeBinding Dark=#e6e6e6, Light=#191919}"/>

color value is not bindable, so it won't work

or

using AppThemeColor as a resource, then reusing it won't work either. it's a Binding, Binding can't be shared by instance. You can set a Binding to multiple properties using Style, as Style will clone the binding at the time it's applied.

this is why making OnAppTheme public will not get you any further.

The way to avoid duplication is to use Style, as you did.

<Style
        x:Key="Style1"
        TargetType="{x:Type Label}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark="Red", Light="Green"}" />
    </Style>
<Style
        x:Key="Style2"
        TargetType="{x:Type Button}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark="Red", Light="Green"}" />

    </Style>

@StephaneDelcroix

public class AppThemeColor : OnAppTheme<Color>
    {
        public static implicit operator Color(AppThemeColor appThemeColor)
        {
            return appThemeColor.GetValue();
        }

        private Color GetValue()
        {
            return Application.Current.RequestedTheme switch
            {
                OSAppTheme.Dark => Dark,
                OSAppTheme.Light => Light,
                _ => Default,
            };
        }
    }

@VladislavAntonyuk that is unlikely to update the color on theme change

@StephaneDelcroix but I still have code duplication:
Background color is the same for each style, for each target type. Are there any workarounds to resolve it?

The way to avoid duplication is to use Style, as you did.

<Style
        x:Key="Style1"
        TargetType="{x:Type Label}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark="Red", Light="Green"}" />
    </Style>
<Style
        x:Key="Style2"
        TargetType="{x:Type Button}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark="Red", Light="Green"}" />

    </Style>

@VladislavAntonyuk you had code duplication before, same setter applied to 2 different types. this doesn't change a thing. you can probably have a base style that targets VisualElement...

@StephaneDelcroix, No, I had

<AppThemeColor x:Key="Color1" Dark="Red" Light="Green"/>

<Style
        x:Key="Style1"
        TargetType="{x:Type Label}">
        <Setter Property="BackgroundColor" Value="{DynamicResource Color1}" />
    </Style>
<Style
        x:Key="Style2"
        TargetType="{x:Type Button}">
        <Setter Property="BackgroundColor" Value="{DynamicResource Color1}" />

    </Style>

So if I need to change the color from Dark Color from Red to Blue, I need to change it only in one place.

@StephaneDelcroix well, I find a solution:

<Color x:Key="Color1">#123456</Color>
<Color x:Key="Color2">#654321</Color>
<Style
        x:Key="ElementCommonStyle"
        ApplyToDerivedTypes="true"
        TargetType="{x:Type VisualElement}">
        <Setter Property="BackgroundColor" Value="{AppThemeBinding Dark={StaticResource Color1}, Light={StaticResource Color2}}" />
    </Style>

@VladislavAntonyuk Yeah - that is about as good as it (now) gets.
It still results in replicating the AppThemeBinding repeatedly throughout the code, and means that if I am adding theming support I can't just find/replace {StaticResource PrimaryColor} with {DynamicResource PrimaryColor} and replace <Color x:Key="PrimaryColor" ... with <AppThemeColor x:Key="PrimaryColor" ...
I'm not sure why they couldn't fix the bugs in the original design, and the new AppThemeBinding is useful for 1 off logos and images etc., but the overall result is definitely less user friendly and more prone to error.
@StephaneDelcroix Is there no way to bring back a version of AppThemeColor that remove/replaces the mess that is {AppThemeBinding Dark={StaticResource Color1}, Light={StaticResource Color2}} everywhere I want to use a themed colour?

@CZEMacLeod original naming was wrong and caused unrealistic expectations and a wrong mental representation of what this is, and what this does.

Using a style is the correct way to handle this. We plan to bring that capability to our CSS support as well.

But there's definitely a wrong way of doing this. It is possible to define the AppThemeBinding as non-shared resource, and reuse it. If someone asks, I've never told you about it, and never wrote the following snippet of code:

<ContentPage.Resources>
  <AppThemeBinding x:Key="FancyColor" x:Shared="false">
    <AppThemeBinding.Light><Color>HotPink</Color></AppThemeBinding.Light>
    <AppThemeBinding.Light><Color>LimeGreen</Color></AppThemeBinding.Light>
  </AppThemeBinding>
</ContentPage.Resources>

<StackLayout>
  <Label BackgroundColor="{StaticResource FancyColor}"/>
  <Label BackgroundColor="{StaticResource FancyColor}"/>
</StackLayout>

This is supposed to work, but I haven't tested it (please report if it doesn't in a separate issue, and I'll fix it). Why isn't it recommended ? Because the mental model makes you think you're sharing a sort of magical color, while you're sharing a binding.

@StephaneDelcroix Thanks for the snippet. I am happy to use FancyColorBinding as the key name to keep the mental modal right and use Styles - but it is definitely easier to define each colour binding once, and reuse it in each style as per the original AppThemeColor. I will give that a go in a bit.

For now I used the RequestedThemeChanged event with multiple resource dictionaries built at runtime from my own AppThemeColor implementation.
This is loosely based on Theme a Xamarin.Forms Application
I've tested and it seems to work for my purposes (on android).
@VladislavAntonyuk This might be useful to anyone migrating as a temporary step.

using System.Collections;
using System.Collections.Generic;
using System.Linq;
using Xamarin.Forms;

namespace AppThemes
{
    public interface IAppThemeResource
    {
        object Light { get; }
        object Dark { get; }
        object Default { get; }
    }

    public class AppThemeResource<T> : IAppThemeResource
    {
        public T Light { get; set; }
        public T Dark { get; set; }
        public T Default { get; set; }
        object IAppThemeResource.Light => Light;
        object IAppThemeResource.Dark => Dark;
        object IAppThemeResource.Default => Default;
    }

    public class AppThemeColor : AppThemeResource<Color>
    {
    }

    public class AppThemeResources : IReadOnlyDictionary<OSAppTheme, ResourceDictionary>
    {
        private readonly Dictionary<OSAppTheme, ResourceDictionary> _themedResources;
        private readonly ICollection<ResourceDictionary> _mergedDictionaries;
        public AppThemeResources(Application application)
        {
            _themedResources = new Dictionary<OSAppTheme, ResourceDictionary>
            {
                { OSAppTheme.Unspecified, new ResourceDictionary() },
                { OSAppTheme.Light, new ResourceDictionary() },
                { OSAppTheme.Dark, new ResourceDictionary() },
            };
            ResourceDictionary resources = application.Resources;
            _mergedDictionaries = resources.MergedDictionaries;
            var appThemeResources = resources.Where(kvp => kvp.Value is IAppThemeResource).ToList();
            foreach (var kvp in appThemeResources)
            {
                var key = kvp.Key;
                var tr = (IAppThemeResource)kvp.Value;
                _themedResources[OSAppTheme.Unspecified][key] = tr.Default;
                _themedResources[OSAppTheme.Light][key] = tr.Light;
                _themedResources[OSAppTheme.Dark][key] = tr.Dark;
                resources.Remove(key);
            }
            application.RequestedThemeChanged += (s, a) =>
            {
                foreach (var d in _themedResources.Values)
                {
                    _mergedDictionaries.Remove(d);
                }
                _mergedDictionaries.Add(_themedResources[a.RequestedTheme]);
            };
            _mergedDictionaries.Add(_themedResources[application.RequestedTheme]);
        }

        #region "IReadOnlyDictionary"
        public ResourceDictionary this[OSAppTheme key] => 
            ((IReadOnlyDictionary<OSAppTheme, ResourceDictionary>)_themedResources)[key];

        public IEnumerable<OSAppTheme> Keys => 
            ((IReadOnlyDictionary<OSAppTheme, ResourceDictionary>)_themedResources).Keys;

        public IEnumerable<ResourceDictionary> Values => 
            ((IReadOnlyDictionary<OSAppTheme, ResourceDictionary>)_themedResources).Values;

        public int Count => ((IReadOnlyCollection<KeyValuePair<OSAppTheme, ResourceDictionary>>)_themedResources).Count;

        public bool ContainsKey(OSAppTheme key) => 
            ((IReadOnlyDictionary<OSAppTheme, ResourceDictionary>)_themedResources).ContainsKey(key);
        public IEnumerator<KeyValuePair<OSAppTheme, ResourceDictionary>> GetEnumerator() => 
            ((IEnumerable<KeyValuePair<OSAppTheme, ResourceDictionary>>)_themedResources).GetEnumerator();
        public bool TryGetValue(OSAppTheme key, out ResourceDictionary value) => 
            ((IReadOnlyDictionary<OSAppTheme, ResourceDictionary>)_themedResources).TryGetValue(key, out value);
        IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)_themedResources).GetEnumerator();
        #endregion
    }
}
using AppThemes;
using Xamarin.Forms;

namespace App1
{
    public partial class App : Application
    {
        private AppThemeResources _appThemeResources;
        public App()
        {

            Device.SetFlags(new string[] { "AppTheme_Experimental" });

            InitializeComponent();

            MainPage = new MainPage();

            _appThemeResources = new AppThemeResources(this);

        }

        protected override void OnStart()
        {
        }

        protected override void OnSleep()
        {
        }

        protected override void OnResume()
        {
        }
    }
}



md5-51268c003d5b30b223766c5227285648



<?xml version="1.0" encoding="utf-8" ?>
<Application xmlns="http://xamarin.com/schemas/2014/forms"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             xmlns:d="http://xamarin.com/schemas/2014/forms/design"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
             xmlns:theme="clr-namespace:AppThemes"
             mc:Ignorable="d"
             x:Class="App1.App">
    <Application.Resources>
        <theme:AppThemeColor x:Key="PageBackgroundColor" Light="Blue" Dark="Red" Default="Green" />
        <theme:AppThemeColor x:Key="PrimaryTextColor" Light="Black" Dark="White" Default="Gray" />
    </Application.Resources>
</Application>

@StephaneDelcroix , I tried what you suggested, but get a runtime error:

System.InvalidOperationException: Cannot determine property to provide the value for.
  at Xamarin.Forms.Xaml.AppThemeBindingExtension.Xamarin.Forms.Xaml.IMarkupExtension<Xamarin.Forms.BindingBase>.ProvideValue

A search for this error only found the source code where this is thrown.

Here's the xaml I'm trying:

    <AppThemeBinding x:Key="ThemeColorPageBackground">
        <AppThemeBinding.Light>
            <Color>White</Color>
        </AppThemeBinding.Light>
        <AppThemeBinding.Dark>
            <Color>#222222</Color>
        </AppThemeBinding.Dark>
    </AppThemeBinding>

I'm also a bit concerned about this solution, even if it did work. My reasoning is that there is no control in this syntax that ensures the object for Light and Dark are the same. In theory you could make Light a Color and Dark a Thickness. Of course that doesn't make sense, so it would be nice if the syntax enforced that better, similar to the original syntax in this issue.

@CZEMacLeod original naming was wrong and caused unrealistic expectations and a wrong mental representation of what this is, and what this does.

Using a style is the correct way to handle this. We plan to bring that capability to our CSS support as well.

But there's definitely a wrong way of doing this. It is possible to define the AppThemeBinding as non-shared resource, and reuse it. If someone asks, I've never told you about it, and never wrote the following snippet of code:

<ContentPage.Resources>
  <AppThemeBinding x:Key="FancyColor" x:Shared="false">
    <AppThemeBinding.Light><Color>HotPink</Color></AppThemeBinding.Light>
    <AppThemeBinding.Light><Color>LimeGreen</Color></AppThemeBinding.Light>
  </AppThemeBinding>
</ContentPage.Resources>

<StackLayout>
  <Label BackgroundColor="{StaticResource FancyColor}"/>
  <Label BackgroundColor="{StaticResource FancyColor}"/>
</StackLayout>

This is supposed to work, but I haven't tested it (please report if it doesn't in a separate issue, and I'll fix it). Why isn't it recommended ? Because the mental model makes you think you're sharing a sort of magical color, while you're sharing a binding.

This doesn't works.
This error is triggered:
System.InvalidOperationException: Cannot determine property to provide the value for.

Was this page helpful?
0 / 5 - 0 ratings