Bug#988951: regression: focus_path on last items no longer works properly

[Cyril Brulebois]

Cyril Brulebois  (2021-05-23):
> And right before getting some rest, it struck me that I should be
> looking into the size-request and size-allocate signals. The latter is
> the right one and that's indeed getting me the focus where it should be!
> \o/ I suppose this isn't entirely crazy since that's an idea you had as
> well. :)
> 
> … back to MR #12, I'm a little reluctant to changing the workaround at
> this stage. After all, I've run a bunch of test cases already, which all
> looked good, and while reverting the initial workaround would mean less
> code / technical debt, all those lines should go away in bookworm
> anyway. So I think I would *slightly* prefer paper over the focus
> regression (this bug) with an extra callback. I would have to need to
> power up coffee and brain a little more though. :)

Apart from introducing a memory leak, that alone is sufficient to fix
the glitch:
  https://salsa.debian.org/installer-team/cdebconf/-/merge_requests/13

For reference, here's what size-allocate events can look like. For the
avoidance of doubt, I logged x,y and w(idth),h(eight) on two separate
lines to ease comparing values, but each x,y + w,h couple of lines
come from the same size-allocate event (a GdkRectangle is provided):

May 23 19:57:01 debconf: - update title - ← mirror country selection
May 23 19:57:01 debconf: allocate: x,y=8x49
May 23 19:57:01 debconf: allocate: w,h=208x60
May 23 19:57:01 debconf: allocate: x,y=8x49
May 23 19:57:01 debconf: allocate: w,h=722x368
May 23 19:57:01 debconf: allocate: x,y=8x49
May 23 19:57:01 debconf: allocate: w,h=740x368
May 23 19:57:01 debconf: allocate: x,y=8x81
May 23 19:57:01 debconf: allocate: w,h=740x336
May 23 19:57:02 debconf: - update title - ← mirror hostname 
selection
May 23 19:57:02 debconf: allocate: x,y=8x49
May 23 19:57:02 debconf: allocate: w,h=241x60
May 23 19:57:02 debconf: allocate: x,y=8x49
May 23 19:57:02 debconf: allocate: w,h=722x368
May 23 19:57:02 debconf: allocate: x,y=8x49
May 23 19:57:02 debconf: allocate: w,h=740x368
May 23 19:57:02 debconf: allocate: x,y=8x66
May 23 19:57:02 debconf: allocate: w,h=740x351
May 23 19:57:02 debconf: allocate: x,y=8x95
May 23 19:57:02 debconf: allocate: w,h=740x322
May 23 19:57:02 debconf: - update title - ← proxy => Back
May 23 19:57:04 debconf: - update title - ← mirror hostname 
selection again
May 23 19:57:04 debconf: allocate: x,y=8x49
May 23 19:57:04 debconf: allocate: w,h=241x60
May 23 19:57:04 debconf: allocate: x,y=8x49
May 23 19:57:04 debconf: allocate: w,h=722x368
May 23 19:57:04 debconf: allocate: x,y=8x49
May 23 19:57:04 debconf: allocate: w,h=740x368
May 23 19:57:04 debconf: allocate: x,y=8x66
May 23 19:57:04 debconf: allocate: w,h=740x351
May 23 19:57:04 debconf: allocate: x,y=8x95
May 23 19:57:04 debconf: allocate: w,h=740x322


Cheers,
-- 
Cyril Brulebois (k...@debian.org)
D-I release manager -- Release team member -- Freelance Consultant


signature.asc
Description: PGP signature

Bug#988951: regression: focus_path on last items no longer works properly

[Cyril Brulebois]

Simon McVittie  (2021-05-23):
> > > I've tried various things like having the focus_path happens in a
> > > “_later” indirection using the same kind of logic as Simon
> > > introduced for setting the text (with a different priority), but
> > > that would happen waaay before set_text_in_idle anyway.
> > 
> > Please could you share what you tried so I can check whether it's
> > doing something wrong? I feel as though this approach ought to be
> > workable
> 
> Actually, never mind: the code structure for this gets increasingly
> messy, with components needing to know about implementation details of
> unrelated components. I think that's technical debt we probably don't
> want to pay.

I was willing to have that for buster, given we're planning on
rewriting the whole thing for GTK 3 next release cycle away. But indeed,
that's quite messy, keeping track of other things we shouldn't be caring
about. And as mentioned, we are a few events behind anyway.

> I have a couple of ideas for possible ways to deal with this.
> 
> One idea is to undo my workaround for #988787 on the cdebconf side,
> and instead, hook onto the GtkTextView's size-request signal and force
> it to be allocated with at least some arbitrary reasonable size (I'm
> trying 300px). This will hopefully still work around #988787, and if
> it doesn't, we have the workaround #988786 in GTK as a fallback.
> https://salsa.debian.org/installer-team/cdebconf/-/merge_requests/12

