Fix dangling child widgets from Screen's focus/drag

[wardw]

This is a further step along from #341 which itself addresses #332 and the other variants discussed in that PR.

The essential change from #341 is to let a Widget be responsible for it's own removal from Screen, as a structured part of its destruction. Reasoning follows typical RAII semantics:

Stepping back a bit, it strikes me that root of the issue is that Screen is holding a reference to Widgets for drag/focus, but isn't taking part in their reference counting. I assume this is because Screen really only wants a "weak" reference, but in so far as it's still a reference (and in the absence of implementing alternatives) I suspect it's still probably a good idea that these references are accounted for somewhere..

So - this change is to let a Widget itself own this responsibility and make calling Screen::disposeWidget() part of a widget's destruction. To destroy a widget is to "dispose" of it - the two are semantically linked and so this enforces the point structurally. It enforces the responsibility of a widget to remove itself from the screen.

-- There is one problem with it as it stands - a Widget is a Screen, so when the Screen is destroyed it will throw. I've added a temporary workaround to this PR which wraps the call to Widget::screen() in a try/catch block: I will leave it up to you whether you might consider Widget::screen() returning nullptr to signal 'no screen' and make a Widget having no screen a valid part of it's semantics. I've left this PR in draft state - happy to add further if you can't foresee any other problems with this approach.

On the plus side, it does mean we could dispense with the additional Widget::removeChildHelper() code and we are back to the single API Widget::removeChild() which thanks to @chpatton013 now covers all cases in a consistent way.

Let me know what you think. There's a test example in bugfix_test (just a variant of example1).

[svenevs]

Hi @wardw,

Thanks for re-surfacing this issue, it is a good one to fix.

To destroy a widget is to "dispose" of it - the two are semantically linked and so this enforces the point structurally. It enforces the responsibility of a widget to remove itself from the screen.

I like the spirit of this, but I'm not entirely sure it is solving the whole problem. I'm reacquainting myself with this right now / am going to take a deeper look at #341 again. I have to do some testing, I think we want this responsibility as part of removeChild and not the destructor. I will test later, but I think it's supposed to be ok to transfer a widget to another parent / different screen without deleting it.

I've proposed a possible alternative to the try - catch problem, I don't think that the code duplicated there is very problematic ... it's duplicated a few times across the framework anyway. There's also a problem with Window::dispose causing segfault on this branch. If you are able to fix it great! But given the above, don't spend a lot of time on it because I'm not sure if moving everything to the destructors is the right choice.

This issue is kind of cyclic / confusing though. I could be wrong / this might be the easiest approach.

[svenevs]

Please un-do changes to this file. If we want it documented, it must be done in include/nanogui/screen.h and then run make mkdoc (that will regenerate all python docs). Personally I think it's OK to leave it undocumented for now at least ;)

So to clarify: all you should need to do is run make mkdoc or ninja mkdoc, it's a special target added at the bottom of CMakeLists.txt. If you need help with that let me know :)

[svenevs]

I think doing this kind of exception stuff in the destructor may not be a good decision. I think there are two good options:

1. Make screen() able to return nullptr rather than throwing an std::runtime_error. That's kind of a significant change though.

2. Less pervasive,just do it manually -- the Widget destructor is a special case, exceptions + destructors is dangerous territory. I think we could just do this:

   Widget::~Widget() {
       /* Similar to Widget::screen(), but does not throw:
        * 1. Destruction of orphan widgets should not throw.
        * 2. Special case: every Screen is a Widget.
        */
       Screen *screen = dynamic_cast<Screen *>(this);
       if (screen == nullptr) { // only dispose widgets, screen cannot dispose itself
           Widget *widget = this;
           while (true) {
               if (!widget) break;// 1: orphan widgets (no parent screen)
               screen = dynamic_cast<Screen *>(widget);
               if (screen) break;
               widget = widget->parent();
           }
           if (screen)
               screen->disposeWidget(this);
       }
   
       for (auto child : mChildren) {
           if (child)
               child->decRef();
       }
   }

Basically, in the destructor we need to be extra careful. "Orphan" widgets (no screen in parent chain) are not exactly supported, but not explicitly forbidden. No orphan ever gets drawn until it has a screen in the parent chain, so all drawing code assumes that a screen() can be found. We just want to make sure that we don't crash on what you mentioned (when it's a Screen being destructed), but also in the rare (but possibly abusively used) orphan widget case.

[svenevs]

This does not work. Run Example 1 and use the Info / Warn / Ask dialog. That's the "real" test case for Window::dispose (gets called by the message diaolgs in src/messagedialog.cpp).

[wardw]

Yes on reflection I think making it part of removeChild is better. You make a good point that should it be possible to move a widget between screens then yes it's true the semantics are separate: add/remove is different from create/destroy. For me I think that nails it. It's also less impactful in terms of the scope of the change.

Sorry for the goose chase! So yes - perhaps better to continue on from #341.

[wardw]

Nice library, by the way 👍

[svenevs]

should it be possible to move a widget between screens

For clarity, this was inaccurate (the same Widget instance should not be drawn on two separate screens). But it is possible to remove it from one screen and add it to another without deleting it :)

Nice library, by the way +1

Agreed, I am very grateful for NanoGUI (I am not the author) xD