jhass / crystal-gobject

gobject-introspection for Crystal
BSD 3-Clause "New" or "Revised" License
127 stars 13 forks source link

Accept String besides Bytes when the parameter is an array of guint8. #66

Closed hugopl closed 4 years ago

hugopl commented 4 years ago

No idea why GTK annotate the data parameter of gtk_css_provider_load_from_data as element-type guint8 but the C type is in fact gchar*, my guess if that bindings may assume that gchar is always a UTF8 string and the XML can be encoded in any other encoding.

Anyway... pass a string here is much more friendly.... and I think this shouldn't break other things.

jhass commented 4 years ago

I'm not sure this is always legit and passing a string is as easy as calling to_slice on it. See the discussion I had at https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/1939

From that we can also learn that this is rather an edge case and these days is avoided in their API designs. Do you have other examples?

hugopl commented 4 years ago

So.. it's just a bad API on C side, since it requires a cast on C to use the API as the annotation says, probably a corner case as you said, I don't have more examples... so I think is better to just have the glue/fix on gtk.cr as:

class CssProvider
  def load_from_data(string : String)
    load_from_data(load_from_data(string.unsafe_byte_slice(0)))
  end
end
jhass commented 4 years ago

Passing string.to_slice should be enough:

https://github.com/jhass/crystal-gobject/blob/bad5ce216906bdee79f585fb9a97352dfc546e8f/samples/gtk_css_styling/css_window.cr#L32-L33

hugopl commented 4 years ago

Passing string.to_slice should be enough:

https://github.com/jhass/crystal-gobject/blob/bad5ce216906bdee79f585fb9a97352dfc546e8f/samples/gtk_css_styling/css_window.cr#L32-L33

Oops, I was using the ugly way .unsafe_byte_slice(0)... but I think the ugly way is safe in this case, since this function doesn't keep the data and the string object lives while the C function is using it.

jhass commented 4 years ago

It's essentially the same thing in this case, https://github.com/crystal-lang/crystal/blob/ea80f4c81bf23eab4118796d98b1f6405e8de377/src/string.cr#L4795-L4797 vs https://github.com/crystal-lang/crystal/blob/ea80f4c81bf23eab4118796d98b1f6405e8de377/src/string.cr#L4814-L4816

Still I would set a good example to people looking at this :)

jhass commented 4 years ago

Oh and also let's move the comment from the sample into this and update the sample to use the convenience overload :)

hugopl commented 4 years ago

changes done, BTW I think the project is starting to get in a state the rspecs would be useful...

With PySide we had a small C++ dummy library to mimic the use cases we wanted to test in the generator, maybe would be the case to do the same here... e.g.

int test_dummy_closure_parameter(GClosure* closure) { /* call the closure */ }

then we would bind this and test the generator like

it "accept proc as GClosure" do
  proc = -> () { 42 }
  TestDummy.closure_parameter(proc).should eq(42)
end
jhass commented 4 years ago

Yeah, I've been dreading this a bit... contributions welcome :D

jhass commented 4 years ago

❤️