XSizeHints fixes

[x42]

This fixes the following regression introduced in db07a21d484

• The window may not have been realized when wrapper_wrap is called. In this case XSizeHints are not set.
• Changes to XSizeHints were never queried, and the original min-size was enforced even when the window called ui:resize()

Previously this worked, because XSizeHints were unconditionally queried in every call to forward_size_request.

[drobilla]

Merged as 4912f67 and fbf5c73, thanks.

[brummer10]

To bad. What you call "remove cruft", removes indeed the possibility to reduce the size below the initial size which was my main intention when I introduced this patch. Setting the min_size in a xwindow smaller then the initial size becomes useless this way. Please revert that.

[x42]

suil_x11_on_size_request() handles min_size, just fine, doesn't it? One can still reduce the size of a plugin window below the default size to the minimum.

Also why do you think this should be set when the window is re/mapped?

[brummer10]

The user cant resize the window smaller then the initial size. It's because gtk2 set the size request, there is no min/max size, you cant get below the size request given. True, the plugin could call size request and that will work. When the window is mapped we could set the size request to the min size given by a plug, without resize the window to that size. This way the user could reduce the size as well.

[brummer10]

@x42 please mention one plug which could be resized below initial size to min size by the user, after your patch have been applied.

[drobilla]

@brummer10 Please mention at least one concrete plugin, and host, and window manager scenario where you specifically expect this behavior.

I read the original PR yesterday and couldn't find anything like this either. My testing did, however, confirm that it blatantly broke a bunch of things, and @x42's changes fixed them. It was clearly a mistake to merge the original code.

Please revert that.

Reality check: the original changes caused serious regressions. There is zero chance I'll merge any further size tinkering on blind faith again (especially when it seems weird, which the dropped hunks certainly do).

If you want something changed, make a PR, and please test it thoroughly this time, including at least 2 independent plugins and at least 2 independent hosts, and document what they are so that it can be verified.

[brummer10]

@drobilla This works with all GxPlugins as with all new guitarix pugins, in Ardour5/6 and jalv. from the original pull request:

Thus allow init a Plugin UI with a default size and shrink it afterwards to the min size.

Would you point me to what regressions you talk about? I may understand that it could be useful to check for changes in XSizeHints, but why do you need to remove the call to the map_event? That is exactly what allow to present a plugin window with a default size and give the user the possibility to shrink it to the min size.

[drobilla]

I used x42 Tuna and LSP parametric EQs in both Jalv and Ardour. @x42 has links to various videos but I lost my logs.

Forcefully setting size request properties to a stored minimum on map does not seem like how Gtk sizing is supposed to work to me, and intuitively it's not clear why this should just "allow" shrinking it to the minimum size. I'm no expert on the Gtk size model but it seems like a kludge to me. There are callbacks specifically about size things...

[brummer10]

mm, I use x42 Tuna and LSP plugs as well in Ardour and jalv, but I didn't notice a regression.

It allow shrinking because the size-request property of the gtk window otherwise is set to the default-size. That mean, a user cant shrink below that size. That property's 'must' be changed after the window is mapped. When you set it before, it will open with the min size. Setting property's on "map-event" isn't in any way kludge.

[drobilla]

http://robin.linuxaudio.org/tmp/resize-suil-0.10.6-git.mp4 https://user-images.githubusercontent.com/35730375/78544235-82538200-7802-11ea-9a4f-c21002725841.gif

[brummer10]

Okay, I'm given up. Both plugs didn't handle resize anyway, and LSP plugins still have issues (parts of the UI still flicker) after this change. But surly they do that all right! However, I wouldn't bother you any more with this.

[drobilla]

Yeah, LSP seems to have a bunch of resize bugs. I was initially confused and thought suil was resizing them to minimum, but they do that to themselves, and seem to explicitly resize in all sorts of weird situations.

I am working on straightforward Pugl example plugins that should make it easier to test this stuff without a bunch of layers of various toolkits in the way. At a high level I of course agree that default/min/max sizes should work correctly, it's just way too hard to work on this way and I get the feeling that we're all working around each others' bugs, which can't end well.

[brummer10]

