GNOME Bugzilla – Bug 640317
gtk_draw_insertion_cursor should be moved to gtk_render
Last modified: 2011-12-01 00:18:15 UTC
gtk_draw_insertion_cursor was added to gtkstyle in 3.0, but probably before the stylecontext stuff landed. It should be moved to gtk_render_insertion_cursor and we are still in time to remove the other function. The change only affects entry, label and textview. When doing the migration the following things should be kept in mind and possibly fixed. 1) gtk_draw_insertion_cursor is using deprecated style stuff 2) it is leaking the CursorInfo struct it sets into the widget (hopefully using context such struct is not needed at all) 3) the api should be improved: e.g. the context already has a current text direction and the is_primary and has_arrow booleans can probably just be css styles pushed on the context before rendering 4) Once moved the comment in textview.c should be updated to reflect the new location of the code
I've dealt with 1) and 2)
Here is a series of 3 preliminary patches that does the following: 1) Fix an error introduced with the code 2) Use GdkRGBA where possible which is more future-proof 3) Move the get_cursor_color code in gtkwidget where it belongs so that it is easier to implement a stylecontext function without accessing the old style api
Created attachment 179581 [details] [review] patch
Created attachment 179582 [details] [review] patch
Created attachment 179583 [details] [review] patch After entering the first comment I actually realized that this code naturally belongs to stylecontext. I left the method private for now, though I think it should be public.
Thanks for handling this Paolo :), the 3 patches look alright to me, I agree that _gtk_style_context_get_cursor_color() should be made public eventually
Created attachment 179589 [details] [review] patch Here is the final patch to move to gtk_render_insertion_cursor. The previous three patches were committed after review by Carlos.
Created attachment 179590 [details] [review] patch Actually do point (4) I pointed out when filing the bug :)
The patch looks great to me, but as talked on irc: 1) I really like your idea of having a GtkCursorType enum for future usecases 2) gtk_draw_insertion_cursor() is actually from 2.4, only that the signature changed in 3.0 to take a cairo_t, so I guess it'd be good to keep that function as a simple wrapper to the new one, as the other functions in gtkstyle.h
Created attachment 179604 [details] [review] patch Updated according to the comments.
Some things that would be nice to address at the same time: - do we want to allow for angled carets ? (see eg http://download.oracle.com/javase/1.3/docs/guide/2d/spec/j2d-fonts.fm4.html) - do we somehow want to avoid the ugly computation of the cursor area in gtktextview.c by getting that information from the same place where the cursor is rendered, somehow ?
Maybe we can use a set of flags to specify draw_arrow/shape/overwrite etc? and we should probably also pass the width so that it can be used for the overwrite mode, documenting that it is not used in other cases. I am travelling this week, so I will not be able to update the patch immediately (I mention it since I think that 3.0 is very near and would be nice to get this api in)
Lets not rush it; moving to 3.2
Hello Paolo, Maybe you have some time to update the patches now?
Created attachment 200754 [details] [review] updated Here is the patch updated to apply against master. It would be great to figure out the couple of API question raised above and then get this committed so that also this bit of the rendering api is consistent with the rest.
Some notes for future reference after a brief conversation with Benjamin on IRC: 1 - Having cursor drawing themeable does not seem particularly useful and I agree. If it does not belong in the engine where should this API be placed so that third party widget can draw a cursor? 2 - Benjamin does not like having just (x,y,h) in the API and I agree, on the other hand the current callers do not have the width information available because pango does not provides it... should we just add the w to the api for future use and document that for now it is ignored and 0 should be passed? 3 - Benjamin mentioned using CSS for this, but I am not sure what he means exactly in this context: maybe that the api should just specify where to draw the cursor and primary/directon/etc should be CSS properties?
After more discussion on IRC with Benjamin, I think we found a good plan: the main idea is to abstract the api more and have something like gtk_render_insertion_cursor(context, cr, pango_layout, pango_index) and push more logic inside the function itself. (probably though we may also need to pass down the widget text direction) If this works, we will be able to remove quite a bit of duplicated code and we do not have to worry about the api taking the width as a param or no. To do that however we need to first start refactoring code to make the three callers (label, widget and textview) more consistent. The following 3 patches are the first step in this refactoring.
Created attachment 202213 [details] [review] patch
Created attachment 202214 [details] [review] patch 2
Created attachment 202215 [details] [review] patch 3
Created attachment 202231 [details] [review] patch 3 Updated the 3rd patch in the series for some minor formatting issues
Created attachment 202232 [details] [review] patch 4
Created attachment 202238 [details] [review] patch This patch on top of the previuos 4 introduces the new gtk_render_insertion_cursor function and uses it for label, entry, and textview. With respect to what described before the new api also needs to take the origin coordinates as input. The patch misses some details like adding to gtk-section and deprecating the old draw function, but I am first interested to know if this looks okay and if it actually works correctly for real bidi users. I just played a but with the arabic text in gtk3-demo and it seems to work
Review of attachment 202231 [details] [review]: ::: gtk/gtktextdisplay.c @@ +904,2 @@ /* We paint the cursors last, because they overlap another chunk + * and need to appear on top. */ Pet peeve: move the */ to the next line to line it up with the other *s
Review of attachment 202238 [details] [review]: ::: gtk/gtklabel.c @@ +3993,3 @@ { + GtkStyleContext *context = gtk_widget_get_style_context (widget); + PangoDirection cursor_direction = get_cursor_direction (label); I generally prefer to separate declarations and assignments nowadays. GtkStyleContext *context; context = gtk_widget_get_style_context (widget); ::: gtk/gtkstylecontext.c @@ +4472,3 @@ + * @layout: the #PangoLayout of the text + * @index: the index in the #PangoLayout + * @direction: the #PangoDirection of the text x and y are missing from the docs.
Review of attachment 202213 [details] [review]: looks good
Review of attachment 202214 [details] [review]: looks good
Review of attachment 202232 [details] [review]: looks good
Generally looks good to me; just fix the docs
I fixed the documents and the coding style issues you pointed out and pushed the patch series. The only step remaining is deprecating gtk_draw_insertion_cursor... I am not sure how do you prefer to do that since now all deprecated stuff is moved to its own file, but in this case we are sharing the helper function: we either need to make that helper function semipublic or duplicate the code in the deprecated file. Marking as fixed anyway