I'll get back to that in a minute…

> Another idea, still in cdebconf, is to connect to whatever signal is
> emitted when the GtkTreeView is resized (I think this is
> size-allocate?), and take that opportunity to re-adjust the scroll
> position so the (first) selected row comes into view. However, I'm a
> bit concerned that this could itself cause an infinite loop like
> #988787 - I'd have to check the GtkTreeView implementation to make
> sure scrolling cannot schedule a resize.

I've also toyed with the idea of sending a specific “kibi-event” from
the set_text_in_idle that would have its dedicated, not one-shot
callback (focus_path disconnects itself) on the TreeView side, but that
one also comes in too early.

And right before getting some rest, it struck me that I should be
looking into the size-request and size-allocate signals. The latter is
the right one and that's indeed getting me the focus where it should be!
\o/ I suppose this isn't entirely crazy since that's an idea you had as
well. :)

… back to MR #12, I'm a little reluctant to changing the workaround at
this stage. After all, I've run a bunch of test cases already, which all
looked good, and while reverting the initial workaround would mean less
code / technical debt, all those lines should go away in bookworm
anyway. So I think I would *slightly* prefer paper over the focus
regression (this bug) with an extra callback. I would have to need to
power up coffee and brain a little more though. :)

Thanks for all your feedback, much appreciated as always.


Cheers,
-- 
Cyril Brulebois (k...@debian.org)
D-I release manager -- Release team member -- Freelance Consultant


signature.asc
Description: PGP signature

Bug#988951: regression: focus_path on last items no longer works properly

[Simon McVittie]

On Sun, 23 May 2021 at 14:43:33 +0100, Simon McVittie wrote:
> When the GtkTreeView is resized as a result of the text being added,
> the top left corner of the visible area is what's preserved; if its
> selected row was near the bottom, the result is that the selected row
> is no longer visible.

You can see similar behaviour in the "Tree View -> List Store" example in
gtk-demo, gtk3-demo or gtk4-demo (from gtk2.0-examples, gtk-3-examples or
gtk-4-examples respectively). If you select an item low down the list,
then resize the window, you'll see that it's the top left corner that
stays fixed within the window, and the selected item doesn't necessarily
stay visible.

smcv

Bug#988951: regression: focus_path on last items no longer works properly

[Simon McVittie]

