6713 Replace the secondary source type with more granular types

[b-kamphorst]

Pull Request Check List

Resolves: https://github.com/python-poetry/poetry/issues/6713

• [x] Added tests for changed code.
• [x] Updated documentation for changed code.

[ghost]

Hi @tjbarrott. We have added the "s/needs-repro" label to this issue, which indicates that we require steps and sample code to reproduce the issue before we can take further action. Please try to create a minimal sample project/solution or code samples which reproduce the issue, ideally as a GitHub repo that we can clone. See more details about creating repros here: https://github.com/dotnet/maui/blob/main/.github/repro.md

This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

[tjbarrott]

https://github.com/tjbarrott/MergedDictionariesTest

I put the code in MainPage.xaml.cs

That repo uses .net 6. I also tried it in .net 7 with the same result.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[hartez]

This is happening because ResourceDictionary's ContainsKey method only checks the inner dictionary, and ignores the merged dictionaries: https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/ResourceDictionary.cs#L179

The method needs to be updated, and the unit tests need to be updated to test for this.

[hartez]

Did you find any workaround? Yes. run through the keys in foreach loop and see if the key equals the string

You could also use if(dictionary.TryGetValue(key, out _))

[tjbarrott]

Did you find any workaround? Yes. run through the keys in foreach loop and see if the key equals the string

You could also use if(dictionary.TryGetValue(key, out _))

Unfortunately that did not work 100% of the time. Maybe that is another bug. It did return true when the key was in the set, but I had a situation where it returned true when the key was not in the set.

[hartez]

You could also use if(dictionary.TryGetValue(key, out _))

Unfortunately that did not work 100% of the time. Maybe that is another bug. It did return true when the key was in the set, but I had a situation where it returned true when the key was not in the set.

When that happens, what's the value in the out parameter?

[tjbarrott]

I figured out, it's not a bug. I get the value that is associated with the key out. The dictionary I am getting it from does not have the key, but it references the dictionary that does in ResourceDictionary.MergedDictionaries.

[Cybrosys]

I just encountered this yesterday when writing some code to be able to change the colors based on user choice. My initial native approach was to do: Resources["Primary"] = <color>

... which worked great as to just assign a color to a page's Resources dictionary which was completely empty. It obviously did not result in what I was expecting 🤣

Long story short. Various methods and attempts led me to find TryGetValue and the list of merged resource dictionaries in App.Current.Resources.

What I quickly found out was that regardless of which instance I was working with, the method ContainsKey always returned false. Inspecting the Keys property I could clearly see that the value I was looking for was there, so I merely replaced all my ContainsKey calls with Keys.Contains.

I ended up with this extension method named "TryUpdateValue" following the "TryGetValue" namimg; it's far from perfect but is enough to achieve what I was after:

public static class ResourceDictionaryExtensions
{
    public static bool TryUpdateValue(this ResourceDictionary dictionary, string name, object value)
    {
        if (dictionary is null)
            return false;

        if (dictionary.Keys.Contains(name))
        {
            dictionary[name] = value;
            return true;
        }

        foreach (var mergedDictionary in dictionary.MergedDictionaries)
        {
            if (mergedDictionary.Keys.Contains(name))
            {
                mergedDictionary[name] = value;
                return true;
            }
        }

        return false;
    }
}

The ResourceDictionary needs some TLC with the hopes that various ways of accessing and manipulating data is consistent or offers ways for the caller to specify the expected behavior.

For example, I would expect this [string index] to behave in the same was as TryGetValue and my own TryUpdateValue with the exception that it returns true or false instead of throwing an exception.

Looking at the implementation of this [string index] it should have returned the same value as TryGetValue, it however does not because it is using ContainsKey (perhaps it should call TryGetValue instead of having its own logic for recursively go over all the dictionaries): https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/ResourceDictionary.cs#L186

The setter implementation does not update the value in the dictionary where it exists, it assigns it to the _innerDictionary. There is nothing wrong with the setter doing that, as long as the getter matches that behavior. Consistency I think is something to strive for here. If there are methods and/or properties which act on more than the inner and merged dictionary, then it should be clear they do so, or take inputs to indicate the desired behavior.

[hartez]

Unfortunately, it turns out that the unexpected behavior of ResourceDictionary is required for Hot Reload to work. So we won't be modifying it, but we will update the documentation for ContainsKey to make it clear that it only looks at the immediate dictionary and ignores any merged dictionaries.

[Ghostbird]

Unfortunately, it turns out that the unexpected behavior of ResourceDictionary is required for Hot Reload to work. So we won't be modifying it, but we will update the documentation for ContainsKey to make it clear that it only looks at the immediate dictionary and ignores any merged dictionaries.

To me having the code break conventional interface contracts and common sense, because a specific tool's functionality is dependent on it, is extremely weird. Shouldn't the hot reload functionality of .NET CLI Hot Reload be fixed to not rely on this wrong interface behaviour? If it were up to me, I'd talk to those Hot Reload devs and ask them to fix it. IF it turns out that the issue is that they indeed rely on interface-breaking behaviour, AND they refuse to fix it, I'd let it break, and let them deal with the consequences.

[Ghostbird]

@Cybrosys How does that code work for you? If I try that, the dictionary now contains two identical keys, with different values:

> x
{Microsoft.Maui.Controls.ResourceDictionary}
Count: 27
IsReadOnly: false
Keys: Count = 27
[0]: "Primary"
[1]: "Primary"
[2]: "Secondary"
[3]: "White"
…
> x.Keys.First()
"Primary"
> x.Keys.Skip(1).First()
"Primary"
> x.Keys.Skip(1).First() == x.Keys.First()
true

@hartez This is a direct violation of the .NET IDictionary interface contract that ResourceDictionary claims to implement. If behaviour of implementations completely ignores the contracts it is built upon, what use is it to a developer? It would quite literally be better if it didn't exist. It would be less work to write this yourself, than to have to figure out what causes these obscure bugs, then write it yourself.

EDIT: Ugh, never mind. This probably ties back in with the ContainsKey issue. I bet that the underlying this[string index] implementation uses ContainsKey and since ContainsKey's result lies about the dictionary content, it thinks that it needs to add a new entry. And I bet that that underlying Add() implementation also relies on ContainsKey, so that one will not throw either.

Can we please not ignore this issue. An IDictionary without a valid ContainsKey function is clearly not a dictionary at all.

[Ghostbird]

@Cybrosys I guess I did something wrong when first trying your extension method. It's working now.

[Cybrosys]

@Cybrosys I guess I did something wrong when first trying your extension method. It's working now.

It was a quickly cobbled together method that achieved what I was trying to do when testing something. I wouldn't be surprised if there are some quirks or odd behaviors with it due to how ResourceDictionary behaves.

[jfversluis]

Looking at the discussion and linked, closed PR, I think this is a variation of: https://github.com/dotnet/maui/pull/11214#issuecomment-1498922649

As such, closing this for now.