#75 destroy hooks

[PhilippMDoerner]

This is a branch containing 2 approaches to the entire destructor semantics: 1) (First commit) Use a value object to wrap GdkPixBuf called PixBuf 2) (Second commit) use a type alias to GdkPixBuf called PixBuf directly without any value objects.

To go back to the first attempt, just revert the second commit.

[can-lehmann]

Adding a =sink hook fixes it for me. I have no idea why though. I plan to investigate further when I have some more time.

[PhilippMDoerner]

@can-lehmann We're running into the scenario that to support 1.6 we need destructors of type proc=destroy(pixbuf: var PixbufObj) = while for destructors of nim 2.0 that is deprecated.

To achieve 1.6. compatibility all it takes is change the signatures: proc=destroy(pixbuf: var PixbufObj) = proc=destroy(buffer: var TextBufferObj) =

My question would be whether the plan is for owlkettle 3.0 to support nim 1.6 still or not. We can either: 1) Ditch nim 1.6., have the destructors go with the recommended signature of proc =destroy(pixbuf: PixbufObj) = 2) Ignore deprecation warnings in nim 2.0, have the destructors go with the deprecated signature of proc =destroy(pixbuf: var PixbufObj) = 3) Satisfy both by using a when switch, which is slightly more maintenance and only makes sense if owlkettle 3.0 in total is intended to do so. (I will move forward with 3 for now, but I can easily revert the commit so that doesn't mean we have to stick with it)

The third approach looks like this:

when(NimMajor >= 2):
  proc `=destroy`(pixbuf: PixbufObj) =
    if isNil(pixbuf.gdk):
      return

    g_object_unref(pointer(pixbuf.gdk))
else:
  proc `=destroy`(pixbuf: var PixbufObj) =
    if isNil(pixbuf.gdk):
      return

    g_object_unref(pointer(pixbuf.gdk))
----
when(NimMajor >= 2):
  proc `=destroy`(buffer: TextBufferObj) =
    g_object_unref(pointer(buffer.gtk))
else:
  proc `=destroy`(buffer: var TextBufferObj) =
    g_object_unref(pointer(buffer.gtk))

Which path do we want to go down?

Regarding destructors for #76 I'll look into it.

[can-lehmann]

I think we can move forward with 3. for now. You could use a template to reduce code duplication (put that into common.nim as well).

[PhilippMDoerner]

I think we can move forward with 3. for now. You could use a template to reduce code duplication (put that into common.nim as well).

I'm actually somewhat torn here. Because as far as I can see judge freeing the memory for a Pixbuf is a different task than freeing the memory of a TextBuffer is a different task than freeing the memory of a Cstring. At least for now that's what it looks like to me. Given that they don't seem inherently coupled to me and it just so happens that their destructors look similar, I hesitate to start coupling some of them together (because cstrings will most likely need their own strategy) when they should be different things.

Feel free to overrule me here and I'll still implement it, this is just my current instinct/gut-feeling from my very limited experience with owlkettle and GTK.

[can-lehmann]

I agree, I would do something like:

template destructor(name, typ, body: untyped) =
  when NimMajor >= 2:
    proc `=destroy`(name: typ) =
      body
  else:
    proc `=destroy`(name: var typ) =
      body

destructor(pixbuf, Pixbuf):
  ...

[PhilippMDoerner]

Applied the template to TextBuf, PixBuf and Stylesheet and streamlined the two creation procs for textbuf and pixbuf as suggested.

[can-lehmann]

Are the =copy hooks still required? They are currently applied inconsistently.

[PhilippMDoerner]

Are the =copy hooks still required? They are currently applied inconsistently.

Logically speaking they should be required, because a copy means the ref count for gtk goes up by one and they must be informed of that so that they can handle memory management. I swear I wrote a copy-hook for textbuf at one point, must've gotten deleted over the course of the PR.

[can-lehmann]

Since it is a ref object though, there should not be any copys (only if you do pixbuf1[] = pixbuf2[]). So I think it is up to you if you want to support that, I just think it should be consistent.

[PhilippMDoerner]

Which is a fair point but in that case I'd still want to have the copy-hook, just for "correctness" even if nobody should ever do that. So yeah, give me a sec I'll provide a copy hook

[can-lehmann]

Looks good!