@drobilla
I've nothing against @x42 Ensure that XSizeHints are set and updated This patch is nice, clean, and a step to the better. What I'm talking about is the @x42 Remove cruft patch. This one have nothing to to with the mentioned issues. It looks like a "I want to do a clean up, and didn't bother anyway who depend on this shit". Both mentioned plugs behave exactly same with or without this patch. I've just tested it with the given examples. Just my plugs, couldn't be resized below the initial size when "the cruft" is removed.

[x42]

I'm sorry to have worded the commit message but I was/am under the impression that this is not required. Furthermore, using map-event to set sizes is really bad practice.

A plugin can set XHints the min/max sizes and size-increments, then resize itself initially using XResizeWindow() when the window is created or realized (usually in puglCreate). The window has the correct initial size, while gtk's size request returns the min size so a user can shrink it.

However before 4912f67839b, this didn't work in cases that I have tried. Due to the async nature of X, the window itself is not usually registered with the X server when libsuil wrapper_wrap is called. This caused the constraints to be ignored (which lead to the issue that tuna.lv2 had).

[x42]

Just my plugs, couldn't be resized below the initial size when "the cruft" is removed.

OK. I was not aware of this. It was not my intention to break guitarix resizing. I really thought this is a NO-OP, just cruft removal.

Does guitarix somehow special case g_object properties? What is the relationship between g_object_set(), and the explicit size-request constraint?

A property change ends up doing the same as gtk_widget_set_size_request. Under the hood gtk_widget_set_usize_internal() is called. That in turn calls g_object_notify to emit a signal "width-request" and "height-request", which calls the size-request callback.

However the size-request that libsuil implements ignores any of the g_object properties, so you might just as well not set them. Perhaps calling queue_resize() when the window is mapped is sufficient? I though that's implicit.

[brummer10]

First, it didn't set a size, it set a property, and, that is not bad praxis, it's usual. Then, a call to gtk_widget_set_usize_internal() would be ignored, when the new size-request is smaller then the current widget size. The logic in the callback ensure that that is the case. So, it couldn't do any harm to any plug, regardless if they use resizing or not.

Perhaps calling queue_resize() call when mapped is sufficient? I though that's implicit.

No, is it not, this shouldn't do anything else then set a property to the value given by the plugin, if the plugin gives this hint. No resizing or whatever.

However the size-request that libsuil implements ignores any of the g_object properties, so you might just as well not set them.

If that would be the case, the cruft wouldn't work at all.

[brummer10]

Does guitarix somehow special case g_object properties? What is the relationship between g_object_set(), and the explicit size-request constraint?

No, there is nothing like this in guitarix LV2 plugs, no (g_stuff) or GTK at all any-more. It's the (suil) host window which use the property's!!

A plugin can set XHints the min/max sizes and size-increments, then resize itself initially using XResizeWindow() when the window is created or realized (usually in puglCreate). The window has the correct initial size, while gtk's size request returns the min size so a user can shrink it.

That is, what I call a dirty workaround, and lead to much trouble with QT based hosts! We've already min/base/max size, we just need to use it and be done. And to do it proper, we need to set the gtk-window size-request to the min-size of the X11 window, if the X11 window comes with that hint. This way a plugin work well with QT/GTK/GTKMM based hosts. That is, by the way, a point which is already fixed in GTK3, and properly never will be fixed in GTK2.

[drobilla]

I understand that having your code removed is frustrating, but the original commit was clearly broken and not really tested, so I am rightly skeptical of the whole thing and erred on the side of caution. I was about to revert the entire thing, actually, but @x42 fixed it first.

Sorry, but since I obviously can't trust the claims that things work, I'll need to see it for myself, and I didn't get that far yesterday. Master not being broken takes priority.

[brummer10]

@drobilla I give a shit on my code been implemented or not. I didn't give a shit on removing features to no avail. You could have send me a message as well, that there are issues with this commit, I wasn't aware of it. However, . . .

OK. I was not aware of this. It was not my intention to break guitarix resizing. I really thought this is a NO-OP, just cruft removal.

Thanks for confirming what I think.

[x42]

On 4/11/20 3:14 PM, Hermann wrote:

A plugin can set XHints the min/max sizes and size-increments, then resize itself initially using XResizeWindow() when the window is created or realized (usually in puglCreate).

That is, what I call a dirty workaround

How so? This is how the X11 API works, isn't it? One sets size-constraints and an initial window size. It is not a workaround.

[brummer10]

I'm tiered by your comments. It seems we are not able to talk the same language. Just select what you want and drive a discussion in that direction, while ignore the relevant parts, isn't satisfactory for the conversation.

This is how the X11 API works, isn't it? One sets size-constraints and an initial window size. It is not a workaround.

Nope. Try it with the Qt layer, for example. When I open my own window to draw to, that work, but, relay on a window given by a "u-known source", that could be a rabbit hole.

[brummer10]

@x42 Have you ever open your tuna in Qt based host??? That's what I call dirty work around.

[drobilla]

The initial window size is passed in XCreateWindow(), it shouldn't be necessary to resize (though it probably makes no difference since this is all before the initial map anyway). The hints are actually set after, because you need a Window to do it.

i.e. in a pure X11 context, it looks like this:

1. w = XCreateWindow(...., x, y, width, height, ...)
2. XSetNormalHints(d, w, { .min_width = ... })

[brummer10]

Exact. Hence, when you would ensure a plugin window open with a size bigger then the min-size, you've to set up a call to LV2UI_Resize to let the host window know about your size. This is regardless of any base-size setting in the XWindowHints. Unfortunately after this call, the min-size of the host side gtk widget is the size you've call resize for. Resetting this to the min-size given in the XSizeHints is the approve of the call in map-callback of the gtk-host-window. This way, a plugin, behave the same on qt/GTK/GTK3 based host's. Otherwise, it "may" work like @x42 prefer, in, "put in the name of the host you've the control above how a plugin window is been created", but, at least, it will still be a dirty work around, and didn't work on Qt based hosts.

[brummer10]

I understand that having your code removed is frustrating, but the original commit was clearly broken and not really tested, so I am rightly skeptical of the whole thing and erred on the side of caution. I was about to revert the entire thing, actually, but @x42 fixed it first.

@drobilla Now, both plugs you mention, been clearly broken on Qt based host's, regardless of how we handle this stuff in GTK2. But you, believe, that it is the implementation I introduced, which broke this plugs. You may understand that I'm not willing to follow your logic. I, would say, this both plugins been broken and do the resize handling wrong. Repair the plugins is far more straight forward then arguing here about what is "dirty" or not. A plugin have to behave on all host's same, revert that rule will lead to far more trouble then one or two broken plugs. forgive me please, usually I would leave this discussion far before this point, but the comments I receive, really drive me to the nuts.

@x42 Please, do a step back and consider a free open community, leave ardour for a second out of mind, and review what you do.

bye

[x42]

On 4/11/20 5:34 PM, Hermann wrote:

Please, do a step back and consider a free open community, leave ardour for a second out of mind, and review what you do.

What has any of this to do with Ardour? or QT for that matter?

This pull request proposed two changes to fix the following

• asynchronous X calls (window may not be realized when suil queried XSizeHints
• remove ostensibly dead code, which according to the X11 API should not be required in the first place.

[x42]

@brummer10: A productive way forward would be to open a pull request to reverse the 2nd change I've made, or perhaps even improve upon it.

Accusing me of being partial is not helping with the technical issues at hand.

[brummer10]

@x42 How often I need to reaped the Question? Have you ever tried your tuna plugin on a Qt based Host? Your plug is simply broken by design. And now, you force the suil implementation to support your design. That's what it have to do with ardour and Qt.

• remove ostensibly dead code, which according to the X11 API should not be required in the first place.

Umpf, how stupid. This is about GTK2 code, not about X11 code.

[brummer10]

As I said, I'm tiered about all this, and I know already that I'm on a lost position. So, why the hell should I open a pull request ever again against suil. NEVER EVER AGAIN.

[drobilla]

@brummer10 and you may understand why I'm not about to just take your word for it that the changes are correct, your plugins are correct, and everyone else's are wrong.

Regardless, no, I don't think those plugins are right and your changes were the only problem. Both are clearly broken themselves in different ways. All of this is a complete mess, and it's clear that I need to personally write simple plugins that do all of this stuff, change the suil implementation if necessary, and test these things. Then it will be the case that plugins need to be fixed if they are broken. Yours included.

[drobilla]

why the hell should I open a pull request ever again against suil. NEVER EVER AGAIN.

If this is your attitude, good. Please don't.

[brummer10]

@x42 Still, you know that your plug is broken on Qt:

[drobilla]

Reverted here 7f8737eb332aaa2a0b5a4b73096770f1478af9c7 which also adds a better default size fallback and a clearer description of why this situation exists.