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 666103 - Should be able to match the parent where the a dropdown menu is child
Should be able to match the parent where the a dropdown menu is child
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2011-12-13 17:13 UTC by Andrea Cimitan
Modified: 2012-01-12 19:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
First approach (2.20 KB, patch)
2011-12-15 12:50 UTC, Andrea Cimitan
none Details | Review
Also update on style-updated (1.78 KB, patch)
2011-12-15 13:30 UTC, Andrea Cimitan
none Details | Review
patch for the gtk dir (8.72 KB, patch)
2012-01-06 14:02 UTC, Andrea Cimitan
reviewed Details | Review
patch for the docs dir (4.65 KB, patch)
2012-01-06 14:02 UTC, Andrea Cimitan
none Details | Review
patch for the docs dir (694 bytes, patch)
2012-01-06 14:08 UTC, Andrea Cimitan
none Details | Review
patch for the gtk dir, updated (10.09 KB, patch)
2012-01-11 22:35 UTC, Andrea Cimitan
accepted-commit_now Details | Review
patch for the gtk dir, updated second time (9.83 KB, patch)
2012-01-11 22:54 UTC, Andrea Cimitan
none Details | Review

Description Andrea Cimitan 2011-12-13 17:13:21 UTC
In the css theme you should be able to match various dropdown menus to apply different styles.

For example, you have a dark entry and it's dropdown menu should be dark, but the combobox is light and you want a light dropdown.
Comment 1 Andrea Cimitan 2011-12-15 12:50:30 UTC
Created attachment 203569 [details] [review]
First approach
Comment 2 Andrea Cimitan 2011-12-15 13:30:59 UTC
Created attachment 203574 [details] [review]
Also update on style-updated
Comment 3 Andrea Cimitan 2011-12-15 13:40:12 UTC
I don't think the second patch makes sense for now.
Comment 4 Andrea Cimitan 2012-01-06 14:02:34 UTC
Created attachment 204745 [details] [review]
patch for the gtk dir
Comment 5 Andrea Cimitan 2012-01-06 14:02:57 UTC
Created attachment 204746 [details] [review]
patch for the docs dir
Comment 6 Andrea Cimitan 2012-01-06 14:08:24 UTC
Created attachment 204747 [details] [review]
patch for the docs dir
Comment 7 Matthias Clasen 2012-01-11 12:36:16 UTC
Review of attachment 204745 [details] [review]:

Whats the relationship between this attach-to widget and transient-for ?
That should be spelled out in the docs.
Comment 8 Cosimo Cecchi 2012-01-11 20:30:07 UTC
Review of attachment 204745 [details] [review]:

Looks generally good to me, if our APIs overlords (Benjamin/Matthias) are fine with the new function names; some comments below.

::: gtk-old/gtkwidget.c
@@ +8310,3 @@
+
+  if (attached_windows)
+    g_list_foreach (attached_windows, attached_windows_gfunc, NULL);

Can't you use ((GFunc) reset_style_recurse) here instead of attached_window_gfunc?

@@ +13805,3 @@
+
+              widget->priv->path = gtk_widget_path_copy (gtk_widget_get_path (attach_widget));
+            }

I think this would be more readable:

GtkWidget *attach_widget = NULL;

if (GTK_IS_WINDOW (widget))
  attach_widget = gtk_window_get_attached_to (GTK_WINDOW (widget))

if (attach_widget != NULL)
  widget->priv->path = gtk_widget_path_copy (gtk_widget_get_path (attach_widget));
else
  widget->priv->path = gtk_widget_path_new ();

::: gtk-old/gtkwindow.c
@@ +982,3 @@
+   * GtkWindow:attached-to:
+   *
+   * the widget attached to the window.

Add a link to gtk_window_set_attached_to() here.

@@ +2440,3 @@
+          GList* attached_windows_new = g_list_remove (attached_windows, window);
+          g_object_set_data (G_OBJECT (priv->attach_widget), "gtk-attached-windows", attached_windows_new);
+        }

Is there a reason why you use g_object_get/set_data here?
I think it would be cleaner to have GList *attached_windows as a private member of GtkWidget and having two private methods, _gtk_widget_get/set_attached_windows to access it.

@@ +2652,3 @@
+ * @attach_widget (allow-none): a #GtkWidget, or %NULL
+ *
+ * Attach the window to a widget.

This definitely needs a more detailed description (e.g. in which cases are clients supposed to call this API?).
It should also be made clear that this refers only to the parsing of style information and has nothing to do with gtk_window_set_transient_for().
Comment 9 Benjamin Otte (Company) 2012-01-11 20:51:52 UTC
Thx for the detailed review. Yes, I'm fine with the API.

(In reply to comment #8)
> It should also be made clear that this refers only to the parsing of style
> information and has nothing to do with gtk_window_set_transient_for().
>
I envisioned this not just meant for parsing of style information. An immediate use case is at least the accessibility code that wants to report the attach widget as the parent of the popup. (It currently does that manually in a few cases I think).

In general I envision this API as a way for widgets to say "this window belongs to me", so I expect this to be used by tooltips, entry completions, right-click popups, ctrl-f search entries and whatnot. I'd ideally want to make these windows real child widgets, but I'm afraid that would break too much. In general, every popup window spawned by GTK should have this set.
Comment 10 Andrea Cimitan 2012-01-11 22:35:29 UTC
Created attachment 205049 [details] [review]
patch for the gtk dir, updated
Comment 11 Cosimo Cecchi 2012-01-11 22:41:46 UTC
Review of attachment 205049 [details] [review]:

Looks good to me with the following nitpick.

::: gtk-old/gtkwidget.c
@@ +13725,3 @@
+{
+  return widget->priv->attached_windows;
+}

This is not used, so please remove it.

::: gtk-old/gtkwidgetprivate.h
@@ +105,3 @@
+void         _gtk_widget_remove_attached_window (GtkWidget    *widget,
+                                                 GtkWindow    *window);
+GList       *_gtk_widget_get_attached_windows   (GtkWidget    *widget);

Ditto.
Comment 12 Paolo Borelli 2012-01-11 22:43:55 UTC
Review of attachment 205049 [details] [review]:

A few minor comments...

::: gtk-old/gtkwidget.c
@@ +8303,3 @@
   reset_style_recurse (widget, NULL);
+
+  if (widget->priv->attached_windows)

no need for the if, NULL is a valid empty list

@@ +13710,3 @@
+                                 GtkWindow    *window);
+{
+  widget->priv->attached_windows = g_list_append (widget->priv->attached_windows, window);

does order matter? I do not think so, so better to prepend

@@ +13717,3 @@
+                                    GtkWindow    *window);
+{
+  if (widget->priv->attached_windows)

no need for the if

::: gtk-old/gtkwindow.c
@@ -2408,0 +2431,10 @@
+remove_attach_widget (GtkWindow *window)
+{
+  GtkWindowPrivate *priv = window->priv;
... 7 more ...

g_object_clear?
Comment 13 Andrea Cimitan 2012-01-11 22:54:37 UTC
Created attachment 205051 [details] [review]
patch for the gtk dir, updated second time

Also fixes a couple of typos in the code (; after the functions)
Comment 14 Matthias Clasen 2012-01-11 23:21:01 UTC
Would be nice to unify this new api with the existing gtk_menu_attach_to_widget
Comment 15 Andrea Cimitan 2012-01-11 23:23:18 UTC
One thing at a time :-) I would probably be able to use the api in other widgets, so if someone could create a new bugreport I could look into it...
Comment 16 Cosimo Cecchi 2012-01-12 19:04:21 UTC
Pushed this to master now, thanks Andrea and everyone else involved!