[LV2] Qt5 plugin UIs and libsuil

[David Robillard]

On Mon, 2017-03-06 at 18:20 +0100, Stefan Westerfeld wrote:
>    Hi!
> 
> On Thu, Mar 02, 2017 at 08:06:03PM +0100, David Robillard wrote:
> > On Mon, 2017-02-27 at 13:55 +0100, Stefan Westerfeld wrote:
> > [...]
> > > Ok, so you merged the falkTX Qt5 embedding implementation (not
> > > mine).
> > > Now that
> > > it is in upstream libsuil I rebuilt Ardour5 with that suil. For
> > > synthv1
> > > everything indeed works as expected. For the SpectMorph UI, it
> > > somewhat works.
> > 
> > No, I merged neither.
> > 
> > > However there is the annoying problem that if you open the
> > > SpectMorph
> > > UI in
> > > Arodur, resize it to a larger window size, close it and reopen
> > > it,
> > > the
> > > SpectMorph UI widget will not cover the entire UI space Ardour
> > > provides, but a
> > > smaller subarea.
> > 
> > [...]
> > > I also believe that falkTK himself said that his approach would
> > > be
> > > somewhat
> > > of a hack. It for instance uses raw Xlib functions like
> > > XMoveWindow.
> > > My
> > > implementation on the other hand tells the Qt5 toolkit to do the
> > > embedding
> > > basically like this:
> > > 
> > >   wrap->qembed->windowHandle()->setParent (QWindow::fromWinId
> > > (gtk_socket_get_id (s)));
> > > 
> > > which is why I believe it handles all cases properly, because Qt5
> > > is
> > > doing
> > > the actual embedding, whereas the falkTX implementation is broken
> > > at
> > > least
> > > in one case (triggered by SpectMorph).
> > 
> > They both crash 99% of the time in jalv.gtk, the closest thing to a
> > reference LV2 host there is at the moment, so frankly they're all
> > garbage anyway as far as I can tell, and would need to be disabled
> > by
> > default in the next release.
> 
> Ok, this is of course an issue that needs to be adressed. I've tried
> to come up with a solution here:
> 
> https://github.com/swesterfeld/suil/commit/a5fb6c228739a200fe8c2b0ddc
> 0a518fa8397921
> 
> Unfortunately, when putting multiple toolkits in the same process
> under
> linux, I believe things will probably never be elegant. So this is
> an attempt to solve the problem pragmatically: I've introduced a new
> function suil_init(). On a system that has X11, it will call
> XInitThreads();

Yeah, the general idea of doing this via anything other than the native
window type was probably a mistake, if a pragmatic one.

> I used a module to do that (so -lX11 is not a suil dependancy), but
> it is a
> matter of taste whether to add X11 to the suil depedencies on
> platforms
> that use X11.
> 
> Since XInitThreads() needs to be called before any other Xlib
> function,
> this is now also true for suil_init(). In any case if you add
> suil_init()
> to jalv, Qt5 stuff no longer crashes at initialization. It crashes at
> exit
> though, not sure if this is related or unrelated.

Awesome.  I came to roughly the same solution (mentally), and think
libsuil itself indeed needs to have no platform specific direct
dependencies, but the problem with such a thing is that it could end up
bringing in whole toolkits into memory that are never actually used (if
some Gtk or Qt function needed to be called here, or whatever).  I
suppose there's no real way around that though.  The basic idea seems
unavoidable.

> If you think the basic idea is sound, a few more things can be done
> to
> make it a bit more elegant, like
> 
>  - avoiding code duplication for module loading

Sure.

>  - enforcing that suil_init was called before any other suil function
>    can be used

This would break backwards compatibility, so at most a warning is
appropriate.  The troublesome qt5 module itself could hard fail, but
other suil things shouldn't.

> Also when it comes to the suil_init() API itself, we could
> 
>  - pass the host toolkit as argument suil_init (SUIL_QT5)
>  - pass argc/argv
> 
> and other stuff. However, I tried to intentionally keep the code as
> small
> as possible to fix the problem we have now, without overdesigning.

Yes, the main wart in suil that shold also be addressed by an init is
QApplication (and probably similar things to do Mac and Windows more
correctly).  argc and argv should be taken, and maybe some extensible
thing so an application can pass the proper QApplication or whatever
equivalent to use if it has one.

> > > So after having tested both, I strongly recommend merging my
> > > code.
> > 
> > I strongly recommend all you Qt people agree on a solution that
> > works
> > in whatever cases you care about, submit a patch for it, and I will
> > happily merge that.  I'm not oscillating between patches in some
> > he-
> > said she-said game.
> 
> Ok, I've also put my suggestion on github:
> 
> https://github.com/swesterfeld/suil/commit/ea400f7e70cd6ca81a03a91937
> 1f46779b737f71
> 
> to make it a bit easier to test. Please, especially falkTX, could
> you test this? Would that work for everyone?

Awesome, thanks.  I'll take a closer look when I get home from work
today.

-- 
dr