GNOME Bugzilla – Bug 738315
IRC: network selection appears broken
Last modified: 2017-07-26 14:36:08 UTC
Created attachment 288243 [details] Bug I think this is just a visual glitch, but even after selecting GimpNet the network box is still blank. See attached screenshot.
I can confirm I get this behaviour too. GNOME 3.16, Arch Linux.
This is still an issue in 3.18.1. In fact, that dialog comes from tp-account-widgets (which is used in gnome-online-accounts as a git submodule): <https://git.gnome.org/browse/telepathy-account-widgets/tree/tp-account-widgets/tpaw-account-widget-irc.ui>.
This is still broken in 3.22, for both g-o-a and Empathy. The network name is not shown on the button and requires the user to open the chooser in order to see it.
Created attachment 336533 [details] screencast of the problem
The problem is not in the .ui, rather here: > telepathy-account-widgets/tree/tp-account-widgets/tpaw-irc-network-chooser.c The Button does not contain a label child as shown by Inspector. That's odd, right?
and maybe I'm gdb'ing wrong, but setting breakpoints on gtk_button_set_label only breaks on startup, not when you select a new network, as one would expect and infer from the various pieces of code involved in this.
(I wonder why I was not on the CC list for a bug I filed myself....)
[dboles] the widget is a subclass of GtkButton and seems to chain up, but the label child gets lost [dboles] (or never succeeds in creation) ... [mcatanzaro] dboles: It in fact forgets to chain up in constructed :) [mcatanzaro] Add a chain up in tpaw_irc_network_chooser_constructed, does that fix it? [dboles] mcatanzaro: see, that shows how much i know :D ... dboles was 'sure' i saw a chain-up [mcatanzaro] hergertme: Shouldn't it be easy to detect if gtk_widget_constructed is never called and throw a warning about that? [mcatanzaro] dboles: I'm looking at https://git.gnome.org/browse/telepathy-account-widgets/tree/tp-account-widgets/tpaw-irc-network-chooser.c#n331 [mcatanzaro] No idea why it's NULL-checking when it chains up, I have never done that before.... [dboles] mcatanzaro: my guess is i saw the instance init stuff and just assumed it handled everything [hergertme] programming by superstition [hergertme] i guess you could check in realize [hergertme] or something to that nature [mcatanzaro] dboles: If it works, go ahead and submit a patch on Bugzilla and I'll commit it. I really don't want to build or test empathy or gnome-online-accounts right now. :) [mcatanzaro] [hergertme: Actually... GObject itself should be able to detect that, right...? Why do it in GtkWidget? [mcatanzaro] dispose/finalize/constructed/constructor should all always be called no matter what. [dboles] mcatanzaro: sure, i'll build and submit a patch if it works. got commit access btw so can do that after a review
Created attachment 341094 [details] [review] [PATCH] irc-network-chooser: fix missing button label > ...by adding a missing chain-up to the parent GtkButton's constructed(). \o/
Since this couldn't go into stable due to the string freeze, I guess we should reopen it, to be reapplied in the next cycle.
We normally close bugs once they're fixed in master. Bug tracking becomes too hard otherwise!
Duh, my logic was wrong anyway, since "the next cycle" here is 3.24, not 3.22.x. For 3.22, do you think it would be worth creating a branch in telepathy-account-widgets, or simply copying them into the g-o-a dist, which would fix the button while not changing any strings? Or are we so late in the game that it wouldn't be worthwhile?
(In reply to Daniel Boles from comment #12) > Or are we so late in the game that it wouldn't be worthwhile? ^ This. Empathy has bigger problems right now than this visual glitch. E.g. I needed to make a call the other day, but the app menu didn't exist for whatever reason so it was impossible; I had to close and reopen Empathy about 10 times before it decided to create the app menu....
That's not in any doubt. But I was talking about g-o-a. :P The problem is still exposed there, to underwhelm people configuring IRC for Polari for instance. IIRC there was a redesign in progress for GOA, but it'd still be an issue for 3.22, until March I guess.
*** Bug 764907 has been marked as a duplicate of this bug. ***
Did you know that TpawIrcNetworkChooserDialog also suffers from the same disease? :) That's the dialog you get when you click the TpawIrcNetworkChooser button. (In reply to Daniel Boles from comment #8) > [mcatanzaro] hergertme: Shouldn't it be easy to detect if > gtk_widget_constructed is never called and throw a warning about that? > > [...] > > [mcatanzaro] [hergertme: Actually... GObject itself should be able to detect > that, right...? Why do it in GtkWidget? We already have something like this in GApplication where it checks whether the user chained up in their startup and shutdown implementations. However, GObject instances are far more widespread than GApplication. eg., a process can have hundreds and thousands of GObjects, but usually one GApplication. So, tracking the chain-up might unfavourably affect the size of each GObject instance. Note that GObject doesn't have a private structure, but has a GData (glib/gdataset*). It does have support for bit flags, but it seems restricted to just two and we already use one to track toggle references. See G_DATALIST_FLAGS_MASK. So, I am not sure.
Indeed. I'll look for other missing chain-ups and maybe submit a cleanup patch at some point. Am I right that where I put the chain-up in my patch is technically wrong? https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobject-construction/ says > constructed() override (before chainup) of each class from most derived to base but I chain up before doing the derived, overridden stuff.
(In reply to Daniel Boles from comment #17) > Indeed. I'll look for other missing chain-ups and maybe submit a cleanup > patch at some point. I already fixed TpawIrcNetworkChooserDialog because the dialog was sometimes only showing me the currently selected network. But cleanup patch will be highly appreciated. Thanks! > Am I right that where I put the chain-up in my patch is technically wrong? > > https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobject- > construction/ says > > > constructed() override (before chainup) of each class from most derived > >to base > > but I chain up before doing the derived, overridden stuff. Don't be alarmed. Your code is fine. When you overwrite the function pointer in your _class_init, _GET_CLASS (obj)->constructed (obj) will first call your code. It is up to your code to decide when it should chain up. In that sense, "before chainup" you are going from most derived to base. Once that's done, ie. "after chainup", you are coming back from base to most derived.