Bug [Regression] ICreateHorizontalToolbar and ICreateVerticalToolbar are internal

After updating from Unity 2021.2.17a to 2021.2.19a the compiler complaints about

ICreateHorizontalToolbar

and

ICreateVerticalToolbar

being internal methods. I don't think these are supposed to be internal? If we want to create other than Panel tools.

 

Gotcha. Thanks for the clarification!

 

Okay, after a couple of hours on this... I'm not sure how I feel about this change. Basically ToolbarOverlay extends the Overlay class and implements both ICreateHorizontalToolbar and ICreateVerticalToolbar.

The main issue is that adding elements to the overlay is done using a fixed array in the super constructor:

Code (CSharp):

1. [Overlay(typeof(SceneView), "Toolbar")]
2. public class Toolbar : ToolbarOverlay
3. {
4.     private Toolbar() : base(ARRAY_OF_TOOLBAR_ELEMENTS)
5.     { }
6. }

This makes it impossible to dynamically add new elements, because the base() constructor method is called before anything else.

My proposal, after digging into the ToolbarOverlay class, is asking if you could consider changing the following methods to "virtual":

Code (CSharp):

1. public virtual VisualElement CreateHorizontalToolbarContent() => this.CreateToolbarContent();
2.  
3. public virtual VisualElement CreateVerticalToolbarContent() => this.CreateToolbarContent();

Hope this makes sense! If you need some more concrete code to support the arguments I'm open to provide it!

Also, as a final note, reverting the attribute classes ICreateHorizontalToolbar and ICreateVerticalToolbar to public would also do the trick and would still allow users to use the ToolbarOverlay class as is.

 

Understood. I look forwad to see the design you settle on, but please, be mindful that calling the base constructor adds unnecessary constraints, making it impossible for developers to dynamically add elements to the toolbar.

In our case, for example, are adding a button onto the toolbar by gathering all classes that have a custom attribute [ToolbarButton]. But that's impossible with the current design, because no code can be called before the base(...) method is called.

 

After spending many hours and hours into this, I can't come up with a way to deal with this situation. I would like to propose one solution that would make this Overlays system as flexible as it was when the ICreateHorizontalToolbar and ICreateVerticalToolbar interfaces were available, without having to expose them.

That is, creating a couple new methods inside the ToolbarOverlay class, like this:

Code (CSharp):

1. protected void AddElement(string id)
2. {
3.     this.m_Toolbar.AddElement(id);
4. }
5.  
6. protected bool RemoveElement(string id)
7. {
8.     return this.m_Toolbar.RemoveElement(id);
9. }

By doing this, we (developers) can choose between adding a fixed array at the base constructor of the ToolbarOverlay implementation, or simply call the

AddElement(...)

from within the constructor, allowing us to dynamically choose which elements should be part of the overlay. Like this:

Code (CSharp):

1. [Overlay(typeof(SceneView), "My Toolbar")]
2. public class MyToolbar : ToolbarOverlay
3. {
4.     public MyToolbar()
5.     {
6.         string[] ids = this.DynamicallyGetToolbarButtonIds();
7.         foreach (string id in ids) this.AddElement(id);
8.     }
9.  
10.     private string[] DynamicallyGetToolbarButtonIds()
11.     {
12.         // ... returns a string[] of ids using reflection (for example)
13.     }
14. }

@gabrielw_unity if you could also take a look at this, you'd make my day (well, probably the year )

 

Oops! Sorry, my bad. Thanks though for relaying this to the developers!