Search Unity

  1. Are you interested in providing feedback directly to Unity teams? Sign up to become a member of Unity Pulse, our new product feedback and research community.
    Dismiss Notice

Bug [Regression] ICreateHorizontalToolbar and ICreateVerticalToolbar are internal

Discussion in '2021.2 Beta' started by Catsoft-Studios, Jun 7, 2021.

  1. Catsoft-Studios

    Catsoft-Studios

    Joined:
    Jan 15, 2011
    Posts:
    668
    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.
     
  2. kaarrrllll

    kaarrrllll

    Unity Technologies

    Joined:
    Aug 24, 2017
    Posts:
    463
    Ruchir and Catsoft-Studios like this.
  3. Catsoft-Studios

    Catsoft-Studios

    Joined:
    Jan 15, 2011
    Posts:
    668
    Gotcha. Thanks for the clarification!
     
  4. Catsoft-Studios

    Catsoft-Studios

    Joined:
    Jan 15, 2011
    Posts:
    668
    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.
     
    mariandev likes this.
  5. kaarrrllll

    kaarrrllll

    Unity Technologies

    Joined:
    Aug 24, 2017
    Posts:
    463
    With this release we opted to be conservative about the API exposed, as we want to provide a more feature-rich solution for toolbars. While we don't have the exact design agreed on, we know that the current `ICreateToolbar` interface and virtual override of content creation is not something that we want to keep long term, which is why it was removed from 2021.2.
     
  6. Catsoft-Studios

    Catsoft-Studios

    Joined:
    Jan 15, 2011
    Posts:
    668
    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.
     
    kaarrrllll likes this.
  7. Catsoft-Studios

    Catsoft-Studios

    Joined:
    Jan 15, 2011
    Posts:
    668
    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 :))
     
    PutridEx likes this.
  8. gabrielw_unity

    gabrielw_unity

    Unity Technologies

    Joined:
    Feb 19, 2018
    Posts:
    863
    Ha, thanks @Catsoft-Studios, unfortunately I'm just the UX artist, not enough dev knowledge to really help there ... but I'll note this to the devs! :)
     
    Catsoft-Studios and Ruchir like this.
  9. Catsoft-Studios

    Catsoft-Studios

    Joined:
    Jan 15, 2011
    Posts:
    668
    Oops! Sorry, my bad. Thanks though for relaying this to the developers!
     
unityunity