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 592580 - GDK needs sealing
GDK needs sealing
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: X11
2.20.x
Other All
: Normal blocker
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 588339
 
 
Reported: 2009-08-21 13:41 UTC by Kristian Rietveld
Modified: 2011-02-04 16:12 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
Seal GDK, add missing accessors. (40.29 KB, patch)
2009-08-21 13:42 UTC, Kristian Rietveld
needs-work Details | Review
Seal GDK, add missing accessors. #2 (46.40 KB, patch)
2010-01-11 09:16 UTC, Christian Dywan
none Details | Review
Seal GDK, add missing accessors. #3 (45.24 KB, patch)
2010-02-26 17:11 UTC, Christian Dywan
none Details | Review
Seal GDK, Add missing accessors, #4 (49.90 KB, patch)
2010-03-13 14:38 UTC, Carlos Garnacho
none Details | Review
Seal Gdk and add missing accessors v5 (44.58 KB, patch)
2010-04-29 19:42 UTC, Garrett Regier
reviewed Details | Review
Seal GDK and add missing accessors v6 (46.77 KB, patch)
2010-05-16 22:33 UTC, Garrett Regier
needs-work Details | Review
Seal GDK and add missing accessors - gtk-2-22 (51.50 KB, patch)
2010-05-22 05:20 UTC, Garrett Regier
none Details | Review
Seal GDK and add missing accessors - master (47.24 KB, patch)
2010-05-22 05:34 UTC, Garrett Regier
none Details | Review
Seal GDK and add missing accessors - gtk-2-22 v2 (46.48 KB, patch)
2010-05-24 04:39 UTC, Garrett Regier
none Details | Review
Seal GDK and add missing accessors - master v2 (47.25 KB, patch)
2010-05-24 04:43 UTC, Garrett Regier
committed Details | Review

Description Kristian Rietveld 2009-08-21 13:41:18 UTC
GDK needs sealing and missing accessors added, just like GTK+.  Attached patch gets this mostly done.  I used "git diff" instead of format-patch so I ended up with one patch instead of multiple ones.  I got the commits in a local branch here.

Here follow the outstanding issues:

General:
- Should the private pointers be sealed?  In GTK+, some are sealed, the others are not.
- For some files the accessors have not yet been documented.  If functions are documented, they are still lacking the "Since" comments.
- Still need to make up my mind whether to use "get_foo_field()" or "get_is_foo_field()" or "is_foo_field()" for boolean fields.  This patch is probably still inconsistent in this area.

GdkColormap:
- windowing_data seems a private field to me.

GdkDisplay:
- Now that keyboard_grab and pointer_info are sealed, I don't think there is a need to seal the GdkKeyboardGrabInfo and GdkKeyboardPointerInfo structures.  Rather, both need to be moved to a private header.

GdkDragcontext:
- The Mac OS X port needs access to gdk_drag_context_set_is_source.  Is this a good idea?

GdkEvents:
- Do we want to seal all event structures?  Maybe not, since they are also exposed in the X API.  But still, we might gradually move away from that.

GdkFont:
- Not sealed, is marked as deprecated.

GdkGC:
- To me, it does not make sense to seal GdkGCValues.

GdkRgb:
- Unsure what to do with GdkRgbCmap.

GdkScreen:
- Accessors for closed field does not seem to make sense.
- The GC arrays seem to be considered private.  There are indirect accessors
as in _gdk_drawable_get_scratch_gc() which are also private.

GdkInput:
- I am unsure about GdkDeviceKey, GdkDeviceAxis and GdkTimeCoord, but I am
pretty sure these should not be sealed.
- The accessors are replicated for each of the backends.  Putting them in a
global gdkinput.c does not work because of gdk.symbols.

GdkVisual:
- I merged the mask, shift and prec fields into a single function call,
eg. gdk_visual_get_blue_pixel_details().  Anyone has a better name?

GdkWindow:
- I consider the following fields to be private:
   * update_freeze_count
   * resize_count
   * update_and_descendants_freeze_count
   * redirect (since GdkWindowRedirect is private)
   * paint_stack
   * guffaw_gravity
   * extension_events
Comment 1 Kristian Rietveld 2009-08-21 13:42:25 UTC
Created attachment 141329 [details] [review]
Seal GDK, add missing accessors.

