add GtkScrollable, GtkLayout, GtkViewport (mostly complete) + GtkPixBuf, GtkPixbufLoader, and more!

[raichu]

Tried to be as much as compatible with the original code (I actually copy-pasted the original code and edited it along the way). Ran gofmt (which is the standard practicise among go devs, I believe), and it got rid of some unnecessary parentheses around single return values. If this one gets merged, I hope to add more stuff ;)

Fixes #11 BTW.

[raichu]

New commit (partially, no stream support yet) fixes #9

[raichu]

I would appreciate any comments on how this series of patches going :) Should I go on? Are there any uncomfortable bits?

Some "classes" are quite incomplete, but I strongly believe there is a point of having them even in their current state: they are necessary for importing a glade file using Builder :)

[jrick]

Hi, sorry for the delays with reviewing this.

I looked over your work and a lot of it was good, but there were several issues remaining. Off the top of my head, I fixed the way you tried to implement GInterfaces (not completely obvious so that's somewhat expected), removed a panic(), killed a function from mattn, switched Rectangle to embed a C.GtkRectangle directly, added additional wrap* functions, and tried to keep line lengths not stupid long. I would recommend taking a close look at all the changes I made so you have an idea of what we're going for. If you have any questions, do ask.

Due to the way we mirror our actual git repo code to github, I can't put the fixes on a branch under conformal, and had to do it under my own username. Take a look at my branch raichu_fixes and see if everything still works as expected.

[raichu]

Thanks a bunch for the fixes! I've just gone over them. A few points

• I was a bit at loss what to do about Scrollable and Layout, and a similar situation exists between FileChooser and FileChooserDialog (which I'm planning to push next). Now I can follow your example! (and oops, it's not a Bin indeed!)
• About the Rectangle, I first used C.GtkRectangle, but the problem is, its fields are not visible outside (x, y, width, height). Should there be Get/Set functions to access these fields instead? There is a reason I implemented those structs: I need access to those fields ;D
• I believe nobody uses a teletype or punch cards in 21st century, and any decent text-editor has word wrapping capability when the window width is smaller than line-length (I actually remember someone from Go team criticized this moot "we gotta keep 80 chars per line" argument as well), and as a matter of fact my display is 1920 pixels wide, but anyway, this is kind of religious to some strongly opinionated people. 80 chars per line, right?
• One problematic bit; in the doc of FileChooserDialogNew, you say "If both are equal, it is the equivalent to passing NULL as the first button text" but I think you mean "If both are 0".
• The reason there was no wrapPixbuf was because Pixbuf was originally in gdk, and wrapPixbuf wasn't visible to gdkpixbuf. I forgot to use wrapPixbuf when I moved Pixbuf under gdkpixbuf, thanks!
• Where should I send my next pull request?

[jrick]

Yeah the way we're implementing GInterfaces is not at all obvious, but using struct embedding this way allows us to call obj.InterfaceMethod() instead of obj.ToInterface().InterfaceMethod(). It's either that, or implementing the exact same functions for each type over and over again, and that's just not fun.

For rectangle, I think there are some fancy cgo poops to access those fields but I could be wrong. I'll take a closer look at that tomorrow.

Re line length, I think it's kind of stupid, but at Conformal we try to keep all our code within a 80 char limit when possible, mostly for readability purposes (obviously the punch card doesn't matter to us). And I do remember reading in some Go literature that if a line feels like it's too long, feel free to newline and indent. Meh.

Doesn't really matter where pull requests go, I'll just rebase (squash) and push whenever we both think the current work is ready.

[raichu]

Honestly, the more I understand about how gotk3 works, the more I like it :)

I temporarily added getters for Rectangle, and embedded into gtk.Allocation to use "inheritance". Finger crossed for cgo magic-hat trick :)

I'll just keep adding here then :) I just merged your fixes.

[jrick]

So as a proof of concept of dealing with Rectangle, I just threw together this evil creation: http://sprunge.us/HDaN

Pretend that Example_t is GdkRectangle. If all we ever wanted to do was access its fields in the gdk package, that's no problem, just do (for example) 'rect.x'. However, because C types are scoped to their packages just like Go types, that won't work outside of the gdk package. So instead, we create a new type with the same layout in memory and use field names that will be exported. After that, it's just writing wrappers to use our special exported type, casting to the native C type, instead of using the native type directly.

[raichu]

Well, this is what I did originally, only in Go. Was the assumption int32 == C.int bad? (it should be ok almost universally)

[jrick]

Well, I don't trust that the Go and C structs would align exactly the same on all platforms and in all situations. For GdkRectangle, a 'gint' is a typedef for 'int' which can vary in size, and won't always align with a Go int32. The way I tried it, cgo is responsible for doing the conversion to Go types. According to the example at http://golang.org/misc/cgo/gmp/gmp.go, it does appear that it will type alias _C_int to int32 (and should be doing the same for _C_gint) but I would rather let cgo take care of all that for us (as it might behave differently on different systems).

[raichu]

Two arguable points:

• Do we need binary compatibility for every such structs
• Is it OK to force the user use type-casting all the time

And the serious problem is cgo cannot handle all kinds of stuff. For example, GdkKeyEvent has a field is_modifier which is defined as a bitfield (1 bit), and cgo cannot access it, hence it's not possible to make such mapping.

I agree that size of the int depends on many things (c flags, compiler, target arch) but under "normal" Linux conditions, it's int32 (arm, 386, amd64). And unless a packing has been specified, I don't see alignment getting in our way. On arm and 386, Go structs and C structs are aligned at 4-bytes (unless specified otherwise). 8 for amd64, and we probably don't need to worry about other archs (gccgo people).

