Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Make `SharedDictionaryManager` work inside XAML Designer #950
Conversation
Design time uses `file` URIs that don't work with custom `ResourceDictionary` implementations. They need to be transformed to a `pack` URI so that the assembly that implements the custom `ResourceDictionary` (in this case `SharedDictionaryManager`) can be found.
Any XAML that uses a custom class needs be loaded from a `pack` URI so the assembly for the custom class can be found. This makes it convert all `file` URIs, not just ones containing `SharedDictionary.xaml`.
Theses don't work when running inside the designer. This class could do with tests.
Move all caching and loading responsibilities into SharedDictionaryManagerBase. Move theme support into ThemeDictionaryManager. Always add ResourceDictionary to MergedDictionaries (not just when there is a cache hit). Allow `Source` to be set multiple times to swap out ResourceDictionary (used when changing theme).
Clean up unit tests and make sure they all pass with NCrunch.
| @@ -544,6 +436,18 @@ Global | |||
| {50E277B8-8580-487A-8F8E-5C3B9FBF0F77}.XamlDesign|Any CPU.Build.0 = Release|Any CPU | |||
| {50E277B8-8580-487A-8F8E-5C3B9FBF0F77}.XamlDesign|x86.ActiveCfg = Release|Any CPU | |||
| {50E277B8-8580-487A-8F8E-5C3B9FBF0F77}.XamlDesign|x86.Build.0 = Release|Any CPU | |||
| {9453C37F-44CF-463D-B563-5C3110CEF5E7}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | |||
shana
Apr 6, 2017
Collaborator
This project isn't actually in the solution, is there something missing?
This project isn't actually in the solution, is there something missing?
|
|
||
| public virtual new Uri Source | ||
| { | ||
| get { return source; } |
shana
Apr 6, 2017
Collaborator
NullGuard will throw a NRE if the getter is called when source hasn't been set yet. Mark this with [return: AllowNull] to allow a null return, or make sure source is initialized to something to avoid the NRE.
(technically it won't be called before the setter, but the XamlDesigner might decide to do something wonky, and throwing here will cause it to go boom, so probably better safe than sorry)
NullGuard will throw a NRE if the getter is called when source hasn't been set yet. Mark this with [return: AllowNull] to allow a null return, or make sure source is initialized to something to avoid the NRE.
(technically it won't be called before the setter, but the XamlDesigner might decide to do something wonky, and throwing here will cause it to go boom, so probably better safe than sorry)
| public class SharedDictionaryManagerBase : ResourceDictionary | ||
| { | ||
| static IDictionary<Uri, ResourceDictionary> sharedDictionaries; | ||
| static IList<IDisposable> disposables; |
shana
Apr 6, 2017
Collaborator
You can use a CompositeDisposable type here, it's designed to handle disposables in an optimal way (you can just call Dispose() on it and it will handle disposing everything it holds.
You can use a CompositeDisposable type here, it's designed to handle disposables in an optimal way (you can just call Dispose() on it and it will handle disposing everything it holds.
shana
Apr 6, 2017
Collaborator
Nevermind, CompositeDisposable is an rx thing and not available here
Nevermind, CompositeDisposable is an rx thing and not available here
jcansdale
Apr 6, 2017
Author
Collaborator
As discussed, this is a Rx class, so will avoid taking this dependency. 😄
As discussed, this is a Rx class, so will avoid taking this dependency.
| static IDictionary<Uri, ResourceDictionary> GetAppDomainSharedDictionaries() | ||
| { | ||
| var name = typeof(SharedDictionaryManagerBase).FullName + ".SharedDictionaries"; | ||
| var sharedDictionaries = GetAppDomainData<Dictionary<Uri, ResourceDictionary>>(name); |
shana
Apr 6, 2017
Collaborator
The sharedDictionaries and the disposables list are already static across the domain, why save them in the appdomain?
The sharedDictionaries and the disposables list are already static across the domain, why save them in the appdomain?
jcansdale
Apr 6, 2017
Author
Collaborator
The designer does some shadow assembly tricks, so the same assembly can end up being multiple times. I'm doing this to prevent stale caches from being left behind.
The designer does some shadow assembly tricks, so the same assembly can end up being multiple times. I'm doing this to prevent stale caches from being left behind.
| } | ||
|
|
||
| public void Dispose() | ||
| { |
shana
Apr 6, 2017
•
Collaborator
The pattern for Dispose should be:
bool disposed;
override void Dispose(bool disposing)
{
if (disposed) return;
base.Dispose(disposing);
disposed = true;
if (disposing)
{
// dispose managed resources here
}
}
// in base class
public void Dispose()
{
Dispose(true);
// GC.SuppressFinalize(this); // if there's a finalizer in this class or any subclasses
}
The pattern for Dispose should be:
bool disposed;
override void Dispose(bool disposing)
{
if (disposed) return;
base.Dispose(disposing);
disposed = true;
if (disposing)
{
// dispose managed resources here
}
}
// in base class
public void Dispose()
{
Dispose(true);
// GC.SuppressFinalize(this); // if there's a finalizer in this class or any subclasses
}
jcansdale
Apr 6, 2017
Author
Collaborator
I tried to sidestep this by marking the class as sealed. Maybe this is a bit antisocial, but it wasn't designed with extenders in mind. It kept CodeAnalysis from complaining anyway. 😉
I tried to sidestep this by marking the class as sealed. Maybe this is a bit antisocial, but it wasn't designed with extenders in mind. It kept CodeAnalysis from complaining anyway.
|
|
||
| namespace GitHub.Helpers | ||
| { | ||
| public class SharedDictionaryManagerBase : ResourceDictionary |
shana
Apr 6, 2017
Collaborator
Given that this class needs to manage disposable resources, it should be IDisposable (if you have a CompositeDisposable instead of a List<IDisposable>, the code analysis will let you know this 😉
Given that this class needs to manage disposable resources, it should be IDisposable (if you have a CompositeDisposable instead of a List<IDisposable>, the code analysis will let you know this
|
Looking good! Just have a few comments around disposing resources, a possible nullref and a possible missing project |
| return sharedDictionaries; | ||
| } | ||
|
|
||
| static IList<IDisposable> GetAppDomainDisposables() |
shana
Apr 6, 2017
Collaborator
Feels like an odd name for a method that disposes of a cached list before returning it empty. Perhaps InitializeAppDomainDisposables() would be more descriptive of what this is doing (and when it should be called)?
Feels like an odd name for a method that disposes of a cached list before returning it empty. Perhaps InitializeAppDomainDisposables() would be more descriptive of what this is doing (and when it should be called)?
| return new Uri($"pack://application:,,,/{assemblyName};component/{path}"); | ||
| } | ||
|
|
||
| static IDictionary<Uri, ResourceDictionary> GetAppDomainSharedDictionaries() |
shana
Apr 6, 2017
Collaborator
InitializeAppDomainSharedDictionaries or something similar is probably a better name for this method?
InitializeAppDomainSharedDictionaries or something similar is probably a better name for this method?
Add a local test SharedDictionary.xaml.
Everywhere now uses `GitHub.UI.ThemeDictionaryManager`. SharedDictionaryManager can be used as base for added functionality (e.g. ThemeDictionaryManager).
|
@shana I think it's time to stick of fork in this one. I've incorporated your suggestions, including lifecycle management using a single disposable class. Test coverage also also much better now. Finally, I've done the grand consolidation of the 3 |
This is same as #945 except a PR to master.