Skip to content

Commit 4bd741a

Browse files
keveleighkeveleigh
committed
Ensure invalid types are handled throughout the flow
1 parent 1d8d42e commit 4bd741a

4 files changed

Lines changed: 108 additions & 35 deletions

File tree

‎org.mixedrealitytoolkit.theming/Editor/BaseThemeBinderDrawer.cs‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,10 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten
3434

3535
string[] names = null;
3636
SerializedProperty themeDefinitionItemName = null;
37+
bool hasDataSource = themeDataSourceProperty != null && themeDataSourceProperty.objectReferenceValue != null;
3738

3839
// Only pay the cost of parsing available items when the property is actively expanded
39-
if (property.isExpanded && property.managedReferenceValue != null && themeDataSourceProperty != null && themeDataSourceProperty.objectReferenceValue != null)
40+
if (property.isExpanded && property.managedReferenceValue != null && hasDataSource)
4041
{
4142
themeDefinitionItemName = property.FindPropertyRelative(ThemeDefinitionItemNameField);
4243

@@ -51,18 +52,16 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten
5152

5253
// Draw the foldout and all child properties within the allocated rect.
5354
Rect propertyRect = position;
54-
if (names != null)
55+
if (hasDataSource)
5556
{
5657
// Reserve the last line for the Bound Theme Item popup when expanded.
5758
propertyRect.height -= property.isExpanded ? EditorGUIUtility.singleLineHeight + EditorGUIUtility.standardVerticalSpacing : 0;
5859
}
5960
EditorGUI.PropertyField(propertyRect, property, label, true);
6061

6162
// Draw the Bound Theme Item popup using the Rect API, within the allocated position.
62-
if (names != null && property.isExpanded)
63+
if (hasDataSource && property.isExpanded)
6364
{
64-
int selected = System.Array.IndexOf(names, themeDefinitionItemName.stringValue);
65-
6665
// Child fields are indented 15px from position.x. Unity's label/control
6766
// split is at position.x + labelWidth, so the label width is (labelWidth - 15f)
6867
// and the control starts at that same absolute split point.
@@ -73,13 +72,25 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten
7372
Rect labelRect = new Rect(position.x + indentWidth, rowY, EditorGUIUtility.labelWidth - indentWidth, EditorGUIUtility.singleLineHeight);
7473
Rect controlRect = new Rect(splitX, rowY, rightEdge - splitX, EditorGUIUtility.singleLineHeight);
7574

76-
using (var check = new EditorGUI.ChangeCheckScope())
75+
if (names != null)
76+
{
77+
int selected = System.Array.IndexOf(names, themeDefinitionItemName.stringValue);
78+
using (var check = new EditorGUI.ChangeCheckScope())
79+
{
80+
EditorGUI.LabelField(labelRect, "Bound Theme Item");
81+
selected = EditorGUI.Popup(controlRect, selected, names);
82+
if (check.changed)
83+
{
84+
themeDefinitionItemName.stringValue = names[selected];
85+
}
86+
}
87+
}
88+
else
7789
{
78-
EditorGUI.LabelField(labelRect, "Bound Theme Item");
79-
selected = EditorGUI.Popup(controlRect, selected, names);
80-
if (check.changed)
90+
using (new EditorGUI.DisabledScope(true))
8191
{
82-
themeDefinitionItemName.stringValue = names[selected];
92+
EditorGUI.LabelField(labelRect, "Bound Theme Item");
93+
EditorGUI.Popup(controlRect, 0, new string[] { "(No matching items)" });
8394
}
8495
}
8596
}

‎org.mixedrealitytoolkit.theming/Editor/ThemeDefinitionEditor.cs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// Copyright (c) Mixed Reality Toolkit Contributors
2+
// Licensed under the BSD 3-Clause
3+
4+
using UnityEditor;
5+
6+
namespace MixedReality.Toolkit.Theming.Editor
7+
{
8+
[CustomEditor(typeof(ThemeDefinition))]
9+
public class ThemeDefinitionEditor : UnityEditor.Editor
10+
{
11+
public override void OnInspectorGUI()
12+
{
13+
serializedObject.Update();
14+
15+
DrawDefaultInspector();
16+
17+
ThemeDefinition def = target as ThemeDefinition;
18+
if (def != null && def.ThemeDefinitionItems != null)
19+
{
20+
foreach (var item in def.ThemeDefinitionItems)
21+
{
22+
string displayLabel = string.IsNullOrWhiteSpace(item.Name) ? "(null)" : item.Name;
23+
System.Type type = item.DataType?.Type;
24+
25+
if (type == null)
26+
{
27+
EditorGUILayout.HelpBox($"Item '{displayLabel}' has no DataType selected, or the type could not be resolved.", MessageType.Warning);
28+
}
29+
else if (type.IsAbstract || type.IsInterface || type.IsGenericTypeDefinition)
30+
{
31+
EditorGUILayout.HelpBox($"Item '{displayLabel}' uses an invalid DataType ({type.Name}). It must be a concrete, non-generic class to be instantiated.", MessageType.Warning);
32+
}
33+
}
34+
}
35+
36+
serializedObject.ApplyModifiedProperties();
37+
}
38+
}
39+
}

‎org.mixedrealitytoolkit.theming/Editor/ThemeDefinitionEditor.cs.meta‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎org.mixedrealitytoolkit.theming/Editor/ThemeEditor.cs‎

Lines changed: 46 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ public class ThemeEditor : UnityEditor.Editor
1616

1717
private static bool itemsFoldout = false;
1818

19+
private static readonly System.Collections.Generic.HashSet<Type> failedTypes = new System.Collections.Generic.HashSet<Type>();
20+
1921
private string nameField;
2022
private string dataField;
2123
private string valueField;
@@ -57,18 +59,40 @@ public override void OnInspectorGUI()
5759
SerializedProperty dataProp = themeItemProp.FindPropertyRelative(dataField);
5860
SerializedProperty valueProp = dataProp?.FindPropertyRelative(valueField);
5961

62+
string themeItemName = themeItemProp.FindPropertyRelative(nameField).stringValue;
63+
string displayLabel = string.IsNullOrWhiteSpace(themeItemName) ? "(null)" : themeItemName;
64+
6065
if (valueProp != null)
6166
{
6267
// Draw just the Value field, labelled with the item name,
6368
// skipping the intermediate "Data" foldout entirely.
64-
string themeItemName = themeItemProp.FindPropertyRelative(nameField).stringValue;
65-
EditorGUILayout.PropertyField(valueProp, new GUIContent(themeItemName), true);
69+
EditorGUILayout.PropertyField(valueProp, new GUIContent(displayLabel), true);
6670
}
6771
else
6872
{
69-
// Fallback for any item whose Data doesn't follow the
70-
// BaseThemeItemData<T> shape (e.g. a null reference).
71-
EditorGUILayout.PropertyField(themeItemProp, true);
73+
if (dataProp?.managedReferenceValue == null)
74+
{
75+
var definitionItem = themeDefinition.ThemeDefinitionItems[i];
76+
if (definitionItem.DataType?.Type == null)
77+
{
78+
EditorGUILayout.HelpBox($"'{displayLabel}' has no valid DataType selected in the ThemeDefinition.", MessageType.Warning);
79+
}
80+
else
81+
{
82+
EditorGUILayout.HelpBox($"Failed to initialize data for '{displayLabel}'. Ensure its DataType ({definitionItem.DataType.Type.Name}) is a concrete class with a default constructor.", MessageType.Warning);
83+
}
84+
85+
using (new EditorGUI.DisabledScope(true))
86+
{
87+
EditorGUILayout.LabelField(displayLabel, "null");
88+
}
89+
}
90+
else
91+
{
92+
// Fallback for any item whose Data doesn't follow the
93+
// BaseThemeItemData<T> shape.
94+
EditorGUILayout.PropertyField(dataProp, new GUIContent(displayLabel), true);
95+
}
7296
}
7397
}
7498
}
@@ -109,34 +133,31 @@ private void ReconcileThemeItems(ThemeDefinition themeDefinition, SerializedProp
109133
{
110134
// Move the found item up to position i, preserving its saved values.
111135
themeItemsProp.MoveArrayElement(existingIndex, i);
136+
themeItem = themeItemsProp.GetArrayElementAtIndex(i);
112137
}
113138
else
114139
{
115140
// No existing item found — insert a fresh one with default values.
116-
117141
themeItemsProp.InsertArrayElementAtIndex(i);
118142
themeItem = themeItemsProp.GetArrayElementAtIndex(i);
143+
themeItem.managedReferenceValue = new Theme.ThemeItem(themeDefinitionItemName, null);
144+
}
145+
}
119146

