New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResourceDictionary fixes #536

Merged
merged 2 commits into from Dec 6, 2016

Conversation

Projects
None yet
5 participants
@StephaneDelcroix
Member

StephaneDelcroix commented Nov 16, 2016

Description of Change

Fix the ResourceDictionary interface. Some calls were using the RD values, some others were merging with the merged instance.
Now, all public API only poke the current RD.

StaticResource and implicit styles use an internal API.

This replaces #317.

Bugs Fixed

API Changes

Behavioral Changes

minor

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@StephaneDelcroix StephaneDelcroix changed the title from Rd fixes to ResourceDictionary fixes Nov 16, 2016

@StephaneDelcroix StephaneDelcroix merged commit ff1bf0b into master Dec 6, 2016

1 of 6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Snapshot dependency …
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Snapshot dependency failed to start: ... Windows Debug
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Snapshot depende…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Snapshot dependen…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Snapshot dependen…
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3619, ignored: 10
Details

@StephaneDelcroix StephaneDelcroix deleted the RD-fixes branch Dec 6, 2016

@mikegottlieb

This comment has been minimized.

Show comment
Hide comment
@mikegottlieb

mikegottlieb Apr 7, 2017

I just updated to Xamarin 2.3.4. I had been accessing resources merged into my Application ResourceDictionary programmatically and that is now broken. I'm pretty sure this change is at fault.

Is there a new way to access the merged in elements of a ResourceDictionary?

mikegottlieb commented Apr 7, 2017

I just updated to Xamarin 2.3.4. I had been accessing resources merged into my Application ResourceDictionary programmatically and that is now broken. I'm pretty sure this change is at fault.

Is there a new way to access the merged in elements of a ResourceDictionary?

@ahmedalejo

This comment has been minimized.

Show comment
Hide comment
@ahmedalejo

ahmedalejo Apr 8, 2017

Hi @StephaneDelcroix , this "fix" is actually a bug or a huge breaking change. Because when i merge two things i expect them to act (logically) as one.
That is, merging a RD1 with another should make the other to logically contain all the keys from merged RD2, giving RD1´s key-values precedence.

Scenario: Retrieving values from merged ResourceDictionary

Given i have a ResourceDictionary with the following contents:

key value
PrimaryColour blue
SecondaryColour green
AccentColour red

When i merge it with a ResourceDictionary with following contents:

key value
SecondaryColour Red
AccentColour yellow
TapColour black

Then requesting for the following keys should result as follows:

key value
PrimaryColour blue
SecondaryColour green
AccentColour red
TapColour black

And the Keys count on the ResourceDictionary should be 4

So if understand your positioning, you´d like to only allow the StaticResourceMarkupException to have access to the merged Key value?

ahmedalejo commented Apr 8, 2017

Hi @StephaneDelcroix , this "fix" is actually a bug or a huge breaking change. Because when i merge two things i expect them to act (logically) as one.
That is, merging a RD1 with another should make the other to logically contain all the keys from merged RD2, giving RD1´s key-values precedence.

Scenario: Retrieving values from merged ResourceDictionary

Given i have a ResourceDictionary with the following contents:

key value
PrimaryColour blue
SecondaryColour green
AccentColour red

When i merge it with a ResourceDictionary with following contents:

key value
SecondaryColour Red
AccentColour yellow
TapColour black

Then requesting for the following keys should result as follows:

key value
PrimaryColour blue
SecondaryColour green
AccentColour red
TapColour black

And the Keys count on the ResourceDictionary should be 4

So if understand your positioning, you´d like to only allow the StaticResourceMarkupException to have access to the merged Key value?

_mergedWith = value;
if (_mergedWith == null)
return;
_mergedInstance = _mergedWith.GetTypeInfo().BaseType.GetTypeInfo().DeclaredMethods.First(mi => mi.Name == "GetInstance").Invoke(null, new object[] {_mergedWith}) as ResourceDictionary;
_mergedInstance = s_instances.GetValue(_mergedWith,(key) => (ResourceDictionary)Activator.CreateInstance(key));

This comment has been minimized.

@ahmedalejo

ahmedalejo Apr 8, 2017

couldn´t _mergedInstance be lazily initialized?

_mergedInstance  = new Lazy<ResourceDictionary>(
    () => s_instances.GetValue(_mergedWith, key => (ResourceDictionary)Activator.CreateInstance(key));
@ahmedalejo

ahmedalejo Apr 8, 2017

couldn´t _mergedInstance be lazily initialized?

_mergedInstance  = new Lazy<ResourceDictionary>(
    () => s_instances.GetValue(_mergedWith, key => (ResourceDictionary)Activator.CreateInstance(key));

@samhouts samhouts added this to the 2.3.4 milestone Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment