Double-click doesn't distribute area evenly

[Egmont Koblinger]

Have a giant (perhaps maximized) terminator window. Press Ctrl+Shift+E maybe 3-7 times to have 4-8 panes.

Double click on one of the separators.

Excepted: All panes have the same width.

Actual: The width is distributed quite unevenly. If you have >= 5 panes, some terminals on the right totally disappear (get a width of 0).

[Egmont Koblinger (egmont-gmail)]

terminatorlib/paned.py _do_redistribute()

In the first loop, the length of "curr" is always 2. This suggests to me that terminator never has a hpaned or vpaned with >= 3 children. It only has 2 children, and (according to how you did split them) such a child may also be a same kind of [hv]paned widget, and so on.

number_splits, avail_pixels, handle_size and arr_sizes[] have the correct values at the middle of the method, so the bug is probably in the bottom loop.

[Egmont Koblinger (egmont-gmail)]

It's damn complicated.

do_redistribute is already called 3 times, no clue why. With 5 panes, set_pos on the underlying [hv]paned objects is called a total of about 70 times!

Roughly what I believe happens is: do_redistribute tries to set the position of the 4 (in this case) separators, but each time it sets one, the side that contains multiple panes starts to reorganize itself (expand/shrink evenly) so it begins emitting these kinds of events that will also lead to adjustment of the separator, and also trigger this same story yet another level deeper.

The key to the solution would probably be to disconnect certain signals for the duration of do_redistribute() so that everything's in the direct control of this method and no other indirect signals are raised.

[Stephen Boddy (stephen-j-boddy)]

That code was one of my earliest contributions, when I had even less of a clue than I do now. It was horrid, and I shudder now when I think of looking at it again. Unfortunately GTK's splitter is rubbish in comparison to the Qt one. with Qt a splitter can have as many children as you want, and everything is sensible. In GTK a splitter is only allowed two children, and you have to nest the crap out of it. This of course makes things like redistribute pretty god-awful to code. I fantasize about the day that someone produces a Qt like Splitter for GTK. The only thing I would defend, is that at least I got the thing working... mostly.

It got so complicated with things moving around, not being where they should etc that, yes, I gave in and used a sledgehammer of repeatedly calling the redistributes. When someone like you (i.e. a real programmer) calmly says that turning off signals will make this much better, then I realise how little of GTK I really understand.

[Egmont Koblinger (egmont-gmail)]

I just looked at [hv]paned and was shocked that it only supports 2 children. And I can't find the handlebar on its own as a widget to be put inside a [hv]box between many children. So this recursive crap is indeed required. And it's damn hard to get it right. I'm not sure disconnecting the signals is the best approach, it's just something that occurred to me right now. (I'll come up with a different one in a separate bug very soon!)

Rest assured, I really don't know much about this part of gtk!! I just go along now and try to discover stuff and some up with ideas that may not even work.

[Egmont Koblinger (egmont-gmail)]

Sorry, didn't mean "crap" as its quality, meant it that it's a crap situation that we need it.

[Egmont Koblinger (egmont-gmail)]

As for disconnecting a signal: A typical pattern I believe is to set some boolean in the object like "already_handling_a_resize_stand_back_and_let_me_do_it" and then the handles are not technically disconnected, just start by checking for this boolean and bail out if it's set.

[Egmont Koblinger (egmont-gmail)]

Bug 1521765 is relevant.

[Stephen Boddy (stephen-j-boddy)]

"Sorry, didn't mean "crap" as its quality, meant it that it's a crap situation that we need it."

But you were right on both counts :-D Funny/weird story: I was struggling so much it actually helped when I started using single letter variables so I could "see" the algorithm. I think that is the only time I remember that happening.

[Egmont Koblinger (egmont-gmail)]

Actuall, arr_sizes is computed incorrectly, it should be:

for i in range((avail_pixels - (number_splits * handle_size)) % (number_splits + 1)):
    ...

But it's value is not used at all later on, which is another bug.

[Stephen Boddy (stephen-j-boddy)]

Bleurgh!

I got nothing. It 's probably just experimental or debugging cruft that I missed to clean up somehow. The log info is useless, because it was one of those things where by the end it, it sorta works, but you have no clue how you got there.

[Egmont Koblinger (egmont-gmail)]

I've been working on this for like 2 hours now, and I have no freaking clue. Terminator tries to set the position to the correct desired value. Someone always comes along and reverts it to the previous.

After setting the new value, immediately reading it back gives it back perfectly. And then there's a call to new_size() which who knows why tries to read this value and write it back, but when it reads it it's reverted to its previous undesired value.

I think I've covered all the possible code paths where Terminator would revert it, and no, it's not Terminator. Must be Gtk internally doing some terrible things.

[Stephen Boddy (stephen-j-boddy)]

If this is vexing the man who got text reflow into VTE, then I no longer feel quite so stupid :-D

[Egmont Koblinger (egmont-gmail)]

That one had nothing to do with Gtk :D

[Egmont Koblinger (egmont-gmail)]

Got it!

It's a one-liner.

But let me tease you for a day or two, I'm not showing it yet :P

[Egmont Koblinger (egmont-gmail)]

This is an easy warmup. Some bits will be required for the actual fix. I recommend to apply everything.

- Counting the number of panes instead of the number of separators is IMHO easier to follow mentally, and seems to lead to fewer ±1 corrections. Not mandatory at all, it's just my recommendation.

- single_size received a safeguard so that it can't underflow to negative.

- As mentioned previously, arr_sizes was calculated incorrectly. Even though it's unused, now that we've spotted a bug I think we should rather fix it than leave it broken. It was a copy-paste error, and instead of having a duplicated formula I moved the common bits to a new variable.

- The last hunk is definitely needed. For some weird reason (probably it's legacy/hack and could be cleaned up) we try to set the separator both synchronously and asynchronously as an idle step, but the two values were different. Again, instead of copy-pasting the same formula twice, I preferred to go with a temporary variable.

[Egmont Koblinger (egmont-gmail)]

This is another cleanup. Not sure if it belong here or to bug 1520902. Adding here because this is the bug that's being worked on now.

I didn't like the exact semantics of "ratio" denoting the middle of the handlebar within the entire available size. This has the weird property that it can't reach 0.0 or 1.0, except if handlebar size happens to be zero. I have a guts feeling that this approach is prone to some subtle errors.

I find it cleaner if the handle size is irrelevant, and ratio encodes the proportion of the two terminals to each other. That is, the ratio as if the handlebar was taken out and the entire available area was that much smaller.

Also note the beautiful self-assignment "handle_size = handle_size" :)

Please apply this too.

[Egmont Koblinger (egmont-gmail)]

Forget the previous, use this one please.

[Egmont Koblinger (egmont-gmail)]

"non_separator_width" should be called "non_separator_size" or similar, since it might be height as well.

[Egmont Koblinger (egmont-gmail)]

Okay, I don't want to be evil, so here are a couple of hints followed by the solution... in a little bit cryptic form, but it should be trivial to decode. Enjoy! :)

[Stephen Boddy (stephen-j-boddy)]

OK, so you're not entirely evil... I can get to text under hint 1, and follow the action, but nothing presents itself as the next logical step. I'm wondering if perhaps your more recent gtk is giving you some other possibilities than me.

[Egmont Koblinger (egmont-gmail)]

About the puzzle, those bunch of lettters... it's a mere coincidence of 1/3 chance that it does not end in 1 or 2 '=' characters. And if you get binary garbage, you're not decoding the correct bunch of letters.

[Stephen Boddy (stephen-j-boddy)]

Well I finally figured out the puzzle, which took longer than it should because I'm tired with a grade "A" headache.

Proof: "pack1 is okay, only pack2 needs change"

When I have some more time (hopefully this weekend) I'll apply your previous patches, and have a dig further into this. The headache means I'm only reading your words, not really understanding them :-(

[Egmont Koblinger (egmont-gmail)]

I used base64 and rot13.

[Egmont Koblinger (egmont-gmail)]

Rationale behind the main fix, with two typos corrected:

The position is stored as an absolute offset from the top/left. In order for
this not to change on a resize, the first child needs to have 'resize'
disabled, and the second child needs to have it enabled. Adjust pack1()'s and
pack2()'s _arguments_ accordingly. (pack1 is okay, only pack2 needs change.)

Note: This changes the behavior described in bug 1521765, dragging a separator
no longer distributes (or takes up) the extra area evenly, but adds (removes)
to one of the terminals. If this is undesired, I think the solution would be
to _enable_ the 'resize' property of child2 only for the duration of handling
this double-click. I'd leave this up to you. It's probably tricky because of
the steps executed via an add_idle(), so you probably can't revert the
property synchonously. Dunno, just a guess. Up to you to figure out :)

[Egmont Koblinger (egmont-gmail)]

Actually the new behavior is closer to the one I find ideal in bug bug 1521765. For the sake of simplicity, we might just leave things this way. I doubt anyone would complain and ask for the previous behavior :)

[Stephen Boddy (stephen-j-boddy)]

This is cool stuff! I have however seen that, as well as the thing you mention, this also isn't very nice when resizing or (un)maximising the window. The terminals don't take up new space, or adapt to having less.

I think possibly creating a function to enable/disable resize on child2 might be the way to go, then wrap the _do_distribute with it (possibly as a decorator). This might also be doable for the splitter drag. That way, redistribute works, the dragging is absolute and "nearly ideal" for you, but resize/maximise will adapt as desired. I wonder if maybe a "redistribute on drag" might be added for those who prefer the old behaviour.

One other very strange curiosity was when I tried to backport. In gtk2 simply applying the fix patch on it's own (i.e. resize = True for child2) causes the focus to be lost in both children. I have no idea why that change should cause that problem. It doesn't really make sense.

[Stephen Boddy (stephen-j-boddy)]

OK, figured out the gtk2 oddity I think. It seems like somehow having resize on, and the grab_focus before flushing pending events, causes the loss of focus. The super easy fix is to put the grab after the flush (split_axis in paned and window.) Just for code alignment I'll probably switch their position in gtk3 provided that doesn't regress anything else.

I still need to come up with an "on/off" function for the resize property. Once I figure that out I'll commit the full queue for both series.

[Egmont Koblinger (egmont-gmail)]

> the dragging is absolute and "nearly ideal" for you

What's "ideal" for me is still unclear even to me, and I'm wondering if that can be implemented at all :D

> I wonder if maybe a "redistribute on drag" might be added for those who prefer the old behaviour.

I'd be glad if we had one reasonably working solution, let alone offering preferences :)

[Stephen Boddy (stephen-j-boddy)]

OK, so I think *finally* figured out the way to switch the resize parameter off, then on, just for the redistribute. Two things left:
1) Merge it all up with your other patches to see if it still works fully. On its own it there is still occasional unevenness when there's large numbers of splits, but I think one of your cleanups improved that before.
2) Break it out into wrapper function(s), and see if they can be called on the begin-drag/end-drag signals of the splitter.

[Stephen Boddy (stephen-j-boddy)]

Argggggggggghhhhhhh!!!!! This is driving me nuts!

Mostly it works, but I have a bug with a deeply nested set of panes. When setting the resize property to False for the last time, it dicks with the position of one of the splitters. Why, GTK? why?

Even using your fix, and only turning off the resize property causes the bloody thing to mess up. I think I'll have to walk away and come back with fresh eyes.

[Stephen Boddy (stephen-j-boddy)]

