Closed Bug 1288413 Opened 3 years ago Closed 3 years ago

Move GtkTreeView and co. to WidgetCache

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: stransky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Let's move other widgets to WidgetCache and use them in other widget code.
Attached patch patch (obsolete) — Splinter Review
There's a patch for it, Thanks.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd605031300
Attachment #8775110 - Flags: review?(andrew)
Comment on attachment 8775110 [details] [diff] [review]
patch

Review of attachment 8775110 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/WidgetStyleCache.cpp
@@ +286,5 @@
> +static GtkWidget*
> +CreateTreeHeaderSortArrowWidget()
> +{
> +  /* TODO, but it can't be NULL */
> +  return gtk_button_new();

Any reason we can't just parent this into the window container with AddToWindowContainer? It's not appropriately styled regardless, I don't think being a descendant on GtkWindow will make a difference. The use of a floating reference saves some code.

@@ +519,1 @@
>      default:

According to the CSS node documentation for GtkTreeView, header cells should be a descendant of the "header" CSS node;

> GtkTreeView has a main CSS node with name treeview and style class .view. It has a subnode with name header, which is the 
> parent for all the column header widgets' CSS nodes. For rubberband selection, a subnode with name rubberband is used.

Should we be using this for MOZ_GTK_TREE_HEADER_CELL?

::: widget/gtk/gtk3drawing.cpp
@@ +1174,5 @@
>                            rect->width - 2 * xthickness,
>                            rect->height - 2 * ythickness);
> +    ReleaseStyleContext(style_tree);
> +
> +    style = ClaimStyleContext(MOZ_GTK_SCROLLED_WINDOW, direction);

Why do we need to render the frame with MOZ_GTK_SCROLLED_WINDOW here where we didn't before?
Attachment #8775110 - Flags: review?(andrew) → review-
Attached patch patch v.2Splinter Review
Thanks, there's the updated one with button integrated to our widget tree.


> @@ +519,1 @@
> >      default:
> 
> According to the CSS node documentation for GtkTreeView, header cells should
> be a descendant of the "header" CSS node;
> 
> > GtkTreeView has a main CSS node with name treeview and style class .view. It has a subnode with name header, which is the 
> > parent for all the column header widgets' CSS nodes. For rubberband selection, a subnode with name rubberband is used.
> 
> Should we be using this for MOZ_GTK_TREE_HEADER_CELL?

It means we have to construct whole CSS tree for this widget which also includes MOZ_GTK_TREEVIEW_VIEW, MOZ_GTK_TREEVIEW_EXPANDER clases. We also need to change button rendering routine to accept style instead of widget for that and update combobox rendering code.

I'd rather leave that for separated path, it's marked as TODO now.
 
> ::: widget/gtk/gtk3drawing.cpp
> @@ +1174,5 @@
> >                            rect->width - 2 * xthickness,
> >                            rect->height - 2 * ythickness);
> > +    ReleaseStyleContext(style_tree);
> > +
> > +    style = ClaimStyleContext(MOZ_GTK_SCROLLED_WINDOW, direction);
> 
> Why do we need to render the frame with MOZ_GTK_SCROLLED_WINDOW here where
> we didn't before?

We get the style again because our API does not allow to hold more than one style and we need to render background first and then the frame. A workaround would me more complicated (to set state, invalidate...). The style is cached so there's no big deal here.
Attachment #8775110 - Attachment is obsolete: true
Attachment #8775941 - Flags: review?(andrew)
Attachment #8775941 - Flags: review?(andrew) → review+
Keywords: checkin-needed
Blocks: 1291769
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc77bef0fabe
Move GtkTreeView widgets to WidgetCache, r=acomminos
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc77bef0fabe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.