Open DanRStevens opened 4 years ago
One of the notable differences is support for sorting:
void sorted(bool isSorted) { mSorted = isSorted; }
bool sorted(bool) const { return mSorted; }
void sort() { if (mSorted) { std::sort(mItems.begin(), mItems.end()); } }
I found this implementation a little non-intuitive. Calling sort()
does nothing unless the sorted
property is first set to true. I was thinking the ListBox
should sort the contents whenever sort()
is called. There doesn't seem to be a clear reason to first require setting a sorted
property.
Additionally the sorted property may be misleading, in that it could be taken to mean the contents are currently sorted. However, having the sorted
property set doesn't mean the contents are actually sorted. The sort()
method may not have been called yet, or a new item may have been added since last calling sort()
.
To add to the sort
topic:
The ComboBox
explicitly sets the sorted
property to false
, but this is the default value, so does nothing.
The FileIo
control calls .sort()
manually, but does not set the sorted
property, which defaults to false
. Technically this means the .sort()
call from FileIo
does nothing. This is probably unintentional.
The use of ListBox
from FactoryReport
does not make any use of the sort()
or sorted
features. The default is to not sort.
In all cases, no sorting is done. This feature is effectively unused.
Additionally, the ListBox::removeItem
method calls sort()
after removing an item. If the list is already sorted before removal, then it will still be sorted after removal. There is no need to re-sort the list.
To add to the above:
The ListBox::addItem
method was calling sort()
when the sorted()
property was set, so there was some partial support for ensuring the ListBox
remained sorted once the sorted()
property was set. However, this was only true if an additional item was added, or if sort()
was manually called.
Additionally, it is less efficient to sort items as they are added one by one, than to sort them all at once. Sorting items one by one is going to have at least quadratic worst case complexity, and possibly worse. Actually, if the sort()
is done using quicksort, with worst case quadratic efficiency, and the items being added are already sorted so as to hit the worst case of quicksort, and this is done each time for n items, we may end up with cubic efficiency, which is quite lousy. By sorting all items at once, either before or after adding them, efficiency can be improved to quadratic overall worst case for a simple implementation, or O(n lg n) for the average case, which can actually be guaranteed with some search algorithms.
I'm kind of thinking sorting should be kept as an external concern. Maybe it would have a use for multicolumn sort support, if that was eventually added, though even that could be handled externally, and in advanced cases, would probably need to be handled externally anyway.
I would agree that it is unintuitive to call sort and not have the list box sort. Probably better if sort is either a property that when set sorting occurs automatically, or sorting only occurs when a sort function is called externally.
Also agree should be pared down to one ListBox class for now.
ListBoxBase was the intended version to keep over ListBox -- IO think I mentioned it on Discord a couple days ago. When I was building the list boxes used in the fullscreen UI's, I realized I was copy/pasting a lot of code so I started a base ListBox with the base functionality that could then be derived from. I called the new 'base' class ListBoxBase while I developed it. I don't remember why but ultimately I got distracted and never actually finished the process.
To comment on the sorting... I would generally agree? I don't remember why it ended up that way. Organic development often leads to weird code like this.
I figure my first task will be to resolve the minor differences between the list box types to make them as identical as possible. That way the real differences will stand out more. I already started with simple things like header includes and formatting.
I suspect most of the remaining differences could then be resolved by copy/pasting code for the feature differences.
There may be some differences though in terms of multi column support, and ownership of the list box items. I noticed one of the list boxes uses a std::vector
of list box items, while the other uses pointers to items. The rendering of items in derived classes seems to have some custom code, so that will need to be looked at.
One of the big differences between ListBox
and ListBoxBase
is in the treatment of ListBoxItem
. In ListBox
, there is a std::vector<ListBoxItem>
stored directly, and cleaned up automatically. In ListBoxBase
, it instead uses std::vector<ListBoxItem*>
, which requires manual cleanup of the ListBoxItem
pointers in the destructor. The reason for this is that derived classes may use derived ListBoxItem
types to store extra data, and those extra fields aren't known to the base class.
As an alternative, the ListBox
class could be made a template
, with the ListBoxItem
being a parameter (possibly defaulted). That way, the class can directly store a std::vector
of the derived type, removing the need to use pointers, and the need for manual cleanup in the destructor.
As part of this, we may need to make the ListBoxItem
class responsible for drawing it's own instances. That way it can draw any extra fields that it stores.
Drawing extra fields by the ListBoxItem
should work well when items have a natural size, such as fixed size icons. It might not work so well for multi-column text list boxes, where there is no natural column size, and the text length between rows differs. For that, we may also need some kind of header information that can be used to size each column, and can be passed to the rendering method of each item to allow for proper and consistent sizing. Currently though, we don't have this requirement, so we can perhaps put that consideration off to another day.
Was thinking back to this, and was considering if the data list could be extracted from the list box, and accessed through an interface. That could allow that core ListBox
class to not be a template
class, while still allowing for various data types, not known to ListBox
at compile time, and still allow items to be directly stored in a std::vector
without needing to use pointers.
The data source would need to expose how many items there are, how many pixels tall an entry would need to be, and code to draw the contents of an item slot. A reasonable data source simplification might be to assume that all items have the same pixel height. If a data source exposed that information through an interface, that derived instances of the interface could be used to implement the various list boxes. The ListBox
itself could be responsible for background color and borders. The data source would be responsible for drawing item text, and any additional info such as icons, or progress bars.
There are two independent classes:
ListBox
ListBoxBase
They are highly similar, though don't derive from each other. We should seek to eliminate one of them.
Currently
ListBox
is used by:ComboBox
FileIo
FactoryReport
Currently
ListBoxBase
is used as a base class by:FactoryListBox
ProductListBox
StructureListBox
It seems
ListBox
may be slightly more feature complete, so that may be the candidate class to keep. To that end, we should investigate usingListBox
as a base class instead ofListBoxBase
. It may be some features will need to be added before this can be done.