On Sun, 23 May 2021 at 11:31:58 +0100, Simon McVittie wrote:
> >  - Slightly shorter (`kvm -m 1G -cdrom mini.iso`, no disk layout or even
> >disk required), pick a language like French and all default choices,
> >until the mirror country selection, pick the very last one
> >(États-Unis), and on the mirror host selection, pick the very last
> >one again (the actual hostname doesn't matter). Now, on the next
> >dialog, hit “Revenir en arrière” (Back), and see the selected
> >hostname isn't focussed. Another step back shows the selected country
> >isn't focussed either. That should happen with other languages as
> >well, using French has the main advantage for me to get the
> >appropriate keyboard layout automatically plus get two “back” steps
> >that exhibit the problem (other countries might not have a mirror
> >list as big as the US one).
> 
> I can read French somewhat better than Swedish (and infinitely better
> than Sinhala where I don't even recognise the glyphs), so this is probably
> the more convenient test-case :-)

I can reproduce something similar with more defaults, like this:

* accept defaults (in particular en_US) until you reach the list of
  USA mirrors
* choose the last mirror
* the next question asks for a proxy; do not answer, but instead "Go back"
* good result: the mirror you chose is focused and visible
* bad result: the mirror you chose is focused but you have to scroll with
  the mouse to be able to see that

> > I suppose we have a slightly taller widget at
> > first, we scroll down to the bottom; then when set_text_in_idle happens,
> > the widget is resized slightly smaller, the position is not correct
> > anymore (it's no longer “full-bottom” but a little higher as seen in the
> > scrollbar), and the selected line gets out of sight.

I think you're correct. Deferring addition of the text to the GtkTextView
means the initial window layout for select/multiselect questions will
be done based on the assumption that there is no text, which means the
GtkTreeView will receive nearly the full window height. When we scroll
the GtkTreeView to bring the selected row into view, that also happens
before the text is added (I think we probably draw one frame without the
text, then add the text in the next frame).

When the GtkTreeView is resized as a result of the text being added,
the top left corner of the visible area is what's preserved; if its
selected row was near the bottom, the result is that the selected row
is no longer visible.

> > I've tried various things like having the focus_path happens in a
> > “_later” indirection using the same kind of logic as Simon introduced
> > for setting the text (with a different priority), but that would happen
> > waaay before set_text_in_idle anyway.
> 
> Please could you share what you tried so I can check whether it's doing
> something wrong? I feel as though this approach ought to be workable

Actually, never mind: the code structure for this gets increasingly messy,
with components needing to know about implementation details of unrelated
components. I think that's technical debt we probably don't want to pay.

I have a couple of ideas for possible ways to deal with this.

One idea is to undo my workaround for #988787 on the cdebconf side,
and instead, hook onto the GtkTextView's size-request signal and force
it to be allocated with at least some arbitrary reasonable size (I'm
trying 300px). This will hopefully still work around #988787, and if
it doesn't, we have the workaround #988786 in GTK as a fallback.
https://salsa.debian.org/installer-team/cdebconf/-/merge_requests/12

Another idea, still in cdebconf, is to connect to whatever signal is
emitted when the GtkTreeView is resized (I think this is size-allocate?),
and take that opportunity to re-adjust the scroll position so the (first)
selected row comes into view. However, I'm a bit concerned that this
could itself cause an infinite loop like #988787 - I'd have to check
the GtkTreeView implementation to make sure scrolling cannot schedule
a resize.

smcv

Bug#988951: regression: focus_path on last items no longer works properly

[Simon McVittie]

On Fri, 21 May 2021 at 21:54:15 +0200, Cyril Brulebois wrote:
> focussing on the last items of a GtkTreeView
> no longer works correctly

FYI I didn't receive the initial message reporting this bug, only the
follow-up, despite you having X-Debbugs-Cc'd me. I'm not sure why. Luckily
`bts show --mbox` exists.

>  - Slightly shorter (`kvm -m 1G -cdrom mini.iso`, no disk layout or even
>disk required), pick a language like French and all default choices,
>until the mirror country selection, pick the very last one
>(États-Unis), and on the mirror host selection, pick the very last
>one again (the actual hostname doesn't matter). Now, on the next
>dialog, hit “Revenir en arrière” (Back), and see the selected
>hostname isn't focussed. Another step back shows the selected country
>isn't focussed either. That should happen with other languages as
>well, using French has the main advantage for me to get the
>appropriate keyboard layout automatically plus get two “back” steps
>that exhibit the problem (other countries might not have a mirror
>list as big as the US one).

I can read French somewhat better than Swedish (and infinitely better
than Sinhala where I don't even recognise the glyphs), so this is probably
the more convenient test-case :-)

> My first hunch was that the focus_path callback (one-shot call, it
> disables itself once it has triggered gtk_tree_view_scroll_to_cell on
> the first expose event) happens before the set_text_in_idle one, and
> that's indeed correct. I suppose we have a slightly taller widget at
> first, we scroll down to the bottom; then when set_text_in_idle happens,
> the widget is resized slightly smaller, the position is not correct
> anymore (it's no longer “full-bottom” but a little higher as seen in the
> scrollbar), and the selected line gets out of sight.
> 
> I've tried various things like having the focus_path happens in a
> “_later” indirection using the same kind of logic as Simon introduced
> for setting the text (with a different priority), but that would happen
> waaay before set_text_in_idle anyway.

Please could you share what you tried so I can check whether it's doing
something wrong? I feel as though this approach ought to be workable -
an idle callback with higher priority is guaranteed to happen sooner
(but note that smaller numbers are higher priority), and in my experience
idle callbacks of the same priority seem to get run in the order they
were scheduled.

The stuff here with expose events is beyond my GTK knowledge, but I
feel as though it's probably not idiomatic, and there might well be a
higher-level way to do it.

Note that the use of focus_path says

We need to focus the row *after* the widget realization, see #340007

but #340007 was a bug in the old GTK-on-DirectFB installer, and the
message opening the bug specifically said that it didn't affect
GTK-on-X11. So perhaps this delay isn't even necessary any more?

smcv

Bug#988951: regression: focus_path on last items no longer works properly

[Cyril Brulebois]

Cyril Brulebois  (2021-05-21):
> There might be simpler (shorter) ways to trigger this but the following
> is robust:
> 
>  - Initially detected while testing an encrypted LVM install in Swedish
>(confirming all hangs go away), when reaching the partition layout
>confirmation dialog, the selected entry is the last one in the list,
>but the selection isn't seen. One might wonder where the focus is,
>why no entry was selected, etc. Since that can happen in various
>places, I think users might get confused. That should not be specific
>to Swedish, it just happens to be the first occurrence I noticed.

That particular part seems solved with my current hackish patch series.

>  - Slightly shorter (`kvm -m 1G -cdrom mini.iso`, no disk layout or even
>disk required), pick a language like French and all default choices,
>until the mirror country selection, pick the very last one
>(États-Unis), and on the mirror host selection, pick the very last
>one again (the actual hostname doesn't matter). Now, on the next
>dialog, hit “Revenir en arrière” (Back), and see the selected
>hostname isn't focussed. Another step back shows the selected country
>isn't focussed either. That should happen with other languages as
>well, using French has the main advantage for me to get the
>appropriate keyboard layout automatically plus get two “back” steps
>that exhibit the problem (other countries might not have a mirror
>list as big as the US one).

That one is strange:
 - getting back to mirror hostname selection is still buggy;
 - getting back to mirror country selection is no longer buggy.

I'm not sure whether my current approach can be refined to work in all
cases, but solving at least the obvious “forward-only” (= no step back)
issue that was seen at least on the layout confirmation screen… seems
worth considering for bullseye, even if it's ugly and doesn't solve all
cases? And maybe users are going to tell us about other occurrences
because of different languages/options combinations that result in
different layout, triggering the scrolling issue or not.

> Next on my list of things to try was adding a pointer to the frontend
> object (and its `data` member) so that we could keep the callback alive
> until set_text_in_idle has done its job. I thought it might need some
> mutex or locking around a counter of pending set_text calls and I
> haven't touched that yet. And today, the following rang a bell… :)
>   https://salsa.debian.org/installer-team/cdebconf/-/merge_requests/7

Merging that on top of my code didn't seem to make any difference
regarding the “back to the mirror hostname selection” issue that
remains.

I'll try and tidy up my patches, and push a branch somewhere, also
posting traces that show that events seem to have happened in the right
order.


Cheers,
-- 
Cyril Brulebois (k...@debian.org)
D-I release manager -- Release team member -- Freelance Consultant


signature.asc
Description: PGP signature

Bug#988951: regression: focus_path on last items no longer works properly

[Cyril Brulebois]

Package: cdebconf-gtk-udeb
Version: 0.258
Severity: important
X-Debbugs-Cc: Simon McVittie 

Hi,

The 0.258 update is *very* important for us since it makes extra sure
(together with libgtk2.0-0-udeb 2.24.33-2) we don't run into relayout
loops meaning hangs from a user point of view.

Yes, it comes at a price: focussing on the last items of a GtkTreeView
no longer works correctly.

There might be simpler (shorter) ways to trigger this but the following
is robust:

 - Initially detected while testing an encrypted LVM install in Swedish
   (confirming all hangs go away), when reaching the partition layout
   confirmation dialog, the selected entry is the last one in the list,
   but the selection isn't seen. One might wonder where the focus is,
   why no entry was selected, etc. Since that can happen in various
   places, I think users might get confused. That should not be specific
   to Swedish, it just happens to be the first occurrence I noticed.

 - Slightly shorter (`kvm -m 1G -cdrom mini.iso`, no disk layout or even
   disk required), pick a language like French and all default choices,
   until the mirror country selection, pick the very last one
   (États-Unis), and on the mirror host selection, pick the very last
   one again (the actual hostname doesn't matter). Now, on the next
   dialog, hit “Revenir en arrière” (Back), and see the selected
   hostname isn't focussed. Another step back shows the selected country
   isn't focussed either. That should happen with other languages as
   well, using French has the main advantage for me to get the
   appropriate keyboard layout automatically plus get two “back” steps
   that exhibit the problem (other countries might not have a mirror
   list as big as the US one).

With both gtk+2.0 and cdebconf being uploaded recently, I've made sure
to determine what triggers this:
 - bulleye: OK
 - unstable: KO
 - bullseye + gtk2.0: OK
 - bullseye + cdebconf: KO

My first hunch was that the focus_path callback (one-shot call, it
disables itself once it has triggered gtk_tree_view_scroll_to_cell on
the first expose event) happens before the set_text_in_idle one, and
that's indeed correct. I suppose we have a slightly taller widget at
first, we scroll down to the bottom; then when set_text_in_idle happens,
the widget is resized slightly smaller, the position is not correct
anymore (it's no longer “full-bottom” but a little higher as seen in the
scrollbar), and the selected line gets out of sight.

I've tried various things like having the focus_path happens in a
“_later” indirection using the same kind of logic as Simon introduced
for setting the text (with a different priority), but that would happen
waaay before set_text_in_idle anyway.

I've also tried to implement a “double-tap” approach, letting the
callback be called twice, so that we would focus first, let the text be
set and get a new expose event, and re-focus. But it seems the amount of
events we need to reach this point is not constant (I didn't conduct a
real study but it seems one might need up to 4-5 such events).

Next on my list of things to try was adding a pointer to the frontend
object (and its `data` member) so that we could keep the callback alive
until set_text_in_idle has done its job. I thought it might need some
mutex or locking around a counter of pending set_text calls and I
haven't touched that yet. And today, the following rang a bell… :)
  https://salsa.debian.org/installer-team/cdebconf/-/merge_requests/7


I'm happy to be told whether the vague idea above looks like a
workaround that could or even should work before diving a little more
into it, and/or to be suggested better ideas!


Cheers,
-- 
Cyril Brulebois (k...@debian.org)
D-I release manager -- Release team member -- Freelance Consultant