After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 793435 - GtkScrollbar warning "How does the code know the size to allocate?"
GtkScrollbar warning "How does the code know the size to allocate?"
Status: RESOLVED FIXED
Product: vte
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-02-13 19:42 UTC by Christian Persch
Modified: 2018-03-04 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Christian Persch 2018-02-13 19:42:34 UTC
Cloning bug 769566 to add a workaround for vte while leaving the gtk+ bug for them to resolve.

The patch under discussion is attachment 368224 [details] [review].
Comment 1 Christian Persch 2018-03-03 12:05:54 UTC
I'm ok with working around this warning on gtk3, but I'd like to still preseve it in the hope of gtk4 being fixed...

So I propose to move the adjustment changing code to a new method, and only structure the rest of the code around #if !GTK_CHECK_VERSION(3,90,0) so the change only happens on !gtk4.
Comment 2 Egmont Koblinger 2018-03-03 19:30:20 UTC
I'd rather not introduce conditionals – or at least as long as we don't win anything by them, just have a more complicated code.

---

What's the plan for gtk4? Have a single active VTE branch with #if GTK_CHECK's, or separate branches for gtk3 and gtk4 (maybe even 4.1, 4.2 etc. later – this whole story with versioning is just not yet clear to me)?

(Any up-to-date info about the current schedule plans? I couldn't find anything fresh.)

---

Current codebase goes like:
- changes to adj value          => async
- changes to adj upper/lower    => async
- changes to adj step/page size => sync

The first two can occur while producing output, the third (which triggers the warnings) only while resizing.

As per bug 709089, as well as an irrelevant bug 771899 comments 20-21, I _believe_ we should do everything synchronously. GTK+ _should be_ smart enough to optimize things out for us without any noticeable performance penalty, and we'd have a cleaner code (plus automagically fix 709089).

I _guess_ the reason for the current code is that the first two, which can happen during terminal activity, used to be slow if done synchronously, or were falsely believed to be slow, or they are indeed slow (as opposed to my assumption).

Alas, the fully sync approach would mean we can't get rid of the warnings. Plus, gtk4 might even change whether things are slow or fast. That's why I suggest that we delay this investigation 'til gtk4.

---

Right now, however, moving in the opposite direction and making even the 3rd bullet point above happen asynchronously pretty sure doesn't introduce any regressions. (In fact it might even fix temporary glitches due to upper/lower and page size being out of sync since we'd modify the params in a single {freeze,thaw}_notify, and we'd even get one step closer to using the convenience method gtk_adjustment_configure() (since ideally all params, including the value itself, would be modified at once).)

Plus this approach happens to eliminate the warnings, and gives us a simpler code (unless we stuff it with conditionals).

For now I think it's a clear win, and a new state from which I wouldn't go back to the current state even in gtk4, or even in gtk3 if it fixed the warnings, that's why I don't see the point in conditionals. For gtk4 I'd go to the full synchronous version if we can, or keep the currently proposed new, fully asynchronous if we cannot.

I'm wondering, does this make sense...? :-/ Did I manage to convince you? :-)
Comment 3 Christian Persch 2018-03-03 19:58:56 UTC
(In reply to Egmont Koblinger from comment #2)
> What's the plan for gtk4? Have a single active VTE branch with #if
> GTK_CHECK's, or separate branches for gtk3 and gtk4 (maybe even 4.1, 4.2
> etc. later – this whole story with versioning is just not yet clear to me)?

I don't want to repeat the gtk 2->3 fiasco where I went with dropping gtk2 support immediately while there's still users out there. 

So this time, I want a unified vte, which depending on a configure switch, will build a gtk3 or gtk4 version of vte, and the code uses #if GTK_CHECK_VERSION() for that.

The 3.9x are unstable leading up to gtk 4.0 which will be API stable again, so I think we'll only support the final release.

> (Any up-to-date info about the current schedule plans? I couldn't find
> anything fresh.)

IIRC it's scheduled for this autumn; but we don't have to support it immediately. I'll probably look at gtk4 late this year/early next year, at the earliest.

> ---
> 
> Current codebase goes like:
> - changes to adj value          => async
> - changes to adj upper/lower    => async
> - changes to adj step/page size => sync
> 
> The first two can occur while producing output, the third (which triggers
> the warnings) only while resizing.
>
> As per bug 709089, as well as an irrelevant bug 771899 comments 20-21, I
> _believe_ we should do everything synchronously. GTK+ _should be_ smart
> enough to optimize things out for us without any noticeable performance
> penalty, and we'd have a cleaner code (plus automagically fix 709089).

Yes, I think you're right, everything _should_ done in the same phase (sync/async) here.
So it seems I mis-read the patch.
 
> I _guess_ the reason for the current code is that the first two, which can
> happen during terminal activity, used to be slow if done synchronously, or
> were falsely believed to be slow, or they are indeed slow (as opposed to my
> assumption).

They are probably indeed slow, since any adjustment changes will cause the scrollbar to queue a resize.

> Alas, the fully sync approach would mean we can't get rid of the warnings.
> Plus, gtk4 might even change whether things are slow or fast. That's why I
> suggest that we delay this investigation 'til gtk4.

Ok.
 
> ---
> Right now, however, moving in the opposite direction and making even the 3rd
> bullet point above happen asynchronously pretty sure doesn't introduce any
> regressions. (In fact it might even fix temporary glitches due to
> upper/lower and page size being out of sync since we'd modify the params in
> a single {freeze,thaw}_notify, and we'd even get one step closer to using
> the convenience method gtk_adjustment_configure() (since ideally all params,
> including the value itself, would be modified at once).)
> 
> Plus this approach happens to eliminate the warnings, and gives us a simpler
> code (unless we stuff it with conditionals).
> 
> For now I think it's a clear win, and a new state from which I wouldn't go
> back to the current state even in gtk4, or even in gtk3 if it fixed the
> warnings, that's why I don't see the point in conditionals. For gtk4 I'd go
> to the full synchronous version if we can, or keep the currently proposed
> new, fully asynchronous if we cannot.
> 
> I'm wondering, does this make sense...? :-/ Did I manage to convince you? :-)

Yes you did :-)
Comment 4 Egmont Koblinger 2018-03-03 20:09:57 UTC
(In reply to Christian Persch from comment #3)

> So this time, I want a unified vte, which depending on a configure switch,
> will build a gtk3 or gtk4 version of vte, and the code uses #if
> GTK_CHECK_VERSION() for that.

Fair enough. This means though that they should install under different filenames an SONAMEs, since the whole point is to allow gtk3 and gtk4 vte-based apps in parallel (as soon as all apps switch, we can drop the old one), for which we'll need the libraries (including their devel symlinks - what about the headers though?) to parallel installable. Dunno if gtk will provide some help here, or we should do it totally manually. (Or maybe I misunderstand something here?)

> They are probably indeed slow, since any adjustment changes will cause the
> scrollbar to queue a resize.

What I'm hoping here is queueing is as cheap as checking whether it's already queued and if so then bail out immediately. But we'll really need to measure it.

> Yes you did :-)

Yay! :-)
Comment 5 Egmont Koblinger 2018-03-04 13:40:30 UTC
Submitted.