GNOME Bugzilla – Bug 536229
Improve themability of the notebook scrolling gap
Last modified: 2015-02-12 03:43:47 UTC
On Maemo it turned out to be desirable to style the notebook border differently when the notebook has scroll arrows. Therefore a different detail can be used to paint the border when arrows are present. Patch following.
Created attachment 111965 [details] [review] Pass a different detail if arrows are shown
It might be interesting to instead pass a detail string that tells the engine if there is any tab on the far left or right. Currently at least Clearlooks has a hack to figure this out. The reason to do this is so that the corners are only drawn rounded, if there are no tab at the corner.
Created attachment 112053 [details] screenshot to show what the goal is As you can see in the screenshot, the corners marked red are rounded, while the corners that I marked blue are squared. This is currently done in Clearlooks with some hacks, but may could be done via some style properties.
"detail strings" instead of "style properties" of course.
Created attachment 112902 [details] [review] Enhanced notebook detail I am not quite sure if what I am thinking of is the right solution for the corner drawing difficulties. As I see it the engine needs to tell which side will have an arrow, so a more verbose detail would help. You may also need some hint for the non-scrolling case with too many tabs? I implemented the detail differently this time. It now is always "notebook" plus "_arrow" if any arrow is going to be visible, plus "_left" and/ or "_right" according to which of the four possible arrows are going to be present.
Created attachment 116571 [details] [review] Implement notebook_front_rear After letting some time pass I took another fresh look at the screenshot. Then I had an idea about a different approach than before. As before, the detail is always "notebook", and as appropriate more information is appended to that string. Now "_front" is appended if a visible tab is at the beginning of the notebook. "_rear" is appended if a visible tab is at the end of the notebook. If both applies, both strings are appended. From a comment I included in the patch: > /* The following schema describes the expected detail > in different cases, "1" meaning any number of visible tabs: > 1 child front => notebook_tab_front > n children front => notebook_tab_front > n children + arrows => notebook > n children + !arrows => notebook_tab_front_rear > n children rear => notebook_tab_rear > 1 child left + 1 child rear => notebook_tab_front_rear > */
Oops, a little bit too late I noticed that the patch actually appends "_front" and "_rear", not "_tab_front" and "_tab_rear". But for the moment the naming shouldn't be too much of an issue, and I have not made up my mind with regard to that anyway, we can just think of something later on, provided it works as expected.
Created attachment 116582 [details] [review] Implement notebook_front_rear plus boolean smart-gap As pointed out by Benjamin, we need to make this opt-in via a style property. Otherwise existing themes will break. Therefore I added a property "smart-gap" that defaults to %FALSE. If enabled, the notebook applies the described behaviour, otherwise the detail is always "notebook" just like it is now. Again, the name "smart-gap" was a quick choice, I don't mind if somebody has a better suggestion.
The prior art for this in treeviews is "row-ending-details". So maybe we could make it "tab-details". Wrt to the added details itself, I wonder if you need to differentiate between "tab at the end" and "tab at the end, but more tabs hidden". Probably for theme authors to say...
Created attachment 116672 [details] [review] Fixed version of Christian's patch This version of the patch actually works :-)
Created attachment 116675 [details] [review] This version corrects the comment describing the detail strings.
I still think "tab-details" is a better property name than "smart-tab"
I agree that smart-tab is not a good name. I just wanted to fix the patch so it actually works :-) Though maybe not tab-details either. Because the detail string is set on the notebook (box_gap) drawing, and not when drawing the tabs.
Created attachment 116852 [details] [review] Updated "notebook-position-detail" with short docs Including Benjamin's fixes this patch calls the style property "tab-position-details", since this really does give information about the position of visible tabs. And I shortened the doc comment, the two existing "foo-details" are one liners as well, so that should be sufficient.
Comment on attachment 116852 [details] [review] Updated "notebook-position-detail" with short docs Just stylistic comments for the moment: >Index: gtk/gtknotebook.c >=================================================================== >--- gtk/gtknotebook.c (Revision 21146) >+++ gtk/gtknotebook.c (Arbeitskopie) >@@ -838,6 +838,21 @@ gtk_notebook_class_init (GtkNotebookClas > 0, > GTK_PARAM_READABLE)); > >+ /** >+ * GtkNotebook:tab-position-details: >+ * >+ * When %TRUE, the notebook gap is drawn with a >+ * detail indicating the position of visible tabs. >+ * >+ * Since: 2.16 >+ */ >+ gtk_widget_class_install_style_property (widget_class, >+ g_param_spec_boolean ("tab-position-details", >+ P_("Tab Position Details"), >+ P_("When %TRUE, the notebook gap is drawn with a detail indicating the position of visible tabs"), >+ FALSE, >+ GTK_PARAM_READABLE)); >+ > notebook_signals[SWITCH_PAGE] = > g_signal_new (I_("switch-page"), > G_TYPE_FROM_CLASS (gobject_class), >@@ -4555,7 +4570,9 @@ gtk_notebook_paint (GtkWidget *widget > GtkNotebookPrivate *priv; > GtkNotebookPage *page; > GList *children; >- gboolean showarrow; >+ gboolean tab_position_details; >+ gboolean tab_front, tab_rear, tab_overflow; >+ GString *detail; > gint width, height; > gint x, y; > gint border_width = GTK_CONTAINER (widget)->border_width; >@@ -4645,13 +4662,79 @@ gtk_notebook_paint (GtkWidget *widget > break; > } > } >+ >+ /* Determine the expected layout of tabs: >+ tab_front: TRUE if at least one visible tab is at the front >+ tab_rear: TRUE if at least one visible tab is at the rear >+ tab_overflow: TRUE if we have more tabs than we can display >+ */ Please always prefix all lines of a comment with '*' >+ tab_front = FALSE; >+ tab_rear = FALSE; >+ tab_overflow = FALSE; >+ children = gtk_notebook_search_page (notebook, NULL, step, TRUE); >+ while (children) >+ { >+ page = children->data; >+ children = gtk_notebook_search_page (notebook, children, >+ step, TRUE); >+ if (!GTK_WIDGET_VISIBLE (page->child)) >+ continue; >+ if (!GTK_WIDGET_MAPPED (page->tab_label)) >+ { >+ /* If a tab is not visible because of scrolling, all the other >+ * tabs will be expanded. */ The closing comment mark '*/' should generally go onto its own line in gtk sources. >+ tab_overflow = TRUE; >+ tab_front = TRUE; >+ tab_rear = TRUE; >+ } >+ else if (page->expand == TRUE) >+ { >+ tab_front = TRUE; >+ tab_rear = TRUE; >+ } >+ else if (page->pack == GTK_PACK_START) >+ { >+ tab_front = TRUE; >+ } >+ else >+ { >+ tab_rear = TRUE; >+ } The braces around one-liner if-else branches can be removed. >+ } >+ >+ /* The following logic applys if "tab-position-details" is TRUE */ >+ gtk_widget_style_get (widget, "tab-position-details", &tab_position_details, NULL); >+ >+ /* The following schema describes the expected detail: >+ no arrows: >+ n children front => notebook_front >+ n children rear => notebook_rear >+ n children left + m children rear => notebook_front_rear >+ n children, m expanded => notebook_front_rear >+ >+ with arrows (all tabs are expanded in this case): >+ arrows on both sides => notebook >+ arrows at the front => notebook_rear >+ arrows at the rear => notebook_front >+ arrows invisible because of theme => notebook_front_rear >+ */ Brr, please add the missing stars here as well ;) >+ detail = g_string_new ("notebook"); >+ if (tab_position_details && (tab_front || tab_rear || tab_overflow)) >+ { >+ if (tab_front && !(tab_overflow && notebook->scrollable >+ && (notebook->has_before_previous || notebook->has_before_next))) >+ g_string_append (detail, "_front"); >+ if (tab_rear && !(tab_overflow && notebook->scrollable >+ && (notebook->has_after_previous || notebook->has_after_next))) >+ g_string_append (detail, "_rear"); >+ } > gtk_paint_box_gap (widget->style, widget->window, > GTK_STATE_NORMAL, GTK_SHADOW_OUT, >- area, widget, "notebook", >+ area, widget, detail->str, > x, y, width, height, > tab_pos, gap_x, gap_width); >+ g_string_free (detail, TRUE); > >- showarrow = FALSE; > children = gtk_notebook_search_page (notebook, NULL, step, TRUE); > while (children) > { >@@ -4660,13 +4743,11 @@ gtk_notebook_paint (GtkWidget *widget > step, TRUE); > if (!GTK_WIDGET_VISIBLE (page->child)) > continue; >- if (!GTK_WIDGET_MAPPED (page->tab_label)) >- showarrow = TRUE; > else if (page != notebook->cur_page) > gtk_notebook_draw_tab (notebook, page, area); > } > >- if (showarrow && notebook->scrollable) >+ if (tab_overflow && notebook->scrollable) > { > if (notebook->has_before_previous) > gtk_notebook_draw_arrow (notebook, ARROW_LEFT_BEFORE); What'd be really nice would be a screenshot showing the new details in action...
Created attachment 118568 [details] [review] Updated notebook-position with style fixes Thanks for the comments, I addressed the style fixes. Not sure about the screenshot - if the patch works as intended you should actually not see anything else than before. This is about removing crazy hacks from themes, not about something that was impossible before.
Wouldn't it be easier to find out the current tab by looking at the notebooks active page and number of pages?
The amount of pages and active page is irrelevant for this particular bug. What this bug is about is the drawing of the notebooks box_gap. It is possible to hack this by iterating over the notebook tabs in the engine (and clearlooks does this to some extend), but I would much prefere having the information available from GTK+ directly.
Review would be appreciated
this patch is not longer applicable. nowadays, this would be done with style classes.