tractordev / toolkit-go

MIT License
5 stars 4 forks source link

Purego #6

Closed gysddn closed 1 month ago

gysddn commented 1 month ago

This is a somewhat aggressive refactor of the current cgo bridge.

So far, most, almost all, functions calls have been integrated to purego. If the end goal is to get rid of cgo altogether, this is the todo list:

Testing Window API (Manually)

gysddn commented 1 month ago

About:

  • [ ] Remove cgo types

If removing cgo is not a very strict requirement, I'd like to point out that it provides the types which in turn allows type checking on pointers, that wouldn't otherwise be possible (to my knowledge) in a purego only configuration.

I can, of course, convert them into purego only calls but I wanted to ask for an input about this @progrium before doing so.

Purego with cgo:

    GtkWidgetShowAll                          func (widget *C.GtkWidget)

Purego only:

    GtkWidgetShowAll                          func (widget unsafe.Pointer)
gysddn commented 1 month ago

I am working on some small issues with some API calls but this is ready to review for what it is.

progrium commented 1 month ago

About:

  • [ ] Remove cgo types

If removing cgo is not a very strict requirement, I'd like to point out that it provides the types which in turn allows type checking on pointers, that wouldn't otherwise be possible (to my knowledge) in a purego only configuration.

I can, of course, convert them into purego only calls but I wanted to ask for an input about this @progrium before doing so.

Purego with cgo:

    GtkWidgetShowAll                          func (widget *C.GtkWidget)

Purego only:

    GtkWidgetShowAll                          func (widget unsafe.Pointer)

Thanks for asking for clarification here. We do have a hard requirement to remove CGO so I am ok with unsafe.Pointer internally. If its exposed to the user, we'll do something extra, but I'm pretty sure it's not. Is that correct?

gysddn commented 1 month ago

Last commit should be "GdkRectangle" not "GdkGeometry", my bad there 😅 Don't want to force-push while in review

gysddn commented 1 month ago

Thanks for asking for clarification here. We do have a hard requirement to remove CGO so I am ok with unsafe.Pointer internally. If its exposed to the user, we'll do something extra, but I'm pretty sure it's not. Is that correct?

Correct. Only exposed types are structs that hold pointers to those types, which are hidden away, so changing them didn't break anything.

gysddn commented 1 month ago

I have tested the API after the last change and reported the problems I found as issues. Other than that, the PR is ready for review. @progrium

gysddn commented 1 month ago

Also added function comment docs to denote the actual types of unsafe.Pointers, and to document functions in general.

gysddn commented 1 month ago

@progrium I have cleaned up the file and formatted with proper indentation. PR is ready for review.

I have a commit for app indicators and working on the handling of SetSize (#8) which is partially ready. But I didn't want to make this PR more crowded so I'm planning to submit them as separate PRs after this one is merged.