After loads of testing, experimenting, and going around in circles, something occurred to me. What if we set resize on child1 too in the same if/else block. Rebalance still works, but unfortunately that makes resizing inconsistent. Sometimes the splits go to the outer edge of the window, sometimes they resize. This is dependent on the sequence the splits were made. The two torture test cases for showing this are the following:
1) Open new window, and do 7 x Ctrl+Shift+O, rebalance, and then play with the vertical window size.
2) Open a new window (which you will probably want to maximise vertically) then do the following sequence
    4 x Ctrl+Shift+O
    3 x Alt+UpArrow
    2 x Ctrl+Shift+O
    3 x Alt+UpArrow
    3 x Ctrl+Shift+O
    3 x Alt+UpArrow
    2 x Ctrl+Shift+O
    Then use the top splitter to rebalance, and then play with the vertical window size.

They show very different behaviour. For 1) terminals in turn disappear at the bottom of the window, and don't come back. For 2) they scale how we want. This is obviously not acceptable.

I tried setting line 81 to be True too, but this gives the original behaviour: The window resize works for both torture cases, but the rebalance takes multiple calls depending on how deep the nesting is.

My earlier original idea for this was to only active resize=True for child2 while we do the rebalance, then revert to False... except... #*!$ing GTK! As near as I can figure this is what is happening (lots of tests, and monkey code later):

- When the child widget is packed the allocation is set based on the space available from the splitter
- User double-clicks to initiate rebalance
- A single call to the do_redistribute is made (no multiple calls needed)
- I have a pre function that brute forces all visible child2's to resize = True
- The rebalance happens, and I flush the event buffer, and sleep for a few seconds. Everything is laid out perfectly
- The same pre function is called, but this time it sets the resize to False
- Both the torture cases have splitters that move to the wrong location.

Arrrrrrghhhhhh!

From digging through GTK docs, and even the source code this is what is happening as near as I tell.
- When the resize is set to True it and the rebalance is being done, the allocations of the child widgets do not get updated
- When the resize property is set back to False a notify event is sent to the child widgets
- As soon as the on_button_click (which calls do_redistribute) function exits and hands control back to the main gtk loop, the terminal widgets receive this notification, and renegotiate their allocation with the splitter... based on the values they had from before Resize was set to True! So the splitter tries to oblige, and sometimes moves around to accommodate.

I simply cannot wrap my head around enough of these areas to come up with a way to deactivate/delete that notification. I can't find a way to quietly change the allocation without causing splitters to move. The last thought that just occurred to me is to override the splitter function that responds to the size request, and force it to give the new allocations, but I have not tried that at all, or even checked the docs to ...

[Stephen Boddy (stephen-j-boddy)]

Just a "wouldn't it be nice" moment:
https://github.com/AdaCore/gtkada/blob/master/testgtk/create_splittable.adb (like .c)
https://github.com/AdaCore/gtkada/blob/master/testgtk/create_splittable.ads (like .h)
which uses:
https://github.com/AdaCore/gtkada/blob/master/src/gtkada-multi_paned.adb
https://github.com/AdaCore/gtkada/blob/master/src/gtkada-multi_paned.ads

And gives the following attached screenshot (from the testgtk program).

It's actually more than we would need, because I think a single widget contains is going all those splits. It also has a non-opaque mode that would be a trivial answer to the "too many huge buffers with reflow will resize badly" concern. Of course I have never looked at Ada before today, so have no chance of porting, and don't even understand if we could achieve the rebalancing using that widget.

[Egmont Koblinger (egmont-gmail)]

Cool :)

No clue if we should go for it right away, or try to patchwork the current solution to something not horribly broken and try to port for 2.1 maybe.

[Stephen Boddy (stephen-j-boddy)]

It was very definitely a tongue-in-cheek comment. I think it would be quite difficult to port it without decent knowledge of GTK, Ada, and Python. And that's without even getting into the legality. It is licensed as GPLv3 while Terminator is GPLv2 only (no plus). This means different parts being differently licensed, and I have no idea how that works.

Ironically, it might also be *too* complicated :-) If all terminals are children of a single widget, how do you determine what is sibling, what is ancestor, and what is descendant? (the Shift and Super modifiers for the rebalance)

