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 611709 - The gseal hides GtkStatusBar->messages but doesn't give anything in return
The gseal hides GtkStatusBar->messages but doesn't give anything in return
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
2.20.x
Other All
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 597610 621634
 
 
Reported: 2010-03-03 15:18 UTC by Snark
Modified: 2011-02-04 16:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add gtk_statusbar_remove_all (2.76 KB, patch)
2010-04-30 23:25 UTC, Garrett Regier
needs-work Details | Review
Added 2.22 (2.78 KB, patch)
2010-05-11 08:16 UTC, Garrett Regier
needs-work Details | Review
Updated (2.97 KB, patch)
2010-05-15 21:01 UTC, Garrett Regier
accepted-commit_now Details | Review

Description Snark 2010-03-03 15:18:28 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.
Comment 1 Christian Dywan 2010-03-05 16:41:07 UTC
> 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.
Comment 2 Snark 2010-03-05 18:04:04 UTC
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).
Comment 3 Christian Dywan 2010-03-05 19:40:23 UTC
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);
Comment 4 Snark 2010-03-06 11:18:36 UTC
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 ;-)
Comment 5 Garrett Regier 2010-04-30 23:25:16 UTC
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.
Comment 6 Christian Dywan 2010-05-10 11:09:23 UTC
Review of attachment 160027 [details] [review]:

Please add a Since: 2.22.
Comment 7 Garrett Regier 2010-05-11 08:16:25 UTC
Created attachment 160805 [details] [review]
Added 2.22
Comment 8 Emmanuele Bassi (:ebassi) 2010-05-11 08:25:21 UTC
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.
Comment 9 Garrett Regier 2010-05-15 21:01:19 UTC
Created attachment 161135 [details] [review]
Updated
Comment 10 Christian Dywan 2010-05-27 17:04:12 UTC
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.
Comment 11 Christian Dywan 2010-05-27 17:05:21 UTC
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.
Comment 12 Matthias Clasen 2010-06-01 18:06:11 UTC
Looks ok to me; and I agree that it should go to 2.22 as well.
Comment 13 Christian Dywan 2010-06-02 16:08:39 UTC
Pushed to master and gtk-2-22.