Hierarchy Icons added #26
Conversation
…rface.cs. Hierarchy status icons added (HierarchyWindowInterface.cs)
There was a problem hiding this comment.
This is cool, thank you so much for the PR! I just have a couple of code reviews and comments:
This should probably be an optional setting, because the hierarchy window is very sensitive to performance issues, and there are a lot of plugins that also add icons to the hierarchy, which could also cause visual problems. If you could add a setting to the settings tab to toggle this, that would be great!
Your PR has highlighted the fact that ProjectWindowInterface is acting both as a database and as a UI handler, which is not great. If you're looking to do more things, splitting the AssetPostProcessor data handling parts from the UI parts in that class would be awesome. This is not a requirement for this PR at all, it's just something I noticed needs doing :P
| { | ||
| static HierarchyWindowInterface() | ||
| { | ||
| EditorApplication.hierarchyWindowItemOnGUI += OnHierarchyItemTryToDrawStatusIcon; |
There was a problem hiding this comment.
This shouldn't be subscribed at InitializeOnLoad time, because there is no guarantee that this callback will happen after all of the git bits have been initialized by ApplicationManagerBase. You probably haven't noticed any issues because you already have git set up in your project, so in most cases this callback happens at a safe time, but at times when git needs to be set up or there's an update happening, this callback will get called before the setup is done, and the ProjectWindowInterface.GetStatusIconForAssetGUID(guid) call below will likely throw an exception.
To make sure there's no race conditions at startup, this should be done in a Initialize method like https://github.com/spoiledcat/git-for-unity/blob/main/src/com.spoiledcat.git.ui/Editor/UI/ProjectWindowInterface.cs#L39, and the call to it should be added in https://github.com/spoiledcat/git-for-unity/blob/main/src/com.spoiledcat.git.ui/Editor/ApplicationManager.cs#L36C35-L36C35
There was a problem hiding this comment.
You're right, your way is much better integrated with the tool ;) I have done as you asked, thanks!
|
|
||
| // place the icon to the right of the list: | ||
| Rect r = new Rect(selectionRect); | ||
| r.x = r.width + 10; |
There was a problem hiding this comment.
The icons seem to be aligned with an inverse indentation to the list item indentation, instead of being all aligned to the right of the window regardless of the indentation of the list entry. That looks odd? This probably needs to be r.x = r.xMax - 10, so it's aligned to the right no matter how much indentation the entry has.
There was a problem hiding this comment.
I've been using this tool for a while now in my own project and I kinda got used to that incidental indentation - it's easier to see which Hierarchy Icon belongs to which prefab object, when those Hierarchy Icons mimick the indentation of the prefab objects. But, I understand that it might not be for everyone, so I added an option to the SettingsView to turn on/off that indentation, I hope it is okay :)
|
@shana About refactoring |
|
I looked at You can tell it's a singleton because haha everything in there is static. 🫣 It's an AssetPostProcessor subclass, but we're not implementing any callbacks for that? That's probably a sign it doesn't need to be an AssetPostProcessor. The likely reason we're not hooking up any AssetPostProcessor events? That's because it only fires for files Unity cares about, not actually for all filesystem changes in the whole repository, and we have a native filesystemwatcher that watches and raises events for everything, so AssetPostProcessor is redundant. If we did want to hook up to asset post processing, I would do it in a handler class that just fires off "hey files might have changed" events, and that would be something that RepositoryManager would listen for, because RepositoryManager is responsible for listening to filesystem changes events and updating git data accordingly (but we already have all the filesystem data we need, so we don't need to do this) I haven't had time yet to look at your updated changes, but I'll get to it as soon as I can, hopefully this evening. Thank you again for taking the time to do this, I can't express enough how much I appreciate it! ❤️ |
|
@MateuszBudny This is now merged and included in the v1.0.27 release (available from https://registry.spoiledcat.com/ or from the |
@shana |
Description of the Change
Hierarchy Icons has been added! They show the same icons as in the Project View for prefabs.
I have refactored
ProjectWindowInteraceclass by extractingGetStatusIconForAssetGUID(guid)method from it, so I can get an icon for an asset GUID easily, without copying code.Alternate Designs
I was thinking about adding such icons next to a prefab name in prefab mode too, but that's maybe for the future, as it is easier now after my
ProjectWindowInteraceclass refactor.Benefits
It is much easier to see now, which prefabs are currently locked. It is not needed to look at Locks window or to look at Project View before modifying any prefab, so it is harder to make changes in locked files by mistake.
Possible Drawbacks
It is possible for the icons to be drawn on prefabs names. But you need to have a very long prefab's name or a very narrow Hierarchy window. For me, it is a small price to pay for less mistakes made by developers.
Applicable Issues
Before this change, checking if a prefab is locked was easy from scene/prefab view, but demanded additional action (finding the prefab in a Project View or opening a Locks window and comparing a list of locks with your prefab). Now, you see this info in Hierarchy window, which means you can see if the prefab is locked without additional actions, when you modify the prefab from Scene View.