GNOME Bugzilla – Bug 71926
TreeView vs. bg_pixmap
Last modified: 2014-08-03 19:02:28 UTC
Maybe the TextView should be extended to not ignore the bg_pixmap from the style.
s/TextView/TreeView/
Reassigning bugs to new component owner. Sorry for the flood.
*** Bug 144797 has been marked as a duplicate of this bug. ***
See also Bug 340410
Sry meant Bug 340310
*** Bug 354039 has been marked as a duplicate of this bug. ***
*** Bug 340310 has been marked as a duplicate of this bug. ***
*** Bug 468764 has been marked as a duplicate of this bug. ***
I have been able to make a patch that honours the bg_pixmap property in the TreeView. This is done in the 2.18.4 branche of the code. However, I did not check whether all the other issues reported in the bugs 144797, 340310, 354039, 468764 are all solved. Would it still be interesting to post the patch
(In reply to comment #9) > I have been able to make a patch that honours the bg_pixmap property in the > TreeView. > > This is done in the 2.18.4 branche of the code. However, I did not check > whether all the other issues reported in the bugs 144797, 340310, 354039, > 468764 are all solved. I think the most pressing issue is described in 354039 -- there is a way to get tree view to accept the bg_pixmap property, but scrolling is broken in that case. Getting the bg_pixmap to play nice with scrolling is a hard problem, especially because it has to be done in a way that does not severely affect performance. If I recall correctly, this is really why this feature has not been implemented from the start. > Would it still be interesting to post the patch In my opinion it is always interesting to post the patch. Even in case it is not ready yet to go into the master branch of GTK+, people can always use the patch as a starting point to continue working on it.
(In reply to comment #10) > I think the most pressing issue is described in 354039 -- there is a way to get > tree view to accept the bg_pixmap property, but scrolling is broken in that > case. Getting the bg_pixmap to play nice with scrolling is a hard problem, > especially because it has to be done in a way that does not severely affect > performance. If I recall correctly, this is really why this feature has not > been implemented from the start. I have a working version that allows the use of a pixmap background. It has proper scrolling, and supports scrolling in 2 ways - fixed background image and the text scrolles over image - background image scrolls along with text The performance is not affected very much; it is about the same performance as with a solid color background. There is a small issue with resizing columns which I'm now working on. I hope to post a wel tested patch within a couple of days
(In reply to comment #11) > (In reply to comment #10) > > I think the most pressing issue is described in 354039 -- there is a way to get > > tree view to accept the bg_pixmap property, but scrolling is broken in that > > case. Getting the bg_pixmap to play nice with scrolling is a hard problem, > > especially because it has to be done in a way that does not severely affect > > performance. If I recall correctly, this is really why this feature has not > > been implemented from the start. > > I have a working version that allows the use of a pixmap background. > It has proper scrolling, and supports scrolling in 2 ways > - fixed background image and the text scrolles over image > - background image scrolls along with text > > The performance is not affected very much; it is about the same performance as > with a solid color background. > > There is a small issue with resizing columns which I'm now working on. I hope > to post a wel tested patch within a couple of days Feel free to post your current version already. This makes sure you can receive feedback as early as possible.
I have uploaded my patch for the background picture for GtkTreeView. The patch supports: * fixed background with tree/list scrolling over it * moving background moving along with tree/list * parent relative background Apart from setting the background bitmap picture, you can get/set the background scrolling behavior with the functions: gtk_tree_view_get_bg_scrolling/gtk_tree_view_set_bg_scrolling Next to that, I added a patch for the dragwindow of rows; it is now possible to define an anchor to the dragwindow so that the position of the dragwindow relative to the mouse pointer can be controlled: gtk_tree_view_get_enable_dragwindow_anchor/gtk_tree_view_set_enable_dragwindow_anchor and gtk_tree_view_set_dragwindow_anchor/gtk_tree_view_get_dragwindow_anchor The implementation of this feature is indicated by IMPLEMENT_DRAGWINDOW_ANCHOR Furthermore, I found that the implementation of treelines was not quite correct, especially when the treelines were active but the expanders were hidden: in that case the treelines were drawn through the text of the row; also the indentation was, in my view, to perfectly implemented. A added a patch to the code that is marked by IMPLEMENT_EXTENDED_TREELINES I added the diff patch, the modified code files and a file that can be used to test the implementation
Created attachment 151188 [details] [review] diff summaries
Created attachment 151189 [details] modified source code/gtkstyle.c
Created attachment 151190 [details] modified source code/gtktreeprivate.h
Created attachment 151191 [details] modified source code/gtktreeview.h
Created attachment 151192 [details] modified source code/gtktreeview.c
Comment on attachment 151189 [details] modified source code/gtkstyle.c gtkstyle.c
Created attachment 151193 [details] example test program example test program
Hi Hans, thanks a lot for providing your changes. Here are some hints: If you use the git version of GTK+, you can simply run "git diff > bg-pixmap.patch" to create a patch file showing your changes only. These files are easier to handle for review and commit purposes. If you changed GTK+ from a tarball release, feel free to run move your changed folder to gtk-bg-pixmap and then run "diff -burp gtk-x.y.z gtk-bg-pixmap > bg-pixmap.patch". This way we can look at the changes only (and properly review your changes).
Created attachment 151288 [details] [review] patches sorry for my ignorance about creating the patch file; please find now a second attempt
Comment on attachment 151288 [details] [review] patches Here are some comments about the changes. Feel free to press "reply" next to my name to reply to the questions. >diff -burp gtk+-2.18.4.org/gtk/gtkstyle.c gtk+-2.18.4/gtk/gtkstyle.c >--- gtk+-2.18.4.org/gtk/gtkstyle.c 2009-12-01 04:21:24.000000000 +0100 >+++ gtk+-2.18.4/gtk/gtkstyle.c 2010-01-11 22:01:37.000000000 +0100 >@@ -3514,6 +3514,7 @@ gtk_default_draw_box (GtkStyle *sty > if (area) > gdk_gc_set_clip_rectangle (gc, area); > >+ if (!detail || strncmp (detail, "cell", 4) != 0) > gdk_draw_rectangle (window, gc, TRUE, > x, y, width, height); > if (area) Two comments: 1) you could have used g_str_has_prefix (detail, "cell") - which also makes it a bit more clear that you only check a prefix and not the complete string length (if you intended to do that, you need to check on strlen()+1 bytes). 2) This looks like a hack to me. Shouldn't there be a proper change around the original call of gtk_draw_box()/gtk_paint_box()? You could be able to turn off the unrequested rendering by using a style property (as setting a bg-pixmap is a theming operation as well). >diff -burp gtk+-2.18.4.org/gtk/gtktreeprivate.h gtk+-2.18.4/gtk/gtktreeprivate.h >--- gtk+-2.18.4.org/gtk/gtktreeprivate.h 2009-12-01 04:21:24.000000000 +0100 >+++ gtk+-2.18.4/gtk/gtktreeprivate.h 2010-01-11 21:34:15.000000000 +0100 >@@ -105,6 +105,9 @@ struct _GtkTreeViewPrivate > GtkAdjustment *hadjustment; > GtkAdjustment *vadjustment; > >+ GtkTreeViewBgScroll bgscroll_hor; >+ GtkTreeViewBgScroll bgscroll_ver; >+ > GdkWindow *bin_window; > GdkWindow *header_window; > GdkWindow *drag_window; As those enumerations range from 0 (DEFAULT) to 3 (NONE), 2 bits are enough for each. To save some memory, use this: GtkTreeViewBgScroll bgscroll_hor : 2; GtkTreeViewBgScroll bgscroll_ver : 2; >@@ -176,6 +182,11 @@ struct _GtkTreeViewPrivate > GtkTreeViewDropPosition drag_dest_pos; > guint open_dest_timeout; > >+#ifdef IMPLEMENT_DRAGWINDOW_ANCHOR >+ gboolean dragwindow_anchor_set; >+ GtkAnchorType dragwindow_anchor; >+#endif >+ > gint pressed_button; > gint press_start_x; > gint press_start_y; These flags should disappear. We don't expect anyone to change these preprocessor conditionals at all, right? >diff -burp gtk+-2.18.4.org/gtk/gtktreeview.c gtk+-2.18.4/gtk/gtktreeview.c >--- gtk+-2.18.4.org/gtk/gtktreeview.c 2009-12-01 04:21:24.000000000 +0100 >+++ gtk+-2.18.4/gtk/gtktreeview.c 2010-01-11 21:43:14.000000000 +0100 >@@ -580,6 +667,24 @@ gtk_tree_view_class_init (GtkTreeViewCla > GTK_PARAM_READWRITE)); > > g_object_class_install_property (o_class, >+ PROP_BGSCROLL_HOR, >+ g_param_spec_enum ("bgscroll-hor", >+ P_("Background Scroll Horizontal"), >+ P_("Define the horizontol scrolling policy for background placement"), >+ GTK_TYPE_TREE_VIEW_BG_SCROLL, >+ GTK_TREE_VIEW_BG_SCROLL_DEFAULT, >+ GTK_PARAM_READWRITE)); >+ >+ g_object_class_install_property (o_class, >+ PROP_BGSCROLL_VER, >+ g_param_spec_enum ("bgscroll-ver", >+ P_("Background Scroll Vertical"), >+ P_("Define the vertical scrolling policy for background placement"), >+ GTK_TYPE_TREE_VIEW_BG_SCROLL, >+ GTK_TREE_VIEW_BG_SCROLL_DEFAULT, >+ GTK_PARAM_READWRITE)); >+ >+ g_object_class_install_property (o_class, > PROP_HEADERS_VISIBLE, > g_param_spec_boolean ("headers-visible", > P_("Headers Visible"), Shouldn't these two be style properties? Using a backgroudn pixmap requires people to set the background pixmap the the gtkrc file. Shouldn't the scrolling variants be set there as well? Also, this place would be a good chance to provide documentation for the new enumeration. >@@ -764,6 +869,33 @@ gtk_tree_view_class_init (GtkTreeViewCla > -1, > GTK_PARAM_READWRITE)); > >+#ifdef IMPLEMENT_DRAGWINDOW_ANCHOR >+ /** >+ * GtkTreeView:dragwindow-anchor: >+ * >+ * The anchor point of the drag window. >+ * >+ * Since: 2.20 >+ */ >+ g_object_class_install_property (o_class, >+ PROP_ENABLE_DRAGWINDOW_ANCHOR, >+ g_param_spec_boolean ("enable-dragwindow-anchor", >+ P_("Allow dragwindow anchor"), >+ P_("Use a anchor point for the drag window"), >+ FALSE, >+ GTK_PARAM_READWRITE)); >+ >+ g_object_class_install_property (o_class, >+ PROP_DRAGWINDOW_ANCHOR, >+ g_param_spec_enum ("dragwindow-anchor", >+ P_("Dragwindow anchor"), >+ P_("The anchor point of the drag window"), >+ GTK_TYPE_ANCHOR_TYPE, >+ GTK_ANCHOR_CENTER, >+ GTK_PARAM_READWRITE)); >+ >+#endif >+ > /* Style properties */ > #define _TREE_VIEW_EXPANDER_SIZE 12 > #define _TREE_VIEW_VERTICAL_SEPARATOR 2 The doc-comment seems to belong to the second property. The first one is completely undocumented. >@@ -1827,9 +2011,8 @@ gtk_tree_view_realize (GtkWidget *widget > > /* Add them all up. */ > widget->style = gtk_style_attach (widget->style, widget->window); >- gdk_window_set_back_pixmap (widget->window, NULL, FALSE); >- gdk_window_set_background (tree_view->priv->bin_window, &widget->style->base[widget->state]); >- gtk_style_set_background (widget->style, tree_view->priv->header_window, GTK_STATE_NORMAL); >+ SET_BACKGROUND(widget, tree_view, "realize"); >+ gtk_style_set_background (widget->style, tree_view->priv->header_window, GTK_STATE_NORMAL); \ > > tmp_list = tree_view->priv->children; > while (tmp_list) A trailing backslash at the end of the line? >@@ -2422,11 +2637,9 @@ gtk_tree_view_size_allocate (GtkWidget > 0, > MAX (tree_view->priv->width, allocation->width), > tree_view->priv->header_height); >- gdk_window_move_resize (tree_view->priv->bin_window, >- - (gint) tree_view->priv->hadjustment->value, >- TREE_VIEW_HEADER_HEIGHT (tree_view), >+ gdk_window_resize (tree_view->priv->bin_window, > MAX (tree_view->priv->width, allocation->width), >- allocation->height - TREE_VIEW_HEADER_HEIGHT (tree_view)); >+ (allocation->height+pm_height) - TREE_VIEW_HEADER_HEIGHT (tree_view)); > } > > if (tree_view->priv->tree == NULL) Can you explain this please? It looks wrong to me, but I'm convincable. >@@ -2447,7 +2660,9 @@ gtk_tree_view_size_allocate (GtkWidget > /* This little hack only works if we have an LTR locale, and no column has the */ > if (width_changed) > { >- if (gtk_widget_get_direction (GTK_WIDGET (tree_view)) == GTK_TEXT_DIR_LTR && >+ if (early_repaint) >+ ; >+ else if (gtk_widget_get_direction (GTK_WIDGET (tree_view)) == GTK_TEXT_DIR_LTR && > ! has_expand_column) > invalidate_last_column (tree_view); > else Isn't it simpler to turn the if condition into "(!early_repaint || (<old_condition>))"? >@@ -3653,8 +3871,8 @@ gtk_tree_view_horizontal_autoscroll (Gtk > } > offset = offset/3; > >- value = CLAMP (tree_view->priv->hadjustment->value + offset, >- 0.0, tree_view->priv->hadjustment->upper - tree_view->priv->hadjustment->page_size); >+ value = CLAMP (tree_view->priv->hadjustment->value + offset, 0.0, >+ tree_view->priv->hadjustment->upper - tree_view->priv->hadjustment->page_size); > gtk_adjustment_set_value (tree_view->priv->hadjustment, value); > > return TRUE; This change seems to be unnecessary, please xclude it from the patch. >diff -burp gtk+-2.18.4.org/gtk/gtktreeview.h gtk+-2.18.4/gtk/gtktreeview.h >--- gtk+-2.18.4.org/gtk/gtktreeview.h 2009-06-04 21:18:04.000000000 +0200 >+++ gtk+-2.18.4/gtk/gtktreeview.h 2010-01-11 21:31:51.000000000 +0100 >@@ -32,6 +32,16 @@ > > G_BEGIN_DECLS > >+#define IMPLEMENT_DRAGWINDOW_ANCHOR >+#define IMPLEMENT_EXTENDED_TREELINES >+ >+typedef enum >+{ >+ GTK_TREE_VIEW_BG_SCROLL_DEFAULT, >+ GTK_TREE_VIEW_BG_SCROLL_FIXED, >+ GTK_TREE_VIEW_BG_SCROLL_MOVING, >+ GTK_TREE_VIEW_BG_SCROLL_NONE >+} GtkTreeViewBgScroll; > > typedef enum > { IMPLEMENT_DRAWINDOW_ANCHOR/IMPLEMENT_EXTENDED_TREELINES: * these flags should not appear in a publicly installed header file * shouldn't they simply be removed? GtkTreeViewBgScroll: you don't provide any documentation for those, right? What's the difference between "DEFAULT" and "NONE"? ======================================================================== I think this is enough for a first pre-review. One more hint for you: Please try to change the tree test in gtk+-2.18.4/tests to enable using a bg_pixmap, so one can easily verify your changes.
Created attachment 151598 [details] [review] patches updated patch, which takes into account the comment 23, and also provides a patch for the erroneous implementation of the dragwindow for column headers (dragwindow was allocated once and did not take into account changed widths of columns on consequetive drags)
Created attachment 151599 [details] modified source code/gtkstyle.c updated complete source code related to patch 151598
Created attachment 151601 [details] modified source code/gtktreeprivate.h update source code related to patch 151598
Created attachment 151602 [details] modified source code/gtktreeview.h updated complete source code related to path 151598
Created attachment 151603 [details] modified source code/gtktreeview.c updated complete source code related to patch 151598
Created attachment 151604 [details] example test program/testtreeview.c updated example program from the tests directory, which includes the now options implemented by patch 151598
(In reply to comment #23) > (From update of attachment 151288 [details] [review]) > Here are some comments about the changes. Feel free to press "reply" next to my > name to reply to the questions. Hello Sven, thanks for your remarks, I have included a new version that takes you comments into account (as well as a fix for a problem with the drag window of the columns headers). There are two points were I would like to answer your comments/questions (the rest of the text I have deleted for readability): > >diff -burp gtk+-2.18.4.org/gtk/gtkstyle.c gtk+-2.18.4/gtk/gtkstyle.c > >--- gtk+-2.18.4.org/gtk/gtkstyle.c 2009-12-01 04:21:24.000000000 +0100 > >+++ gtk+-2.18.4/gtk/gtkstyle.c 2010-01-11 22:01:37.000000000 +0100 > >@@ -3514,6 +3514,7 @@ gtk_default_draw_box (GtkStyle *sty > > if (area) > > gdk_gc_set_clip_rectangle (gc, area); > > > >+ if (!detail || strncmp (detail, "cell", 4) != 0) > > gdk_draw_rectangle (window, gc, TRUE, > > x, y, width, height); > > if (area) > > Two comments: > 1) you could have used g_str_has_prefix (detail, "cell") - which also makes it > a bit > more clear that you only check a prefix and not the complete string length > (if you intended to do that, you need to check on strlen()+1 bytes). > 2) This looks like a hack to me. Shouldn't there be a proper change around the > original call of gtk_draw_box()/gtk_paint_box()? You could be able to turn > off the > unrequested rendering by using a style property (as setting a bg-pixmap is a > theming > operation as well). 1) yes, I wanted to test on the prefix, so I now use g_str_has_prefix 2) this way of implementing this is no more of a hack than the rest of the gtkstyle.c code; using the "detail" parameter, special handling is forced for several object types. Now, I do the same (in the same manner) for the treeview cells. The only thing is that the treeview cells are defined by the prefix "cell_" and than, depending on the style setting, different suffixes like odd/even, sorted, etc. > >@@ -2422,11 +2637,9 @@ gtk_tree_view_size_allocate (GtkWidget > > 0, > > MAX (tree_view->priv->width, allocation->width), > > tree_view->priv->header_height); > >- gdk_window_move_resize (tree_view->priv->bin_window, > >- - (gint) tree_view->priv->hadjustment->value, > >- TREE_VIEW_HEADER_HEIGHT (tree_view), > >+ gdk_window_resize (tree_view->priv->bin_window, > > MAX (tree_view->priv->width, allocation->width), > >- allocation->height - TREE_VIEW_HEADER_HEIGHT (tree_view)); > >+ (allocation->height+pm_height) - TREE_VIEW_HEADER_HEIGHT (tree_view)); > > } > > > > if (tree_view->priv->tree == NULL) > > Can you explain this please? It looks wrong to me, but I'm convincable. I assume that you refer to the difference gdk_window_move_resize versus gdk_window_resize. This part of the code is dealing with resize the complete gtktreeview widget. When resizing the gtktreeview widget, there is no need to place the bin_window at a different location than before the resizing. Therefore, no 'move' is necessary. Such a move is only necessary when adjusting the sliders. This is proven by the fact that this implementation works correctly. Moreover, one could argue that the move of the header window is a superfluous operation and should be removed. > > ======================================================================== > I think this is enough for a first pre-review. One more hint for you: > > Please try to change the tree test in gtk+-2.18.4/tests to enable using a > bg_pixmap, so one can easily verify your changes. I did this, although I already provided a test program in the first place. But now, the test program is integrated into the gtk development tree. Again, thanks for the comments. I would like to see this code integrated into the main branche, so everything that can help to get there is appreciated.
Created attachment 151610 [details] [review] patches problems with patch file; retry again; sorry for flood
Created attachment 152964 [details] [review] updated patch for image background treeview + extras this patch is an improvement on the previous patch. It fixes a problem in the code, cleans the code but also solves a number of problems in the treeview code. Furthermore, it also contains patches for 132682, 162957, 565559, because these patches are based on the modified scrolling algorithms introduced for the pixmap background scrolling algorithms
Created attachment 152968 [details] background pixmap used by modified testtreeview file to demonstrate pixmap background pixmap used in the testtreeview program as background demonstration pixmap
Created attachment 154973 [details] [review] updated patch for image background treeview + extras I found a problem in the code when changing the height of the header window; when the height was altered the location of the bin window was not updated. The current patch includes a fix for this
Hi Hans, I really appreciate the hard work you have done to implement a solution for this bug (and fixing some other bugs on the way ;) and providing the resulting changes here. We are definitely interested in having this feature merged into the main branch. The patch is pretty large and will take quite some time to review. Unfortunately, I cannot say when exactly I can start review of this patch as I do this in my (limited) spare time as well and have recently been busy with improving the Mac backend of GTK+. Hopefully I can get to this bug in two or three weeks or so (but I cannot promise to hold myself to this). Possibly Sven will have another look as well. My plan of attack for reviewing this patch will be: 1. Split up the larger patch in small, self-contained easily digestible chunks. I will most likely do this in a git branch. Splitting up the patch manually will also give me a better idea of what's all inside. 2. Review and merge chunk-by-chunk.
I have managed to split up the patch from comment 34 and will now attach the resulting patch series patch-by-patch. While splitting up the patches I had to make some changes to get things to compile against git HEAD (most notably changing GTK_WIDGET_STATE to gtk_widget_get_state). These patches series are based on git HEAD from yesterday. When working from tarball releases, I recommend to use the "quilt" tool to work with the patch series. I hope I managed to split up the patch correctly and it would be great if this could be verified. After that, the plan is to review and merge this into the main branch patch-by-patch (which is the main reason why I will attach each patch separately, instead of attaching a tarball containing all patches).
Created attachment 157878 [details] [review] 1/14 Only draw cell background if "cell_" prefixed detail is requested
Created attachment 157879 [details] [review] 2/14 Proposed bug fix for bug 565559
Created attachment 157881 [details] [review] 3/14 Remove headers on model == NULL
Created attachment 157882 [details] [review] 4/14 Proposed bug fix for bug 132682
Created attachment 157883 [details] [review] 5/14 Proposed patch for fixing incorrect drag window sizes in some cases
Created attachment 157884 [details] [review] 6/14 Proposed patch implementing background pixmaps for tree view
Created attachment 157885 [details] [review] 7/14 Add test code for background picture
Created attachment 157886 [details] [review] 8/14 Correct tree line drawing in tree view
Created attachment 157887 [details] [review] 9/14 Proposed patch for implementing drag window anchors
Created attachment 157888 [details] [review] 10/14 Add test code for drag window anchor
Created attachment 157890 [details] [review] 11/14 Add toggle buttons for various tree view features
Created attachment 157891 [details] [review] 12/14 Show notification labels for received events
Created attachment 157892 [details] [review] 13/14 Proposed patch for bug 162957
Created attachment 157893 [details] [review] 14/14 Add test code for discrete scrolling
Some general and initial comments on the full patch: 1) I don't fully understand what the changes to treeview-scrolling.c are for. I have not included these changes in the patch series. 2) The idea of a configurable drag window anchor is interesting. It could provide a solution to bug 440937. I am however not sure about the usefulness of making this configurable. I agree that the current situation is far from ideal. Is there a default setting that could fix this for most cases? I would say that always attaching the north-west corner of the drag window to the mouse cursor should work fine for almost any case. (Additionally, I think that making the drag window translucent would also be nice). 3) The inclusion of code to test the new features and bug fixes is very appreciated! 4) I like the idea of making it easier to turn certain tree view features on and off. Instead of having check buttons for these, it might be more worthwhile to make it possible to pop up a "property window" for the GtkTreeView so that the entire spectrum of properties can be modified. Such a property window is currently only popped up for a GtkTreeViewColumn when you click on a header button. 5) Please try to fully adhere to the coding standards, we tend to be very strict about this in GNOME projects. Opening and closing curly brackets typically go on a new line and no semicolon is put after a closing bracket. When adding brackets to an if-statement clause that did not have brackets before, make sure to properly indent the contents (even though this might "pollute" the diff). In general I try to keep things within 80 columns, but this is not always possible and there are enough examples within GTK+ that do not comply to this rule ;)
> 1) I don't fully understand what the changes to treeview-scrolling.c are for. > I have not included these changes in the patch series. There are four changes in the treeview-scorlling.c file; 2 are #ifdef's and these should have been removed by me, so sorry for that the other two deal with the fact that is a race-condition between the start of the modified treeview and the test to see whether the treeview has scrolled to the correct place. This kind of loop occurs more often in the code of the test program. It turned out in my situation that the race-condition did actually happen and that caused the test program (wrongly) to fail where it merely did not yet had processed all actions. > 2) The idea of a configurable drag window anchor is interesting. It could > provide a solution to bug 440937. I am however not sure about the usefulness > of making this configurable. I agree that the current situation is far from > ideal. Is there a default setting that could fix this for most cases? I would > say that always attaching the north-west corner of the drag window to the mouse > cursor should work fine for almost any case. (Additionally, I think that > making the drag window translucent would also be nice). I just looked at bug 440937 and the fact alone that there is a dicussion on what is nice/needed/... might already be a reason why configuration of this fetaure is desirable. I agree that for LR environments, the NW corner will be fine in most of the cases, but I also can image that for RL environments, the NE corner would be preferred. Personally, I think any corner would be fine as long as it is not the current situation where the drag window often hides the drag destination completely and one cannot see on which row is dropped. > 3) The inclusion of code to test the new features and bug fixes is very > appreciated! Thank you. > 4) I like the idea of making it easier to turn certain tree view features on > and off. Instead of having check buttons for these, it might be more > worthwhile to make it possible to pop up a "property window" for the > GtkTreeView so that the entire spectrum of properties can be modified. Such a > property window is currently only popped up for a GtkTreeViewColumn when you > click on a header button. I could have a go at that if you like me to do so. I cannot say exactly when I have enough time for this, but I suppose that somewhere this month would be oke? > 5) Please try to fully adhere to the coding standards, we tend to be very > strict about this in GNOME projects. Opening and closing curly brackets > typically go on a new line and no semicolon is put after a closing bracket. > When adding brackets to an if-statement clause that did not have brackets > before, make sure to properly indent the contents (even though this might > "pollute" the diff). > In general I try to keep things within 80 columns, but this is not always > possible and there are enough examples within GTK+ that do not comply to this > rule ;) I will try to be more carefull in the future ;-)
(In reply to comment #52) > the other two deal with the fact that is a race-condition between the start of > the modified treeview and the test to see whether the treeview has scrolled to > the correct place. This kind of loop occurs more often in the code of the test > program. It turned out in my situation that the race-condition did actually > happen and that caused the test program (wrongly) to fail where it merely did > not yet had processed all actions. I see. In the patch I have also seen a couple of places where function calls to the scrolling code have been added and removed, so it might be related to that as well. During review we will most likely figure out if these changes are really necessary. Since the scrolling code is very hard to get right and pretty fragile, we have to be careful here :) > I just looked at bug 440937 and the fact alone that there is a dicussion on > what is nice/needed/... might already be a reason why configuration of this > fetaure is desirable. I agree that for LR environments, the NW corner will be > fine in most of the cases, but I also can image that for RL environments, the > NE corner would be preferred. That's very true. GTK+ should be selecting the correct corner based on the RTL/LTR setting. > > 4) I like the idea of making it easier to turn certain tree view features on > > and off. Instead of having check buttons for these, it might be more > > worthwhile to make it possible to pop up a "property window" for the > > GtkTreeView so that the entire spectrum of properties can be modified. Such a > > property window is currently only popped up for a GtkTreeViewColumn when you > > click on a header button. > > I could have a go at that if you like me to do so. I cannot say exactly when I > have enough time for this, but I suppose that somewhere this month would be > oke? I might get to this myself while reviewing. Most likely this patch will only be a couple of lines, the property dialog source code is already there and the dialog just needs to be set up. It probably makes sense to show the property dialog immediately when testtreeview is launched.
Comment on attachment 157878 [details] [review] 1/14 Only draw cell background if "cell_" prefixed detail is requested >Subject: [PATCH] Only draw cell background if "cell_" prefixed detail is requested I made a brief mistake in the commit message: it should of course read ""if no "cell_" prefixed detail is requested"". >diff --git a/gtk/gtkstyle.c b/gtk/gtkstyle.c >index f18d76a..eafcbbb 100644 >--- a/gtk/gtkstyle.c >+++ b/gtk/gtkstyle.c >@@ -3551,8 +3551,11 @@ gtk_default_draw_box (GtkStyle *style, > if (area) > gdk_gc_set_clip_rectangle (gc, area); > >- gdk_draw_rectangle (window, gc, TRUE, >- x, y, width, height); >+ /* Only draw a flat box if no cell_ detail has been set. */ >+ if (!detail || !g_str_has_prefix (detail, "cell_")) >+ gdk_draw_rectangle (window, gc, TRUE, >+ x, y, width, height); My main question here is whether this patch chunk was applied to the correct function and at the correct place. The original patch was not created with diff's -p option so I could not see to which function this was originally applied. Secondly, what exactly is the use case for this patch? Tree view never calls gtk_paint_box() (which invokes gtk_default_draw_box); see also the previous comment.
Comment on attachment 157879 [details] [review] 2/14 Proposed bug fix for bug 565559 I have commented on this issue in bug 565559 already. Suppressing the current machinery is not the way to go.
Comment on attachment 157881 [details] [review] 3/14 Remove headers on model == NULL >Subject: [PATCH] Remove headers on model == NULL Again I got the commit message slightly wrong. Currently, the columns are still visible with a height of 1 or 2 pixels when the model is set to NULL. Your patch fixes that. However, I think the patch is quite complex. Wouldn't this be much easier to do by just handling this in gtk_tree_view_size_allocate_columns () -- that is hide/show the header_window in there based on the state of tree_view->priv->model?
Comment on attachment 157882 [details] [review] 4/14 Proposed bug fix for bug 132682 This already works fine for rubber banding; it is only the drag-and-drop case that breaks. The distinction that is made between reorderable vs. non-reorderable makes sense to me and it looks like this is the only way to get this done correctly. Again, I think that this patch is a little too complex. If I am not mistaken this can be done much easier by just modifying gtk_tree_view_drag_leave() and set_destination_row(). Instead of removing the call to remove_scroll_timeout() it should be made conditional to the state of tree_view->priv->reorderable.
Review of attachment 157883 [details] [review]: This is a very good catch! ::: gtk/gtktreeview.c @@ +9454,3 @@ + { + gdk_window_resize (tree_view->priv->drag_window, + gtk_tree_view_get_real_requested_width_from_column (tree_view, column), Why is a call to gtk_tree_view_get_real_requested_width_from_column() used here? Using column->button->allocation.width would be better and then the code stays consistent with what is happening in the if branch above (which creates the drag window from scratch). @@ +9455,3 @@ + gdk_window_resize (tree_view->priv->drag_window, + gtk_tree_view_get_real_requested_width_from_column (tree_view, column), + column->button->allocation.height); To further improve the patch: use gdk_window_move_resize() instead and use (column->button->allocation.x, 0) as (x, y) coordinates. Then, the call to gdk_window_new() above is matched.
(In reply to comment #54) > Secondly, what exactly is the use case for this patch? Tree view never calls > gtk_paint_box() (which invokes gtk_default_draw_box); see also the previous > comment. I now see that your main patch changes tree view to actually call gtk_paint_box() in some cases. I assume this is for avoiding a cell background being drawn so that the background image is actually visible.
(In reply to comment #42) > Created an attachment (id=157884) [details] [review] > 6/14 Proposed patch implementing background pixmaps for tree view People have started looking into changes to the current (outdated) theming API over the last few months. Unfortunately, I have just discovered that bg_pixmap might likely be something that will disappear in a new theming API. Therefore, I think it is better that I hold off reviewing this patch until the future of the bg_pixmap setting is clear. So, I will first focus on your remaining patches, that is patch 8 to 14.
Is there a way to fix this now with the new theming API?
bg_pixmaps are no more