#75 destroy hooks #81

PhilippMDoerner · 2023-09-15T15:23:15Z

This is a branch containing 2 approaches to the entire destructor semantics:

(First commit) Use a value object to wrap GdkPixBuf called PixBuf
(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.

This is a version of using destructors where PixBuf is not a value object, but the pointer to GdkPixbuf directly. You can simply revert this commit to return to the prior version where PixBuf is a value object wrapping GdkPixBuf. That version is known to work.

can-lehmann · 2023-09-15T16:28:30Z

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.

This reverts commit ecf8f6d.

This reverts commit fbb70a9.

We wrap the gdk pointer in a ref object instead of an object. PixbufObj becomes an intermediate value-object and Pixbuf an alias to the ref-object version of PixbufObj.

owlkettle/gtk.nim

By adding the path flag to the nim compilation command we can ensure that the owlkettle version used to compile the examples is the **local** version, not one globally installed.

PhilippMDoerner · 2023-09-16T02:37:59Z

@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:

Ditch nim 1.6., have the destructors go with the recommended signature of proc =destroy(pixbuf: PixbufObj) =
Ignore deprecation warnings in nim 2.0, have the destructors go with the deprecated signature of proc =destroy(pixbuf: var PixbufObj) =
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.

This is a measure for better support of both nim 1.6. and nim 2.0 2.0 no longer supports lock-levels where they are deprecated. So the locks:0 pragma is also deprecated and should be removed. Nim 1.6. meanwhile will spit out warnings over undefined lock levels if the examples are compiled without the pragma. Thus customPragmas was created to define a `locker` pragma. That pragma does nothing on nim2.0 and contains the locks:0 pragma on nim 1.6., satisfying both versions and limiting warning noise.

proc `=destroy`(x: T) = Nim 1.6 desires its destructor hooks to be defined as proc `=destroy`(x: var T) = That is deprecated in nim 2.0. To satisfy both, the new destructor hooks were put behind a when statement

owlkettle/customPragmas.nim

can-lehmann · 2023-09-16T12:40:25Z

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 · 2023-09-16T12:56:14Z

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 · 2023-09-16T13:02:50Z

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):
  ...

owlkettle/customPragmas.nim

owlkettle/widgets.nim

PhilippMDoerner · 2023-09-16T13:21:49Z

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

can-lehmann · 2023-09-16T13:22:34Z

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

owlkettle/mainloop.nim

PhilippMDoerner · 2023-09-16T13:28:07Z

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 · 2023-09-16T13:30:14Z

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 · 2023-09-16T13:32:01Z

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

owlkettle.nimble

can-lehmann · 2023-09-16T13:47:00Z

Looks good!

PhilippMDoerner and others added 5 commits September 13, 2023 19:29

debug attempt

2baeccd

Merge branch 'can-lehmann:main' into main

dec9d45

can-lehmann#75 initial value based semantics version for new destructors

6c6b974

Add =sink hook and change GdkPixBuf to GdkPixbuf

ecf8f6d

PhilippMDoerner added 4 commits September 16, 2023 04:08

Revert "Add =sink hook and change GdkPixBuf to GdkPixbuf"

cb5380c

This reverts commit ecf8f6d.

Revert "can-lehmann#75 second attempt using pointer semantics"

b8cbe1a

This reverts commit fbb70a9.

can-lehmann#75 Third approach for destructors

9c1bbaa

We wrap the gdk pointer in a ref object instead of an object. PixbufObj becomes an intermediate value-object and Pixbuf an alias to the ref-object version of PixbufObj.

can-lehmann#75 remove pointless docs.yml change

d532c91

PhilippMDoerner commented Sep 16, 2023

View reviewed changes

owlkettle/gtk.nim Show resolved Hide resolved

can-lehmann#75 Make examples compile against project owlkettle version

7105001

By adding the path flag to the nim compilation command we can ensure that the owlkettle version used to compile the examples is the **local** version, not one globally installed.

PhilippMDoerner added 3 commits September 16, 2023 11:16

Nim 2.0 desires its destructor hooks to be defined as

cb8a324

proc `=destroy`(x: T) = Nim 1.6 desires its destructor hooks to be defined as proc `=destroy`(x: var T) = That is deprecated in nim 2.0. To satisfy both, the new destructor hooks were put behind a when statement

can-lehmann#76 Add destructors for Stylesheet

f83bfa3