GNOME Bugzilla – Bug 611709
The gseal hides GtkStatusBar->messages but doesn't give anything in return
Last modified: 2011-02-04 16:12:18 UTC
In fact, I saw this trying to make ekiga compile with the seal. The use case is the following : we want to clear all messages with a given context identifier ; so a new api call like : void gtk_status_bar_clear (guint context_id); would be all is needed to get rid of the two ->messages uses in ekiga's code.
> len = g_slist_length ((GSList *) (GTK_STATUSBAR (statusbar)->messages)); > > for (i = 0 ; i < len ; i++) > gtk_statusbar_pop (GTK_STATUSBAR (statusbar), id); For reference, this is the code in Ekiga. And there's a second, similar use elsewhere. How about enhancing gtk_statusbar_remove so that @message_id can be G_MAXINT? Alternatively we could add a gtk_statusbar_remove_all for that. The latter might be cleaner.
Yes, this is that code, and this is why I proposed a gtk_status_bar_clear method taking the context identifier. Notice that the gtk_statusbar_remove_all should accept a context identifier, because the use case you show doesn't want to clear anything (it does loop as much as there are messages in total though).
The code above pops all messages of a given context. So yes, that is what I was thinking of when I suggested gtk_statusbar_remove_all. For instance gtk_statusbar_remove_all (GtkStatusbar* statusbar, guint context);
Ok, so we agree in the api in fact (I forgot the status bar argument, since I'm too OO-minded) ; it's just that I named it _clear and you name it _remove_all ;-)
Created attachment 160027 [details] [review] Add gtk_statusbar_remove_all I used _remove_all because it follows what the standard is for _remove_all vs _clear. For example: void g_queue_clear (GQueue *queue); void g_queue_remove_all (GQueue *queue, gconstpointer data); So because it would need a context_id it makes sense to use _remove_all.
Review of attachment 160027 [details] [review]: Please add a Since: 2.22.
Created attachment 160805 [details] [review] Added 2.22
Review of attachment 160805 [details] [review]: ::: gtk/gtkstatusbar.c @@ +482,3 @@ + * @context_id: a context identifier + * @statusbar: a #GtkStatusBar + * gtk_statusbar_remove_all: since the: if (msg) is the only condition in the body of the function then you should simplify by removing a block and do: if (statusbar->messages == NULL) return; instead; also, all conditions checking for NULL should be explicit to match the coding style. @@ +490,3 @@ + { + gtk_statusbar_pop (statusbar, context_id); + } single statements do not require curly braces. @@ +494,3 @@ + list = statusbar->messages; + while (list) + { while (list != NULL) @@ +501,3 @@ + tmp_list = list; + list = list->next; + statusbar->messages = g_slist_remove_link (statusbar->messages, tmp_list); we should be able to do list surgery without using g_slist_remove_link(), and save us a further iteration on the list. @@ +510,3 @@ + { + list = list->next; + } single statements do not require curly braces.
Created attachment 161135 [details] [review] Updated
Review of attachment 161135 [details] [review]: Looks nice. I guess this should be 3.0 only at this point, since we basically don't want new features in 2.22.
On second thought, we should have it in 2.22 since it replaces a sealed member. So people want to be able to port to it smoothly.
Looks ok to me; and I agree that it should go to 2.22 as well.
Pushed to master and gtk-2-22.