Only show components in skin editor which are usable on the current screen #17279
Conversation
|
I'm just going to review this wholesale with the third dependency not here since that one is a bit circular... |
| fill.Add(new ToolboxComponentButton(instance, target) | ||
| { | ||
| RequestPlacement = t => RequestPlacement?.Invoke(t) | ||
| }); |
There was a problem hiding this comment.
So if I understand this correctly, this add is what actually holds the invariant that only usable components are shown on their corresponding screens, and it works because the synchronous add causes a synchronous load which fails after dependencies are not found in the borrowing container, and therefore the element is finally not added.
I guess it works but I feel some reservations towards that - it feels a bit too magic if I'm honest and needs to be properly xmldoc'd / signposted. Like right now SongProgress are visible when skin editing song select because they have nullable dependencies. The try-catch is also doing a lot of magic - any old other failure that isn't a DI failure will also make the component disappear. I'd be more fine with catching a DI-specific exception and logging any other unexpected one.
Dunno, probably going to wait for @smoogipoo's thoughts on this.
There was a problem hiding this comment.
Maybe it's okay for now? Definitely needs documentation though.
There was a problem hiding this comment.
The try-catch is also doing a lot of magic - any old other failure that isn't a DI failure will also make the component disappear. I'd be more fine with catching a DI-specific exception and logging any other unexpected one.
I also agree with this bit for better safety, DependencyNotRegisteredException should be enough to indicate a DI failure.
All other similar UI components have required dependencies, so this is mainly to bring things in line with expectations. I am using this fact in the skin editor to only show components which can be used in the current editor context (by `try-catch`ing their `Activator.CreateInstance`).
|
I've adjusted the exception handling and added some comments. Let me know if that's enough. |
|
ef29de9 didn't cache enough gameplay clocks to fix all test failures so I cached more. Seems okay otherwise. |
EditorSidebar#17277 (conflict avoidance)PerformancePointsCounterDI resolved components to be required #17276 (to keep the counter placeable)