Euuh, attachment creation when opening the bug failed.
Comment 2 André Klapper 2009-11-12 16:59:56 UTC
So the patch has been here for 10 weeks.
Any plans to review/get this in soon?
Comment 3 Christian Dywan 2009-11-13 15:45:40 UTC
That patch is missing documentation for most of the newly added functions and also Since tags.
Comment 4 Christian Dywan 2009-11-13 15:47:50 UTC
Maybe Carlos should take a look at the device API, while it's about to be enhanced/ changed.
Comment 5 Kristian Rietveld 2009-11-15 11:09:11 UTC
(In reply to comment #3)
> That patch is missing documentation for most of the newly added functions and
> also Since tags.

That is correct, my goal here was to quickly investigate how long it would take to seal GDK and to get a patch up here ASAP for testing compiling applications against it.
Comment 6 Christian Dywan 2009-12-09 16:53:33 UTC
Any status updates on Gdk sealing?
Comment 7 Kristian Rietveld 2009-12-13 14:55:36 UTC
(In reply to comment #6)
> Any status updates on Gdk sealing?

No and I do not have the time to further pursue it.
Comment 8 Christian Dywan 2010-01-11 09:16:07 UTC
Created attachment 151161 [details] [review]
Seal GDK, add missing accessors. #2

(In reply to comment #0)
> - For some files the accessors have not yet been documented.  If functions are
> documented, they are still lacking the "Since" comments.
I added Since tags, missing documentation and missing "(out):" annotations.

And I fixed GdkWindowObject as well as gtktestutils.c to compile again.

> - Still need to make up my mind whether to use "get_foo_field()" or
> "get_is_foo_field()" or "is_foo_field()" for boolean fields.  This patch is
> probably still inconsistent in this area.
The recent GTK+ accessors use _get_is_foo_field(). So it would seem gdk_window_get_composited should follow that, however the setter without _is_ already exists. The same with gdk_window_get_modal_hint, gdk_window_get_accept_focus and gdk_window_set_focus_on_map. So I guess we omit the _is_ here.

We can indeed go with gdk_window_is_input_only and gdk_window_get_is_shaped, I changed those.

I removed gdk_window_get_destroyed from the patch since we already have gdk_window_is_destroyed which is does the same.

> GdkColormap:
> - windowing_data seems a private field to me.
Seems about right. I would rather add a specific function if needed.

> GdkDisplay:
> - Now that keyboard_grab and pointer_info are sealed, I don't think there is a
> need to seal the GdkKeyboardGrabInfo and GdkKeyboardPointerInfo structures. 
> Rather, both need to be moved to a private header.
Dito, it was never really part of the API. That needs to be done in 2.90 then.

> GdkDragcontext:
> - The Mac OS X port needs access to gdk_drag_context_set_is_source.  Is this a
> good idea?
Maybe we want gdk_drag_context_new_source() and gdk_drag_begin_source() and and get rid of is_source assignments?

> GdkEvents:
> - Do we want to seal all event structures?  Maybe not, since they are also
> exposed in the X API.  But still, we might gradually move away from that.
I would like to see an overhauled event API but I suspect it's not realistic in the given time frame. A re-design like that is not something you want to do overnight.

> GdkFont:
> - Not sealed, is marked as deprecated.
Indeed.

> GdkRgb:
> - Unsure what to do with GdkRgbCmap.
It is only publically used in gdk_draw_indexed_image so *maybe* it's best to make it private. I don't know if there is a use case for it outside GTK+.
Comment 9 André Klapper 2010-02-19 20:34:03 UTC
(In reply to comment #8)
> Created an attachment (id=151161) [details] [review]
> Seal GDK, add missing accessors. #2

Any idea who could review this?
Comment 10 Carlos Garnacho 2010-02-25 11:57:37 UTC
Review of attachment 151161 [details] [review]:

Ok, some opinions on the patch:

* gdk_colormap_get_colors(): I'd merge this function and gdk_colormap_get_size() so, there's an out parameter for the array size. This seems to be the convention for returning arrays.
* gdk_drag_get_source_window: After having a look at the code, this function shouldn't return NULL, that can be removed from the API docs. OTOH, get_dest_window() could, depending on where it's being called, so it's right saying that it might return NULL.
* gdk_drag_context_get_targets(): should be named gdk_drag_context_list_targets(), seems the convention for returning allocated lists.
* gdk_drag_context_get_actions() and others returning GtkDragAction: perhaps GDK_ACTION_DEFAULT should be returned on error
* The docs for gdk_drag_context_get_action() are wrong, they refer to gdk_drag_context_get_suggested_action()
* Could be worth to mention in the docs that gdk_drag_context_get_actions() return a set of flags and the other functions just a single value.
* GdkDevice: This is going to conflict with the xi2 branch, the sealing of GdkDevice already happened there, and other more suitable functions were added as well, if this is too urgent perhaps we could take part of what was done there.
* gdk_visual_get_*_pixel_details(): "prec" should be renamed to precision, both in the function declaration and in the api docs.
* gdk_window_get_background(): perhaps it should be renamed to gdk_window_get_background_color()? Same for gdk_window_get_back_pixmap(), although there's already a setter with the same naming...
* gdk_window_get_composited(): perhaps should better be called gdk_window_get_is_composited()?
* gdk_window_get_modal_hint: perhaps should be better calling it gdk_window_has_modal_hint()?
* gdk_window_get_filters(): I don't think this should be exposed, that list contains structs not declared in public headers, and there is already gdk_window_[add|remove]_filter(), applications should know which filters did they add, and not be able to mess with the ones set by the toolkit
Comment 11 Christian Dywan 2010-02-26 17:11:55 UTC
Created attachment 154772 [details] [review]
Seal GDK, add missing accessors. #3

(In reply to comment #10)
> Review of attachment 151161 [details] [review]:
> * gdk_colormap_get_colors(): I'd merge this function and
> gdk_colormap_get_size() so, there's an out parameter for the array size. This
> seems to be the convention for returning arrays.
Done.

> * gdk_drag_get_source_window: After having a look at the code, this function
> shouldn't return NULL, that can be removed from the API docs. OTOH,
> get_dest_window() could, depending on where it's being called, so it's right
> saying that it might return NULL.
Updated.

> * gdk_drag_context_get_targets(): should be named
> gdk_drag_context_list_targets(), seems the convention for returning allocated
> lists.
Done.

> * gdk_drag_context_get_actions() and others returning GtkDragAction: perhaps
> GDK_ACTION_DEFAULT should be returned on error
Done.

> * The docs for gdk_drag_context_get_action() are wrong, they refer to
> gdk_drag_context_get_suggested_action()
> * Could be worth to mention in the docs that gdk_drag_context_get_actions()
> return a set of flags and the other functions just a single value.
Good point. I updated it so the return value lines point that out.

> * GdkDevice: This is going to conflict with the xi2 branch, the sealing of
> GdkDevice already happened there, and other more suitable functions were added
> as well, if this is too urgent perhaps we could take part of what was done
> there.
The GDK sealing should go in as soon as possible since we are behind schedule. Do you think you could cherry pick commits from the branch that do the sealing?

> * gdk_visual_get_*_pixel_details(): "prec" should be renamed to precision, both
> in the function declaration and in the api docs.
Sounds better indeed. Done.

> * gdk_window_get_background(): perhaps it should be renamed to
> gdk_window_get_background_color()? Same for gdk_window_get_back_pixmap(),
> although there's already a setter with the same naming...
Done. Note sure if we should replace the existing setter functions this late in the cycle...

> * gdk_window_get_composited(): perhaps should better be called
> gdk_window_get_is_composited()?
> * gdk_window_get_modal_hint: perhaps should be better calling it
> gdk_window_has_modal_hint()?
Those have according setters and the _is_naming is usually used for readonly values. So I left them as they are.

> * gdk_window_get_filters(): I don't think this should be exposed, that list
> contains structs not declared in public headers, and there is already
> gdk_window_[add|remove]_filter(), applications should know which filters did
> they add, and not be able to mess with the ones set by the toolkit
Sounds plausible.
Comment 12 Carlos Garnacho 2010-02-26 17:23:20 UTC
(In reply to comment #11)
> > * GdkDevice: This is going to conflict with the xi2 branch, the sealing of
> > GdkDevice already happened there, and other more suitable functions were added
> > as well, if this is too urgent perhaps we could take part of what was done
> > there.
> The GDK sealing should go in as soon as possible since we are behind schedule.
> Do you think you could cherry pick commits from the branch that do the sealing?
> 

Yeah, I'll do that during the weekend.
Comment 13 Carlos Garnacho 2010-03-13 14:38:13 UTC
Created attachment 156059 [details] [review]
Seal GDK, Add missing accessors, #4

A bit late on this... In the end I've mostly adapted the xi2 branch to this patch, I've also added gdk_device_get_key() and gdk_device_get_axis_use() to the patch. I really hope it's still in time for 2.20.
Comment 14 Matthias Clasen 2010-04-19 19:39:37 UTC
Review of attachment 156059 [details] [review]:

I've only looked through part of this

::: gdk/gdkcolor.c
@@ +76,3 @@
+ *
+ * Since: 2.20
+ */

I have a really hard time coming up with a plausible use for this. Do we know of any direct access to colors in the wild ?
O

::: gdk/gdkcursor.c
@@ +108,3 @@
+ *
+ * Since: 2.20
+ */

Same question here. Any known users ?

::: gdk/gdkdnd.c
@@ +87,3 @@
+ *
+ * Since: 2.20
+ **/

Definitively not needed.

@@ +105,3 @@
+ *
+ * Since: 2.20
+ **/

Definitively not needed.

@@ +123,3 @@
+ *
+ * Since: 2.20
+ **/

I have a very hard time imagining use cases for this

@@ +141,3 @@
+ *
+ * Since: 2.20
+ **/

and here too

@@ +159,3 @@
+ *
+ * Since: 2.20
+ **/

Why do we return a copy here ?

@@ +232,3 @@
+ *
+ * Since: 2.20
+ **/

start_time is a private implementation detail of the X implementation.

::: gdk/gdkkeys.c
@@ +322,3 @@
+ *
+ * Since: 2.20
+ **/

This one seems pretty unnecessary, given that you always get the keymap from the display in the first place.
Comment 15 Garrett Regier 2010-04-29 19:42:40 UTC
Created attachment 159941 [details] [review]
Seal Gdk and add missing accessors v5

From what I can tell only the backends use the struct members that you mentioned.
Comment 16 Emmanuele Bassi (:ebassi) 2010-05-11 20:19:22 UTC
Review of attachment 159941 [details] [review]:

the GdkDevice accessors should probably be moved into a generic gdkdevice.c in the top-level, since they are just copied in the three main backends.

::: gdk/gdkimage.c
@@ +150,3 @@
+ * @image: a #GdkImage
+ * gdk_image_get_colormap:
+/**

wrong gtk-doc annotation - should be gdk_image_get_image_type

::: gdk/gdkvisual.c
@@ +157,3 @@
+ * @shift: A pointer to a #gint to be filled in, or %NULL.
+ * @precision: A pointer to a #gint to be filled in, or %NULL.
+ *

should add introspection annotations, while we're at it; maybe g-ir-scanner can figure them out, but it's always good to be explicit

::: gdk/quartz/gdkinput.c
@@ +143,3 @@
+gboolean
+gdk_device_get_has_cursor (GdkDevice *device)
+{

since there's no gdk_device_set_has_cursor() maybe we should just call this gdk_device_has_cursor() - to be consistent with similarly named functions.

@@ +159,3 @@
+                    guint            index,
+gdk_device_get_key (GdkDevice       *device,
+gboolean

don't see the point in returning a boolean, since this function can only fail for a programmer's error.
Comment 17 Kristian Rietveld 2010-05-11 20:26:46 UTC
(In reply to comment #16)
> Review of attachment 159941 [details] [review]:
> 
> the GdkDevice accessors should probably be moved into a generic gdkdevice.c in
> the top-level, since they are just copied in the three main backends.
> 
> ::: gdk/quartz/gdkinput.c
> @@ +143,3 @@
> +gboolean
> +gdk_device_get_has_cursor (GdkDevice *device)
> +{
> 
> since there's no gdk_device_set_has_cursor() maybe we should just call this
> gdk_device_has_cursor() - to be consistent with similarly named functions.
> 
> @@ +159,3 @@
> +                    guint            index,
> +gdk_device_get_key (GdkDevice       *device,
> +gboolean
> 
> don't see the point in returning a boolean, since this function can only fail
> for a programmer's error.

For what it's worth -- these GdkDevice issues might have been solved in the xi2 branch, so it might be worth waiting for that to merge?
Comment 18 Emmanuele Bassi (:ebassi) 2010-05-11 20:44:10 UTC
(In reply to comment #17)

> For what it's worth -- these GdkDevice issues might have been solved in the xi2
> branch, so it might be worth waiting for that to merge?

the sealings would likely go into 2.22, which would not have xi2; though punting GdkDevice changes would make the delta for the xi2 merge smaller.
Comment 19 Garrett Regier 2010-05-16 22:33:58 UTC
Created attachment 161187 [details] [review]
Seal GDK and add missing accessors v6
Comment 20 Javier Jardón (IRC: jjardon) 2010-05-21 23:31:42 UTC
Comment on attachment 161187 [details] [review]
Seal GDK and add missing accessors v6

The patch doesn't apply against current gtk-2-22 branch and doesnt compile against master, could you update your patch?
Comment 21 Garrett Regier 2010-05-22 05:20:44 UTC
Created attachment 161696 [details] [review]
Seal GDK and add missing accessors - gtk-2-22
Comment 22 Garrett Regier 2010-05-22 05:34:41 UTC
Created attachment 161698 [details] [review]
Seal GDK and add missing accessors - master
Comment 23 Matthias Clasen 2010-05-22 14:11:33 UTC
Review of attachment 161698 [details] [review]:

Looks mostly good to me.

::: gdk/gdkwindow.c
@@ +7947,3 @@
+    {
+      if (private->bg_pixmap == GDK_PARENT_RELATIVE_BG
+          || private->bg_pixmap == GDK_NO_BG)

Please put the || at the end of the previous line.

::: gdk/x11/gdkinput.c
@@ +241,3 @@
+
+  return device->has_cursor;
+}

The xi2 branch has gdk_device_get_has_cursor, so please use that name.

@@ +268,3 @@
+                    guint            index,
+                    guint           *keyval,
+                    GdkModifierType *modifiers)

The xi2 version of this function returns a boolean, we should match that:

* Returns: %TRUE if keyval is set for @index.
Comment 24 Garrett Regier 2010-05-22 15:44:50 UTC
(In reply to comment #23)

> 
> ::: gdk/x11/gdkinput.c
> @@ +241,3 @@
> +
> +  return device->has_cursor;
> +}
> 
> The xi2 branch has gdk_device_get_has_cursor, so please use that name.
> 
> @@ +268,3 @@
> +                    guint            index,
> +                    guint           *keyval,
> +                    GdkModifierType *modifiers)
> 
> The xi2 version of this function returns a boolean, we should match that:
> 
> * Returns: %TRUE if keyval is set for @index.

No problem but I just fixed these to be like this after comment #16.

So can I get some amount of confirmation? Because I would like to stop this cycle of fixing...
Comment 25 Matthias Clasen 2010-05-24 03:35:28 UTC
> No problem but I just fixed these to be like this after comment #16.

Oh, I hadn't noticed that. Anyway, I think get_has_cursor is better, since GdkDevice in the xi2 branch has a has-cursor property.
Comment 26 Garrett Regier 2010-05-24 04:39:03 UTC
Created attachment 161825 [details] [review]
Seal GDK and add missing accessors - gtk-2-22 v2

Seems the build is broken currently so this is before GdkRegion was replaced.
Comment 27 Garrett Regier 2010-05-24 04:43:44 UTC
Created attachment 161826 [details] [review]
Seal GDK and add missing accessors - master v2
Comment 28 Matthias Clasen 2010-05-25 01:53:31 UTC
Review of attachment 161826 [details] [review]:

::: gdk/gdkvisual.h
@@ +144,1 @@
 

This must be an oversight, right ? I don't think we want to introduce visual_ref/unref here ?

::: gdk/gdkwindow.h
@@ +929,3 @@
 
+/* Various accessors */
+gboolean   gdk_window_get_is_input_only      (GdkWindow     *window);

This should perhaps be 

GdkWindowClass gdk_window_get_window_class (GdkWindow *window)

@@ +944,3 @@
+#define gdk_window_copy_area(drawable,gc,x,y,source_drawable,source_x,source_y,width,height) \
+   gdk_draw_pixmap(drawable,gc,source_drawable,source_x,source_y,x,y,width,height)
+#endif /* GDK_DISABLE_DEPRECATED */

Why do we add deprecated functions here ?
Comment 29 Matthias Clasen 2010-05-25 02:03:28 UTC
Review of attachment 161826 [details] [review]:

::: gdk/gdkwindow.h
@@ +929,3 @@
 
+/* Various accessors */
+gboolean   gdk_window_get_is_input_only      (GdkWindow     *window);

On second thought, this is fine.
Comment 30 Matthias Clasen 2010-05-25 16:00:59 UTC
Comment on attachment 161826 [details] [review]
Seal GDK and add missing accessors - master v2

I've committed a variant of this, with some last minute name fiddling:

gdk_window_is_input_only
gdk_window_is_shaped
gdk_window_get_background
gdk_window_get_back_pixmap
Comment 31 Javier Jardón (IRC: jjardon) 2010-06-15 19:45:31 UTC
This is fixed now, and accessor are available in 2.22 branch