-
Notifications
You must be signed in to change notification settings - Fork 81
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
make it able to use external UI dlls without referencing UnityWeld #35
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks like good code and a useful addition, thanks! There are a couple of minor things I think could be tweaked but the main blocker is that not ready to drop .NET 3.5 support just yet. See issue #36 for discussion about the .NET 3.5 issue.
/// <summary> | ||
/// Helper class for setting up the factory for use in the editor. | ||
/// </summary> | ||
public static class TypeResolver | ||
{ | ||
private static readonly IList<Type> BindingAttributeTypes = new List<Type>(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this and ViewModelProviders both have a size of 2 but only one element? Since most users aren't going to register a custom binding attribute I'd rather just have this default to one, and afaik adding an element to a .NET list is very cheap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thought process is to avoid additional allocations after start. And since in most cases we need list with size of 2 (max, default and custom) I've decided to use this number as initial capacity.
But I missed the fact that it happens only during initialization phase.
However I wouldn't use default capacity, it will be 3 useless refs in memory during whole app life time, unacceptable! :)
What do you think?
/// This will allow to mark bindable properties in external dlls without referencing UnityWeld. | ||
/// </summary> | ||
/// <typeparam name="T"></typeparam> | ||
public static void RegisterCustomBindingAttribute<T>() where T : Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we want RegisterCustomBindingAttribute
and RegisterCustomViewModelProvider
to be used externally, it would make more sense to either move this class out of the UnityWeld.Binding.Internal
namespace or move those functions to a separate class outside that namespace. It occurs to me now that everything inside the UnityWeld.Binding.Internal
namespace should probably also use the internal
access modifier instead of public
but that's not in the scope of this PR so don't worry about changing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Was confused by public access of this class.
But I'm not totally sure what the best way to do this...
In my opinion this is strongly relate to IoC principle, so I think I'll made class like "TypeRegistry" inside IoC namespace. And TypeResolver will use public properties of this class.
pseudo:
class TypeRegistry
BindingAttributeTypes { get; }
ViewModelProviders { get; }
RegisterCustomBindingAttribute()
RegisterCustomViewModelProvider()
Do you agree?
throw new ViewModelNotFoundException( | ||
$"Tried to get view model {viewModelName} but it could not be found on " + | ||
$"object {gameObject.name}. Check that a ViewModelBinding for that view model exists further up in " + | ||
"the scene hierarchy. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to change from string.Format
to string interpolation since it will break the code in older versions of Unity, although I guess if this PR still requires .NET 4.x support then I think all versions of Unity that support that should work with this as well. In general I'd prefer not to change stuff like this unless necessary though.
@@ -25,7 +24,7 @@ public virtual void Init() | |||
{ | |||
_isInitCalled = true; | |||
|
|||
if (!gameObject.activeInHierarchy) | |||
if(!gameObject.activeInHierarchy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please revert all the formatting changes in this and other files? I know it's a bit of a nitpick but there are a few things in your PR that break from the convention that's used throughout the rest of the codebase:
- There should always be a space before the opening
(
after anif
,while
,using
,do
, orfor
statement, e.g.if (SomeExpression())
- When breaking a long statement onto multiple lines, only indent one level - don't indent to make the code line up with something on the previous line. When breaking a function call onto multiple lines, the closing bracket should always be on the same indent level as the opening bracket, and if there are multiple arguments each argument should be on a separate line:
// Do this:
var monoBehaviourViewModel = cache.Components
.FirstOrDefault(component =>
component.GetType().ToString() == viewModelName
);
var providedViewModel = cache.Components
.Select(component => component.GetViewModelData())
.Where(component => component != null)
.FirstOrDefault(
// etc...
UnityWeld/Binding/BindingHelper.cs
Outdated
} | ||
} | ||
|
||
private static readonly Dictionary<Type, Queue<IList>> InternalCache = new Dictionary<Type, Queue<IList>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cache system necessary for the original purpose of making Unity Weld more decoupled from UI code? It looks like a separate feature to me but could maybe still be a useful addition. Could you explain a bit more about how it works?
…I logic without referencing UnityWeld. Benefits: - no tight coupling with unityweld - external dlls keep its abstraction levels and can be used as you want (i.e. in console application) This is very good practice and should be done in every single plugin. For example Zenject has it by default. Done: * registration of custom Binding attribute (default one is preregistered) * registration of custom ViewModelProvider (default one is preregistered)
…bleList to avoid tight coupling with UnityWeld. Discussion: Real-Serious-Games#36 Done: - Remove support of .net3.5 (it's deprecated in Unity since 2018 version) - Remove custom ObservableList, INotifyCollectionChanged, NotifyCollectionChangedEventArgs - Replace refs to ObservableList with ObservableCollection
now you can easily register adapter you want from anywhere no redundant instances of adapters
fix adapter editor
fix CollectionBinding (template reusing)
…ta files for existing projects. Replace in all *.prefab (MAKE BACKUP!) From m_Script: {fileID: -1670825535, guid: fe8162be1e46353499de126efe438c29, type: 3} To m_Script: {fileID: 11500000, guid: 43352902310e02e4895b9d70a80c740e, type: 3} From m_Script: {fileID: -1360956885, guid: fe8162be1e46353499de126efe438c29, type: 3} To m_Script: {fileID: 11500000, guid: ed406354527c60646af11d782e6dd994, type: 3} From m_Script: {fileID: 1223786303, guid: fe8162be1e46353499de126efe438c29, type: 3} To m_Script: {fileID: 11500000, guid: 88c48d5377676054584c97f362f86ddc, type: 3} From m_Script: {fileID: -456091253, guid: fe8162be1e46353499de126efe438c29, type: 3} To m_Script: {fileID: 11500000, guid: 4bf93dac7f39c7f42a98844ee9f5566c, type: 3} From m_Script: {fileID: 526415227, guid: fe8162be1e46353499de126efe438c29, type: 3} To m_Script: {fileID: 11500000, guid: c165104c7335da14ead0c4ebff8e0d79, type: 3} From m_Script: {fileID: -1957390036, guid: fe8162be1e46353499de126efe438c29, type: 3} To m_Script: {fileID: 11500000, guid: f6644ac576482b74684d320d204e5c67, type: 3}
Main goal: make it able to use external dlls which contains abstract UI logic without referencing UnityWeld.
Benefits:
This is very good practice and should be done in every such plugin. For example Zenject has it by default.
Done:
/How to use (just an example)/
Create Unity script with the following (You don't need to create anything if you use default binding attribute and view model provider)