Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  2. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

Suggestions for ReorderableList element in the UnityEditorInternal namespace

Discussion in 'Editor & General Support' started by Gru, Aug 13, 2017.

  1. Gru

    Gru

    Joined:
    Dec 23, 2012
    Posts:
    142
    I have recently started using ReorderableList element. I've had to implement some kind of 'list of lists' and would like to share my ideas about improvements. The API itself is mostly fine, but there are numerous bugs and inconsistencies. Here is my list:

    1. Use generics instead of type parameter. It is better to write

    Code (CSharp):
    1. var list = new ReorderableList(myList)
    than
    Code (CSharp):
    1. var list = new ReorderableList(myList, typeof(string))
    because the compiler can deduce the generic for the constructor defined as
    Code (CSharp):
    1. ReorderableList<T>(IList<T> list);
    There are 2 main benefits to this approach: 1) Less typing 2) resilience to changes. If the type of myList changes, the drawer breaks silently. Some other Unity APIs have also migrated to this standard.

    2. Use params object for construction instead of long constructors. This is a problem with lots of Unity Editor's APIs. There are so many overloads for constructors of elements that it makes the code unreadable. Even worse, if new parameter is desired it's either a breaking change or yet another overload. Instead, it is common to create 'parameter objects'. Instead of having:
    Code (CSharp):
    1. ReorderableList(IList elements, Type elementType, bool draggable, bool displayHeader, bool displayAddButton, bool displayRemoveButton)
    it is better to have
    Code (CSharp):
    1. struct ReorderableListParams {
    2.     public bool draggable;
    3.     public bool displayHeader;
    4.     public bool displayAddButton;
    5.     public bool displayRemoveButton;
    6. }
    7. ReorderableList(IList elements, Type elementType, ReorderableListParams settings) { ... }
    8.  
    9. // ...
    10. var settings = new ReorderableListParams() { displayRemoveButton = false}
    11. var rl= new ReorderableList(myIList, typeof(string), settings);
    12.  
    this allows us to change the params by a hypothetical SetParams method, eases constructors readability and is easy to extend with new parameters. Using a struct ensures these settings are copied by value. Flags are another option, but less convenient because of verbosity with many settings. I've seen the Defaults class is used, and it is similar to how this should work but isn't quite there.


    3. Allow changes of parameters object without recreating the instance. Maybe we want to stop drawing the header without re-creating the instance. This is similar to above, done by using SetParams method. It would be done like so:

    Code (CSharp):
    1. var settings = reorderableList.GetParams();
    2. settings.drawHeader = false;
    3. reorderableList.SetParams(settings);

    4. De-couple what is drawn from how it is drawn.
    This is a problem I've seen floating around with many alternatives to this element. If we specify the callbacks and the setup for the list, then we should keep one instance of that and call Draw(myIList). The ReorderableList should still know the type in order to be able to insert elements. This gets a bit complicated because the buttons are after the list elements and so the buttons must know which list they are applied to even in the cases of nested lists. One solution to this is to keep an internal data structure such a a stack which will hold the stack of instances we have began drawing. The type of the stack is known because of the generic constructor, as in point (1). When a specific list has began drawing, it is added to the stack and once it is completed drawing it is removed from the stack. (Obviously, The stack only holds the reference to the list instance, not list elements themselves). The buttons apply to stack.Peek() element. This way we can keep all the settings and callbacks inside one ReorderableList instance and reuse it do draw many ILists, useful in cases where we want dynamically added and removed ReorderableList's in custom editors.


    5. Don't use custom delegates. Instead, use Action<>, Func<> and Predicate<> from System namespace. All custom delegates do is obfuscate the code, in order to us the API we have to peek into these definitions and chase references. Instead, by using Action<> it is readable when Intellisense pops up. Custom delegates exist for compatibility reasons and are useful only if you need to have ref and out params on arguments.

    6. Remove it from Internal namespace. This denotes the thing may change, and thus may break our editors. It's been a couple of years now and the default list drawer just doesn't cut it, maybe it's time to fix this drawer and put it in the Editor namespace so we can use it without fear.

    7. Allow proper drawing when there is no header. The option allows only removing the text in the header, but not the header itself. These should be separate options. Setting the header height to 2 works for the most part, but it's "hacky".

    8. Display default inspector for element when no default drawing is supplied. Why can't we have this?

    9. Allow us to specify padding in settings. By default this element takes the whole width and no space on top. Working with BeginHorizontal and BeginVertical to fix this is not fun.

    10. Fix the [bug] when the list is displayed in BeginHorizontal. Right now, the list is displayed in 3 columns, which is wrong (header, body, buttons). When drawing the layouts, it should probably be internally wrapped in Begin / End Vertical to prevent this from happening.

    11. Fix [bug] with reordering. If you make a list of 3 elements and set their heights to 300, 200, 100 you will not be able to drag the top element below the bottom most element. Something with the logic of calculating the height is not working when the element is dragged. This is visible because the size of the blue outline changes.

    12. Give a setting to remove active state. Better yet, give a setting to prevent the "active but unfocused" state of the control. This way if there are lots of lists in the editor everything gets painted in grey and blue because the RL does not release the active element when focus is lost (instead it paints it in grey).

    13. Expose DoRemoveButton and DoAddButton. When we do override the onRemoveCallback, why would we ever want to not remove the element? Right now, we have to do the removal ourselves which is cumbersome. The list should know how to do the removal itself, and allow us to specify additional work. However, in order for the API to be consistent, we have to replace the callback, which is also a bad idea. The callbacks should be cumulative with +=. If we do want to replace the callback, then we may want to access the already provided functionality of DoRemoveButton.
     
    psydent and sand_lantern like this.