Make VtkPlot more object-oriented

[psavery]

This will allow us to add more customization to the class without
having a static function with an exceedingly long list of arguments...

Signed-off-by: Patrick Avery psavery@buffalo.edu

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

[psavery]

@cryos @ghutchis
We talked about doing something similar to this at the UGM.

I tested this on PlotPdf and PlotXrd, and it seemed to work fine for both.

[psavery]

@cryos Can you take a look at this when you get a chance? I'll need this sometime soon for another PR...

[psavery]

Unfortunately, this isn't quite as stable as I wish that it was.

It crashes every once in a while after the previous plot is deleted. It appears that the crash is during event handling (a crash log is pasted below - this crash log is consistently produced).

So I will probably need to modify the way cleanup is performed. Maybe ensuring all signals/slots are disconnected. Maybe using deleteLater().

Thread 1 "avogadro2" received signal SIGSEGV, Segmentation fault.
0x00007ffff1691404 in QOpenGLContext::handle() const ()
   from /usr/lib/libQt5Gui.so.5
(gdb) where
#0  0x00007ffff1691404 in QOpenGLContext::handle() const ()
    at /usr/lib/libQt5Gui.so.5
#1  0x00007ffff7fb8889 in  ()
    at /usr/lib/qt/plugins/xcbglintegrations/libqxcb-glx-integration.so
#2  0x00007fffea34c637 in QXcbIntegration::createPlatformOpenGLContext(QOpenGLContext*) const () at /usr/lib/libQt5XcbQpa.so.5
#3  0x00007ffff1694f7e in QOpenGLContext::create() ()
    at /usr/lib/libQt5Gui.so.5
#4  0x00007ffff1696957 in  () at /usr/lib/libQt5Gui.so.5
#5  0x00007ffff1697b86 in QOpenGLWindow::resizeEvent(QResizeEvent*) ()
    at /usr/lib/libQt5Gui.so.5
#6  0x00007ffff1662805 in QWindow::event(QEvent*) () at /usr/lib/libQt5Gui.so.5
#7  0x00007ffff1688aa9 in QPaintDeviceWindow::event(QEvent*) ()
    at /usr/lib/libQt5Gui.so.5
#8  0x00007ffff747af53 in QVTKOpenGLWindow::event(QEvent*) (this=
    0x55555a112f80, e=0x7fffffffe350)
    at /home/patrick/src/openchemistry/thirdparty/VTK/GUISupport/Qt/QVTKOpenGLWindow.cxx:383
#9  0x00007ffff1c27e14 in QApplicationPrivate::notify_helper(QObject*, QEvent*)
    () at /usr/lib/libQt5Widgets.so.5
#10 0x00007ffff1c2f6e1 in QApplication::notify(QObject*, QEvent*) ()
    at /usr/lib/libQt5Widgets.so.5
#11 0x00007ffff125e129 in QCoreApplication::notifyInternal2(QObject*, QEvent*)
--Type <RET> for more, q to quit, c to continue without paging--
    () at /usr/lib/libQt5Core.so.5
#12 0x00007ffff16570ce in QGuiApplicationPrivate::processExposeEvent(QWindowSystemInterfacePrivate::ExposeEvent*) () at /usr/lib/libQt5Gui.so.5
#13 0x00007ffff1657d4e in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () at /usr/lib/libQt5Gui.so.5
#14 0x00007ffff1631a4c in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Gui.so.5
#15 0x00007fffea3db31d in  () at /usr/lib/libQt5XcbQpa.so.5
#16 0x00007ffff125cdbc in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#17 0x00007ffff12650b6 in QCoreApplication::exec() ()
    at /usr/lib/libQt5Core.so.5
#18 0x000055555556d5a6 in main(int, char**) (argc=1, argv=0x7fffffffe768)
    at /home/patrick/src/openchemistry/avogadroapp/avogadro/avogadro.cpp:193

[cryos]

I will try and take a look at this soon, any QObject should preferably use deleteLater to allow for all signals that might be connected to execute before the object goes away (rather than trying to disconnect things).

[psavery]

Unfortunately, adding something like this to my destructor doesn't seem to fix the issue:

m_widget->deleteLater();
m_widget.release();

Perhaps because the other parts like the render window, chart, and view get deleted...

[cryos]

In general the use of QScopedPointer and/or std::unique_ptr is going to cause issues with QObject derived classes. You need to remember that Qt uses a parent-child ownership model for QObjects. We should have a member pointer, initialize it to null, and instantiate in the show/display call (as you are). Ensure the correct ownership of the dialog is passed in so that the window shows up where it should, and let the parent take care of calling destruct. We can change the window flags, but ideally on subsequent calls the same object is reused, and you simply update the data, rather than calling delete, and making another new one.

When dealing with QObject-based classes, we should prefer the Qt-based pointer classes too. When dealing with VTK the VTK-based pointer classes, when dealing with STL/C++ objects certainly prefer the STL pointer classes.

[psavery]

Thanks for taking a look, Marcus!

Yes, it makes sense for me to do all of that. However, since a QWidget requires a QWidget as its parent, we should perhaps figure out which QWidget should be the parent for widgets in plugins.

I see in many plugins where we set the parent of a widget like so: qobject_cast<QWidget*>(parent()). Here, here, and here are some examples. However, this appears to be setting nullptr as the parent, because the parent of the plugins is not a QWidget (I tested it and found that qobject_cast<QWidget*>(parent()) gives nullptr in one of the plugins).

That means we have a lot of QWidgets in our plugins without a parent (although I think the QApplication does keep a record of them). Should we do something about this?

Also, I was considering to allow multiple VTK plots of the same kind to exist simultaneously (in case a user wants to compare, for instance, different settings for the same kind of plot). But for now, we can just allow one plot and re-use it when another plot command is called.

[psavery]

@cryos
My latest commit fixes the crashing issue I mentioned earlier because the old plot objects are no longer deleted, but re-used. I think this is a better model overall. It seems to work fine.

I have not set a Qt parent for them yet (and removed the smart pointer) because I haven't been able to figure out a suitable parent for them to have, as they can only have a widget as their parent.

In addition, the Vtk plot still stays open after the main Avogadro window is closed. This is probably partially caused by #389.

[ghutchis]

Can we bump this to check the AppVeyor build? I'd like to get a few of these merged...