I understand the theoretical problem, but I think it's very unlikely that it will become a problem in practice.

I don't think there is any perfect solution here, so maybe it's time to discuss about trade-offs.

By the way, I noticed that original gotk3 already breaks binary compatibility. All int and enums from C side are defined as int whereas they should rather be int32. They are cast to correct C types within wrappers so it's okay, and I'm basically suggesting we can do the similar thing for structs.

And also, this way, the Window field can become a proper gtk.Window for example.

[jrick]

I would say that we do want binary compatibility. One of the big reasons for beginning gotk3 was due to other bindings not working on all the platforms we needed. That may or may not have been due to making assumptions like this, but we can't assume that everyone is using linux.

What do you mean by forcing the user to type cast? All the heavy lifting would be handled internally in gotk3 code.

I hadn't realized that bit fields weren't supported with cgo (but I suppose that makes sense as Go doesn't have bit fields). I think the best way of doing this is like what I did in my proof of concept above, but provide getters and setters which call out to C code to access those fields.

I do think we may want to start using int32 instead of int for the gotk3 reprensentation of C.int. I'm not particularly fond of the idea (the C spec says that int must be at least 16 bits, with no upper limit specified) but rsc seems to have made the same assumption (given this commit https://code.google.com/p/go/source/detail?r=df5182f90aa0). Again, he recognizes it isn't the greatest assumption to make, but is unaware of any currently supported platforms where the assumption is wrong. I checked the latest Go tip and it still is using int32.

[raichu]

Oh, I see. Well, with the identification of C.int == int32 (and implicity enums are int32 as well) and getter/setters for bitfields, this only leaves the alignment issues. From what I see from the headers, the structs in question (GdkEvent*) are not instructed to be packed in a particular manner, so maybe it's okay to make them pure Go structs?

Otherwise, we need to deal with several issues. Suppose that we implemented these structs in C. How would EventKey look like from Go?

• It should have a *Window field, but this is probably okay, we do have access to Go types from c-side I believe, and it's just a pointer. Edit: This isn't okay. I just realized that the pointer *Window points to a struct that contains the pointer *GdkWindow. So it can't just be cast. This clearly requires a getter/setter.
• The field guint32 time will be converted to guint32 Time (similar for width and height int GdkEventConfigure, to gint). The Go code uses Go types such as int in the wrappers (int for Width and Height). The user need to use casts all the time for every single field like int(ev.Width) --this is what I mean by forcing the user to cast. If we insist on using C structs, then this problem can either be solved 1) by providing getters and setters 2) by using Go integer types at the C side.
• There are gchar * fields (such as string in GdkEventKey or name in GdkEventSetting), which should be mapped to Go strings. Should we provide getters and setters for these as well?

Now, with C structs, the problem is two-fold: 1) it is inconvenient to use: it requires type-casts and getters/setters 2) the interface is not uniform: some fields will require type-casts, some require getters/setters, some none maybe (can be alleviated by using getters/setters for everything, and maybe without exposing any internal fields. Comes at the cost of great inconvenience to the library users).

The advantage (for users) of using Go structs is clear: easy to use, minimal type-casting, no getters/setters. But it maybe require some work the in the library code.

• Packing and int size can be altered using C flags, but by the assumption C.int == int32, we already assume some standards.
• We may need to prepare system/arch-dependent files such as event_structs_windows_amd64.go, event_structs_linux_arm.go etc.

However, more fundamentally, I would like to ask: is the binary-compatibility really really required for everything without exception?

(my proposal below) I think we can have two different structs (EventKey and C.GdkEventKey) and have converter functions (named toNative() for example for conversion to C side) to convert between them vice and versa within the library, without the user ever noticing it. This kind of approach is already used in the main repo: GtkWindowType is actually a 32-bit integer, but on the Go side it is defined as WindowType int, and the binary compatibility is broken on amd64 arch. However, in the wrappers, it is explicity converted between C's GtkWindowType enum and int whenever it passed the boundary between Go and C (which always happen within the wrapper), so it doesn't cause any real-world problems, and there are no inconveniences on the Go side either. What I'm proposing here is basically doing the same thing for Event structs.

Unless we drop the assumption that the boundary between Go and C is only passed by the wrapper and never by the user directly, I see no real-life problems here.

(For further clarity, go-gtk has problems because it assumes binary compatibility and uses C and Go structs interchangeably, causing problems when not handled properly. I suggest keeping C and Go structs separate, without strict binary compatibility, but use conversions vice-versa between two structs behind the scenes and make the whole thing transparent to the user).

(Also, to reduce allocations, we can consider storing a native pointer within the Go struct which can be used when it's non-null).

[jrick]

So after mulling this over for a while, I see your point and think your suggestion is probably the best way to go about this. My only concern is overflowing when converting from a Go type back to a C type, but as long as we stick to Go types with equal sizes, it shouldn't be an issue (switching from int to int32, for example).

[raichu]

Nice! :) I'll go ahread and try to shape it up further then.

[raichu]

Since new stuff is coming in recently, and old pull requests are not merged, should I close this pull request?

[raichu]

Anyway, my fork works for me (I adapt the attitude around gotk3 forks and upstream). Good luck.

[jrick]

No, sorry. It's just that your changes are rather significant and I'd like to avoid anything big until after we release our bitcoin wallet software and I have more time to look over it.

[raichu]

Okay, I'll try to merge the recent changes then.

[raichu]

Wow, that went smooth!