This would be an enhancement...

However I'm not convinced that UI elements that are normally unsensitive, that
are there to provide status information, should become responsive to mouse clicks. 

Perhaps an extension that provides a notification area icon would be more suitable.

Reinout, what about the RSS extension then ?
It provides a clickable icon in the status bar and that's quite intuitive IMHO.

To be consistent, it could be an idea that _all_ icons in this status bar become
clickable.

For example, the lock could bring a dialog with the current certificate or just
tell you : "this site is not secure, do not provide valuable informations on
this page".

Mayve this could be marked for gnome-love task ?

Created attachment 63886 [details] [review]
Adds doubleclick behavior to the popup blocked icon

This was easily solved adding a callback for button-press-event.
I changed only ephy-window.c the function ephy_window_constructor and also added a ephy_popups_manager_frame_clicked_cb function.

However I'm not sure if I took the right approach. In the callback I get the GtkActionGroup ViewPopupWindows and toggle it to true. The other option (I think) is to do something like sync_tab_popups_allowed (), setting the current tab popups_allowed property. I think the ActionGroup is a better solution because that way there's 'sync' between the View menu and the icon (if I would use the property I don't know if the ViewPopupWindows menu would be automagically toggled).

I hope my patch is ok.

Thanks for the patch!

Setting the toggle action to 'active' is the right approach, yes.
But I don't like the way the signal is connected. IMHO this should all stay internal to ephy-statusbar: ephy-statusbar should declare a "popup-frame-clicked" (or so) signal that's emitted when the popup frame is clicked, and the ephy_popups_manager_frame_clicked_cb callback in ephy-window should only connect to that. ephy-statusbar would connect to button-press-event on the priv->popup_manager_evbox and emit the statusbar signal if it's the right kind of click. (You may need to add gtk_widget_add_events ( s->priv->popups_manager_evbox, GDK_BUTTON_PRESS_MASK); in create_statusbar_popups_manager_icon.)

Created attachment 63911 [details] [review]
Implements popups-manager-clicked and changes the approach to detect double clicks

Well, I followed your advice and added the signal, connected to it in ephy-window.c. Just as you said chpe.

I hope the signal is ok, I copy pasted from ephy-find-toolbar.c/h.

The patch itself looks fine now, thanks! :)

However I'm having a few doubts about the functionality itself; it seems to me it'd be a bit surprising to just click the icon and have loads of windows open. Might it be better to show a popup menu with a list of the windows, or just one "Open popup windows"?

That's a good idea, I think the popupmenu with just 'show popup windows (#)' would be nice.

However maybe someone has a better idea. A tooltip could be enough too.

I just thought that a popup might be better because maybe options like firefox ones can be added (show popups, always allow for this site, allow just this time).

Mmmm...

I was thinking about how to implement this and I think that I should add the UI definition inside the create_statusbar_popups_manager_icon() function in ephy-statusbar.c (since I only need to define it once) and leave the uimanager somehow exposed so I can manipulate it from outside (just like now with the new POPUPS_MANAGER_CLICKED signal), I'm not sure if I have to expose the uimanager or I can get it through some function that I haven't seen yet. 
I'm not sure, guidance will be appreciated.

I think that maybe this popups in statusbar idea could or could not be generalized to let other statusbar icons to give popups with possible actions (like the RSS plugin?). Maybe they should implement it themselves? I don't know, just a thought.

If there was any doubt about this feature, I think I have an argument.

I was on a popup-contained website without any toolbar. Then, I clicked on a link a I saw that a popup was blocked. I wanted to see this popup : no way !

So no more hesitation to have ;-)

As discussed in IRC:

< chpe> hmm the lock widget isn't accessible outside statusbar... maybe ephy_statusbar_add_widget should have a string parameter, and the signal would deliver the string + the widget (to position the menu on), i.e. ("lock", <widget>), ("popup-frame", <widget>) etc?

I understand that I should emit a signal throwing the string and the widget, but I don't get what should I do with the string in ephy_statusbar_add_widget(). Wouldn't it be easier to throw this string when I connect:

gtk_widget_add_events (s->priv->popups_manager_evbox, GDK_BUTTON_PRESS_MASK);
g_signal_connect (s->priv->popups_manager_evbox, "button-press-event",
		G_CALLBACK (ephy_popups_manager_evbox_clicked_cb), s);

I think that should be easier. That way the callback connected to the 'clicked' signal would see which widget said 'im clicked' and do the corresponding actions (using a switch()?).

Yes.

Mass changing target 2.16 -> 2.18

Created attachment 79951 [details] [review]
Updated, doesn't work however

:(

 static gboolean
 padlock_button_press_cb (GtkWidget *ebox,
                          GdkEventButton *event,
-			 EphyStatusbar *statusbar)
+			 EphyStatusbar *statusbar,
+			 const char *name)

That won't work, you can only have one datum here.

I guess it would be simplest to just add POPUPS_CLICKED in addition to LOCK_CLICKED and handle it in the same way?

I don't use that function at all, it's actually printed as "warning declared but not used".
Check statusbar_icon_clicked_cb.

ephy_statusbar_get_widget leaks the |children| list, btw.

@@ -696,9 +701,8 @@ ephy_window_fullscreen (EphyWindow *wind
-	action = gtk_action_group_get_action (priv->action_group, "ViewPageSecurityInfo");
-	g_signal_connect_swapped (popup, "lock-clicked",
-				  G_CALLBACK (gtk_action_activate), action);
+	g_signal_connect_swapped (popup, "icon-clicked",
+				  G_CALLBACK (statusbar_icon_clicked_cb), NULL);

This looks wrong, since this concerns the fullscreen popup, not the statusbar.

+	g_signal_connect_swapped (priv->statusbar, "icon-clicked",
+				  G_CALLBACK (statusbar_icon_clicked_cb), NULL);

You get NULL in the callback because you use _swapped and pass NULL userdata. Just use normal g_signal_connect.

It works now! Thanks :).

I'll now make all the icons do something.

Created attachment 80679 [details] [review]
New version, this one works :)

Ok, this version works :).
I think it's fine in concept (the generic icon-clicked signal), now I'm not sure about the implementation. I don't see anything wrong but I'm not that savvy.

The lock icon reaction is still not in the patch, but it's fairly easy to add. Also I have noted that it's sort of slow to show all the hidden windows, I have noted that this is not related to the patch. It might be my pc however.

Created attachment 81016 [details] [review]
Implements the padlock functionality and the popups func.

This makes the change transparent, I mean it implements everything that was there before I modified this. Now it's patch-improving time :).

The changes in create_caret_indicator are not part of this bug; if they contain more than just the variable rename please file them separately, else just drop them (or commit that part if you like).

+	g_print("name:%p ;widget:%p; statusbar:%p\n", name, widget, statusbar);
No g_print's in production code :)

+}
 #if !GTK_CHECK_VERSION (2, 11, 0)
Missing newline.

+	children = gtk_container_get_children
+		(GTK_CONTAINER (statusbar->priv->icon_container));
+	for (l = children; l != NULL; l = l->next)
+	{
+		if (GTK_IS_EVENT_BOX (l->data))
Why this check for event boxes?
+			if (widget_name == name)
+			{
+				g_print("%s\n", widget_name);
+				return l->data;
+			}

This leaks the |children| list.

Also the patch seems to miss the part that this bug is about, catching the click on the popup icon in ephy-window.c :)

(In reply to comment #22)
> The changes in create_caret_indicator are not part of this bug; if they contain
> more than just the variable rename please file them separately, else just drop
> them (or commit that part if you like).
It was only to make the things a little more tidy, but I can commit it separately.

> 
> +       g_print("name:%p ;widget:%p; statusbar:%p\n", name, widget, statusbar);
> No g_print's in production code :)
> 
But they are so lovely!

> +}
>  #if !GTK_CHECK_VERSION (2, 11, 0)
> Missing newline.
> 
> +       children = gtk_container_get_children
> +               (GTK_CONTAINER (statusbar->priv->icon_container));
> +       for (l = children; l != NULL; l = l->next)
> +       {
> +               if (GTK_IS_EVENT_BOX (l->data))
> Why this check for event boxes?
If I remember correctly, because you neeed an event box to catch the clicks, but it's useless to have this here since the named widgets are only the event boxes. It's legacy from the old patch.

> +                       if (widget_name == name)
> +                       {
> +                               g_print("%s\n", widget_name);
> +                               return l->data;
> +                       }
> 
> This leaks the |children| list.
I'll change the logic because I don't like it very much either.

> 
> Also the patch seems to miss the part that this bug is about, catching the
> click on the popup icon in ephy-window.c :)
> 

That's weird, maybe I uploaded an older version. I'll check that.

Created attachment 81699 [details] [review]
Like the previous patch, but this one is complete.

I have some doubts about the get_widget function, I don't know if it's smart to do it like that... :/.

Moving to 2.20 target due to feature and UI freeze for 2.18.

Thinking this through, a simple signal just for popup-clicked instead of the generalised clicked signal would be enought here.

Maybe better than just open the popups on double-click, instead we should open a menu on the icon, with 

Open popup "popup1"
Open popup "popup2"
...
---
Open all popups

?

That would give you better control, and also not open an unexpected number of windows.

I agree with your idea, however I'm not sure about the signal. What are the pros and cons?

pro: less code, con: none? :)

Moving Severity:enhancement bugs off of 2.20 target.

Setting patch status according to comment 26 ff.

Created attachment 101605 [details] [review]
20071225_bgo_300344_popup-clicks.diff: uses its own signal and a popup menu

First draft for this, my doubts so far:
 * how to iterate over the available popup windows
 * if it's ok how I used gtk_get_current_event_time()
 * if it's ok to create the menu by hand
 * if it's possible to selectively hide/show a popup

+static gboolean
+popups_button_press_cb (GtkWidget *widget,
+			   GdkEventButton *event,
+			   EphyStatusbar *statusbar)
+{
+	g_signal_emit (statusbar, signals[POPUPS_CLICKED], 0);
+	
+	return TRUE;
+}

Should copy what the padlock_btton_press_cb here and check for button and state modified to use GDK_2BUTTON_PRESS so you only emit on double click; you can remove the check in ephy-window then.

+		/* placeholder */
+		menu = gtk_menu_new ();
+		menuitem = gtk_menu_item_new_with_label ("Show popup window #1");
+		gtk_menu_shell_prepend (GTK_MENU_SHELL (menu), menuitem);

All ephy menus should be GtkUIManager based.

+		gtk_menu_popup (GTK_MENU (menu), NULL, NULL, NULL, NULL, 0, gtk_get_current_event_time ());

1, not 0 (since it was left-clicked)

@diego, don't forget about this bug? :)

Hiya, I have a problem:

 static void
+popups_button_press_cb (EphyStatusbar *statusbar, 
+                       EphyWindow *window)
+{
+       EphyWindowPrivate *priv = window->priv;
+
+       GtkWidget *menubar;
+

window->priv is *always* 0x0, so I guess something is not really working :). Ideas?

+	g_signal_connect_swapped (priv->statusbar, "popups-clicked",
+				  G_CALLBACK (popups_button_press_cb), priv);

Did you change that to pass window as data, not priv ?

Yeah. window->priv is always 0x0, and if i pass priv, it's 0x0 too.

This
+       g_signal_connect_swapped (priv->statusbar, "popups-clicked",
+                                 G_CALLBACK (popups_button_press_cb), priv);

doesn't fit with this:
+popups_button_press_cb (EphyStatusbar *statusbar, 
+                       EphyWindow *window)

If you connect_swapped, the callback args are reversed.

hehe :).

Something I'm still wondering: how can I know the titles of the windows being blocked? I would like the menu to say "show <title>" and alike.

To answer my question, there's popup_list in ephy-base-embed, although there's no clear way to get it out of there... I think I might have to add a property or maybe a getter? Not sure what would be better, haven't checked.

I think Web doesn't have "popup blocker status/whitelist" anymore. So, obsolete?

Yup. Nowadays popups simply don't work.