floating point exception for TabWidget

[svenevs]

I was creating a tab widget and wasn't being very careful, it looked something like this

mTabWidget = new TabWidget(mControlsWindow, "Controls");
auto *layer = mTabWidget->createTab("First");
// ... some fancy layout stuff that rely on wrappers ...
//                         vvvvvvvvvv
auto *wrapper = new Widget(mTabWidget);// !!!!!
//                         ^^^^^^^^^^
wrapper->setLayout(new GroupLayout());

The wrapper needed to have layer not mTabWidget as the parent, but the side effect is that when the tab widget performs its layout a floating point exception occurs.

Can I just add docs to the tab widget saying "never add a child directly to the tab widget, always create a new tab and add children to that tab"? I don't really feel like trying to find / catch / prevent this is worth it...

Related: would it be worth saying in the docs explicitly that a floating point exception will occur if you do this? It might help future users who accidentally do this understand what is wrong, but it may be overkill

[Thorius]

I remember thinking about this usage problem when I was making the tab widget but I never got to doing anything about it. Digging through my old notes and reacquainting myself with the code, here is what I thought could be alternatives to only documenting it:

1. Document it as above but also overload the addChild method to throw a meaningful exception (TabWidgetAddExceptions or something along that line) when invoked. This is practically the document part but with a meaningful exception.
2. Overload the addChild method to add the child widget as a tab with a default name (like "Tab N", where N is just the tab number/index) - this is probably not the most expected behaviour but it is easily spotted
3. Overload the addChild method but simply forward all widgets to the first tab - not optimal as there could be no tabs and it kind of hides the problem and the misuse.

I personally prefer them in the order given above but some mix of the three might actually be the best.

[svenevs]

Hey @Thorius thanks for the TabWidget! I think docs + (1) are reasonable, the VScrollPanel does something similar:

https://github.com/wjakob/nanogui/blob/926b66fad70d29a718324a3317bbca01b80cd540/src/vscrollpanel.cpp#L28-L29

Doing any kind of default action would be very confusing, and might go unnoticed. My understanding is there should be exactly two children for a TabWidget:

https://github.com/wjakob/nanogui/blob/926b66fad70d29a718324a3317bbca01b80cd540/src/tabwidget.cpp#L27

So if mChildren is not exactly {mHeader, mContent} issue an exception in performLayout?

That might negatively affect subclasses, I was thinking of calling say protected virtual void validateChildren() in performLayout. It's default would be something like

if (mChildren.size() > 2)
    throw std::runtime_error("TabWidget: do not add children directly to the TabWidget, create tabs and add children to those");

// Make sure mHeader and mContent are both present in mChildren
bool header_found = false;
bool content_found = false;
for (auto *child : mChildren) {
    if (child == mHeader) header_found;
    else if (child == mContent) content_found;
    else
        throw std::runtime_error("TabWidget: do not modify the list of children for a TabWidget.  The expected children are exactly two: `mHeader` and `mContent`.");
}
if (!header_found)
    throw std::runtime_error("TabWidget: do not modify the list of children for a TabWidget. `mHeader` was not present in `mChildren`!");
if (!content_found)
    throw std::runtime_error("TabWidget: do not modify the list of children for a TabWidget.`mContent` was not present in `mChildren`!");

That may be a little excessive, but the idea would be that if somebody needs to subclass TabWidget, then they would want to override validateChildren(). This enables them to still call TabWidget::performLayout(ctx).

I can write it up if it makes sense, but it feels kind of clunky to me.

[Thorius]

Well, this seems reasonable to me. It is somewhat long but it is quite straightforward. So, definitely write it up if you can. The only sort-of problem is that we catch this misuse during performLayout and not during addChildren (which is ass soon as we can catch it). Maybe just have validateChildren() called then? But then again, the VScrollPanel class does it at that time so it could be consistent with that. (but it might make sense to update that if it would make for better usage).

I will look into it today/tomorrow to see if I can think up of anything better and I will write back here.

[svenevs]

Hmm. So you can actually modify the children list directly (though I think it's safe to say this is frowned upon). @wjakob is VScrollPanel checking that mChildren.size() == 1 in performLayout to "guarantee" that the check actually takes place (e.g., rather than overriding Widget::addChild methods)?

[Thorius]

@svenevs , modifying the list of children is definitely not a good idea (honestly, I think that it might be better to eventually make mChildren private in Widget and only access then trough some protected observer but that is a topic for another time).

What I was referring to, is something along these lines.

void TabWidget::addChild(Widget* child) override {
    // Only the header and content should ever get added. They should also be added before invoking addChild.
    if (child != mHeader && child != mContent) {
        throw std::runtime_error("TabWidget: do not add children directly to the TabWidget, create tabs and add children to those");
    }
    Widget::addChild(child);
}

Honestly, this seems quite reasonable, but I am not quite sure if this will work because of the circular way children are added to the child vector. Also is it safe to throw an exception. Will the memory pointed to by child be freed if it is not added to the child vector? We might need to invoke Widget::addChild before throwing.

If there is no other fundamental reason to making this, I feel it is a bit cleaner than throwing in performLayout, as we catch the error earlier.

[svenevs]

Yes, I think that is much cleaner. The Widget::addChild(int, Widget *) is the only one that should be overridden (it's the only virtual one, and is the real implementation). There's an "easier" way to bypass the circle problem, which is to construct them as orphans (parent is nullptr). Let's continue the discussion on #306 and change things there if it's not quite what you imagined.

Thanks for weighing in on how to fix this!