There is also the question of performance of a pure Python implementation, as in it might suck. Of course C would be quick, but that is not something I could hope to help with, and would probably require getting it upstreamed to GTK, for which there are no guarantees. And of course then you become dependant on a really recent version of GTK on top of everything.

[Egmont Koblinger (egmont-gmail)]

Take a close look at the documentation of gtk_paned_add1() and gtk_paned_add2(). They are exactly as pack1() and pack2() with the attributes used in my patch, that is, resize only the second child :)

[Egmont Koblinger (egmont-gmail)]

I can't see your pending patches, but here's my attempt to solve the issues.

To be applied _instead of_ the previous version of this patch, that is, keep pack2(..., False, ...).

What it does is it sets the 'resize' property of chid2 only for the duration of do_redistribute().

do_redistribute() did tons of GObject.idle_add(), I've no clue why, and I really hate these (these cause plenty of troubles usually) so I went with the synchronous version and it seems to work. The advantage is that I can safely revert the 'resize' property in an idle hook and be sure that that'll be executed after Gtk is finished with everything.

Please test and let me know if it works reliably for you.

(By the way, yet another, absolutely totally fundamentally different approach could be to follow the spirit of bug 1522542 comment 11's patch, destroy the tree of paneds are rebuild everything, giving the separators the exact sizes we compute.)

[Stephen Boddy (stephen-j-boddy)]

Did you try this with both the torture tests in comment #33? This looks very, very similar to what I came up with (including dropping all those idle_add's), but I still had problems with the torture tests. I'll take a look when I get a chance, but I have a few things to get sorted this week/before Christmas, so I'm struggling to make much time for Terminator short term.

[Egmont Koblinger (egmont-gmail)]

They work reasonably well. Maybe we should create screencasts, it's quite hard to describe the behavior by words.

If there's any difference in the behaviors we see, it can still be due to a difference in our patches, or due to different Gtk version.

Double clicking distributes the area perfectly evenly if there's enough room for each terminal to be at least titlebar + 2 rows tall. Below that size I think it'd require to understand the deepest and darkest corners of Gtk to understand what's happening, and I don't care about it at all since terminals of that size are unusable anyways.

Resizing the windows distributes the area mostly (but not perfectly) evenly. This boils down to at least these things:

- Again, if you go below the height of titlebar + 2 rows, ugly things happen which I don't give a sh.t about.

- All kinds of rounding errors, see e.g. bug 1520902.

- With a handle size > 0 and 3+ panes in the same orientation, Gtk's auto distribute just can't get the maths right. Example: Press Ctrl+Shift+O twice, double click on a separator, maximize the window, double click on a separator again, notice that it moves by a couple of pixels. Why? Let's dew the math. The first double click creates 3 terminals of equal height. For the inner paned, that's a separator position of 50%. For the outer paned, that's a separator position smaller than 33.333%, since on one side it has 1 terminal, on the other side it has 2 terminals + 1 separator, and that's more than the double of the previous. Let's say the separator position is 32.5%. Now you maximize. Gtk maintains 32.5% to remain the same, hence the bottom two terminals receive a tiny bit more extra space each than the one on the top. (They get the extra space that the separator would get if it also grew.) Double clicking again makes sure that they are of the exact same height.

Do you only see these problems, or do you face more serious ones?

I'd say for the first gtk3 tarball let's make sure that Terminator handles all the reasonably complex cases (that is: layouts that can be used in real life) reasonably correctly, but let's ignore the unusable extremes, and let's ignore if the extra area is not pixel-perfectly distributed evenly. We can always improve later.

[Egmont Koblinger (egmont-gmail)]

Just a small update so that it applies against the current bzr head.

[Stephen Boddy (stephen-j-boddy)]

Applied in gtk3 branch rev 1651.

Note to self: Perfect is the enemy of good enough. As Egmont says, there are some real minor quirks to this thing, but that is a battle for another day.