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 640317 - gtk_draw_insertion_cursor should be moved to gtk_render
gtk_draw_insertion_cursor should be moved to gtk_render
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkStyleContext
unspecified
Other Linux
: High critical
: 3.4
Assigned To: Carlos Garnacho
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2011-01-23 13:35 UTC by Paolo Borelli
Modified: 2011-12-01 00:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (735 bytes, patch)
2011-01-29 12:03 UTC, Paolo Borelli
none Details | Review
patch (5.12 KB, patch)
2011-01-29 12:04 UTC, Paolo Borelli
none Details | Review
patch (8.66 KB, patch)
2011-01-29 12:13 UTC, Paolo Borelli
none Details | Review
patch (24.25 KB, patch)
2011-01-29 14:39 UTC, Paolo Borelli
none Details | Review
patch (24.89 KB, patch)
2011-01-29 14:43 UTC, Paolo Borelli
none Details | Review
patch (25.82 KB, patch)
2011-01-29 17:09 UTC, Paolo Borelli
none Details | Review
updated (28.27 KB, patch)
2011-11-05 14:56 UTC, Paolo Borelli
none Details | Review
patch (7.23 KB, patch)
2011-11-26 23:10 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch 2 (1.31 KB, patch)
2011-11-26 23:10 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch 3 (8.80 KB, patch)
2011-11-26 23:11 UTC, Paolo Borelli
none Details | Review
patch 3 (8.80 KB, patch)
2011-11-27 14:14 UTC, Paolo Borelli
reviewed Details | Review
patch 4 (4.84 KB, patch)
2011-11-27 14:14 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch (19.71 KB, patch)
2011-11-27 15:53 UTC, Paolo Borelli
reviewed Details | Review

Description Paolo Borelli 2011-01-23 13:35:48 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
Comment 1 Matthias Clasen 2011-01-24 12:27:33 UTC
I've dealt with 1) and 2)
Comment 2 Paolo Borelli 2011-01-29 11:50:22 UTC
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
Comment 3 Paolo Borelli 2011-01-29 12:03:45 UTC
Created attachment 179581 [details] [review]
patch
Comment 4 Paolo Borelli 2011-01-29 12:04:40 UTC
Created attachment 179582 [details] [review]
patch
Comment 5 Paolo Borelli 2011-01-29 12:13:05 UTC
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.
Comment 6 Carlos Garnacho 2011-01-29 12:37:31 UTC
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
Comment 7 Paolo Borelli 2011-01-29 14:39:06 UTC
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.
Comment 8 Paolo Borelli 2011-01-29 14:43:04 UTC
Created attachment 179590 [details] [review]
patch

Actually do point (4) I pointed out when filing the bug :)
Comment 9 Carlos Garnacho 2011-01-29 16:39:10 UTC
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
Comment 10 Paolo Borelli 2011-01-29 17:09:26 UTC
Created attachment 179604 [details] [review]
patch

Updated according to the comments.
Comment 11 Matthias Clasen 2011-01-31 13:05:56 UTC
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 ?
Comment 12 Paolo Borelli 2011-01-31 15:34:07 UTC
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)
Comment 13 Matthias Clasen 2011-02-05 04:51:31 UTC
Lets not rush it; moving to 3.2
Comment 14 Javier Jardón (IRC: jjardon) 2011-10-20 14:55:21 UTC
Hello Paolo,

Maybe you have some time to update the patches now?
Comment 15 Paolo Borelli 2011-11-05 14:56:18 UTC
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.
Comment 16 Paolo Borelli 2011-11-26 10:20:41 UTC
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?
Comment 17 Paolo Borelli 2011-11-26 23:09:07 UTC
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.
Comment 18 Paolo Borelli 2011-11-26 23:10:29 UTC
Created attachment 202213 [details] [review]
patch
Comment 19 Paolo Borelli 2011-11-26 23:10:59 UTC
Created attachment 202214 [details] [review]
patch 2
Comment 20 Paolo Borelli 2011-11-26 23:11:24 UTC
Created attachment 202215 [details] [review]
patch 3
Comment 21 Paolo Borelli 2011-11-27 14:14:12 UTC
Created attachment 202231 [details] [review]
patch 3

Updated the 3rd patch in the series for some minor formatting issues
Comment 22 Paolo Borelli 2011-11-27 14:14:40 UTC
Created attachment 202232 [details] [review]
patch 4
Comment 23 Paolo Borelli 2011-11-27 15:53:30 UTC
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
Comment 24 Matthias Clasen 2011-11-30 01:11:16 UTC
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
Comment 25 Matthias Clasen 2011-11-30 01:17:46 UTC
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.
Comment 26 Matthias Clasen 2011-11-30 01:18:16 UTC
Review of attachment 202213 [details] [review]:

looks good
Comment 27 Matthias Clasen 2011-11-30 01:18:33 UTC
Review of attachment 202214 [details] [review]:

looks good
Comment 28 Matthias Clasen 2011-11-30 01:19:04 UTC
Review of attachment 202232 [details] [review]:

looks good
Comment 29 Matthias Clasen 2011-11-30 01:19:45 UTC
Generally looks good to me; just fix the docs
Comment 30 Paolo Borelli 2011-12-01 00:18:15 UTC
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