120-
Type dataType = definitionItem.DataType?.Type;
121-
object dataInstance = null;
147+
// Unified instantiation and auto-healing for missing or mismatched data types
148+
Type expectedType = definitionItem.DataType?.Type;
149+
SerializedProperty dataProp = themeItem.FindPropertyRelative(dataField);
150+
Type actualType = dataProp?.managedReferenceValue?.GetType();
122151

123-
if (dataType != null)
124-
{
125-
try
126-
{
127-
dataInstance = Activator.CreateInstance(dataType);
128-
}
129-
catch (Exception e)
130-
{
131-
Debug.LogWarning($"Failed to instantiate data for ThemeItem '{themeDefinitionItemName}' (Type: {dataType.Name}). Falling back to null. Exception: {e.Message}");
132-
}
133-
}
134-
else
135-
{
136-
Debug.LogWarning($"Could not resolve DataType for ThemeItem '{themeDefinitionItemName}'. Falling back to null.");
137-
}
138-
139-
themeItem.managedReferenceValue = new Theme.ThemeItem(themeDefinitionItemName, dataInstance);
152+
if (expectedType != null && expectedType != actualType && !failedTypes.Contains(expectedType))
153+
{
154+
try
155+
{
156+
dataProp.managedReferenceValue = Activator.CreateInstance(expectedType);
157+
}
158+
catch
159+
{
160+
failedTypes.Add(expectedType);
140161
}
141162
}
142163
}

0 commit comments

Comments
 (0)