Add State Refs

[PhilippMDoerner]

This PR is a draft to demonstrate how I plan on acting while in the DSL part of the code.

Given that macros and DSLs in general have a tendency to be pretty mean in terms of complexity, I developed a few habits over the years to structure those well enough so I can more easily get back into it after a while.

I plan on introducing some of those in the owlkettle DSL because I believe them to be beneficial to the overall project and for readability.

I wanted to open up this PR to already show you how I tend to act on every proc I touch when it comes to these to give you the chance to veto any of these habits up front.
(Sidenote, the below proc I mostly touched because I originally added a stateRef field to the generated WidgetState, then realized that's unnecessary, but at that point I'd already written myself a doc comment and it felt like a waste to throw that away)

In general:

• Add doc comments to any proc involved in code generation. Even private procs.
  Procs that generate NimNodes and thus Code directly I also annotate with a pattern that demonstrates what they generate (see doc comment in the code)
• A variable should always only hold one type of NimNodeKind.
  I just found it a ton easier to reason about what a given NimNode is if I can know for sure what NimNodeKind it is.
  That's visible here in how I replaced the bits of result with var fieldName until the final code generation step
• Separate sections in a proc into either smaller procs with fitting names, or at the very least split with doc comments. Code-generating related procs tend to become hard to see what they're about really fast, which is why I have this rule so you have something that allows you to see at a glance what this codeblock is about.

[PhilippMDoerner]

The last 2 commits implement an example on how to use the stateRef by implementing a feature we previously didn't have - keyCaptureWidget on search.

It allows "forwarding" keypress-events that happen while another widget is in focus to the SearchEntry Widget.
This allows in the example to have the list of items underneath the searchentry in focus, and while typing with it in focus the searchEntry fills with text.

From what I can tell it works correctly.

Some thoughts where I'd like to have your opinion:

1. As written in discord, StateRef is the only field where assignments can have 2 meanings: Assigning to a stateRef field means "Fill the StateRef instance I passed into the widget with a reference". Assigning to anywhere else means "Use the StateRef instance I passed into the widget for whatever". Unless you know stateRef is the "source" for the Ref and the other fields are the "sinks" for their usage, this is very non-obvious
2. To properly deal with subscribing/unsubscribing as stateRef changes on a widget, I need to do things in both the build and the update hook of stateRef. In the build-hook I need to do the initial subscription with observer. In the update-hook I need to first check if the stateRef changed. If it did, I want to unsubscribe the observer from the old stateRef and then add the observer to the new stateRef. That's a bit wordy and I'm not 100% if that's accurate.

[PhilippMDoerner]

Just had a chat with leorize: He suggests using the as operator for the semantics on assigning a ref. And I very much agree that the semantics look a lot clearer.

This would basically change this:

      ScrolledWindow:
        stateRef = app.keyCaptureRef
        ... other stuff....

to this

      ScrolledWindow as app.keyCaptureRef:
        ... other stuff....

It'd require some finageling with the DSL I think but in my eyes this improves clarity on where a ref comes from quite a bit.

Any thoughts on that or hints where I'd best look at for implementing that?

[PhilippMDoerner]

I did some experimenting about how to implement this.
With the newest commit (which can easily be reverted) we can now have the as Syntax, see search_entry.nim in this PR:

      ScrolledWindow as app.keyCaptureRef:
        ListBox:
          ... other stuff ...

I wrote an in-depth explanation of what I did and the reasoning behind it in the commit message if further info is required.

[can-lehmann]

 ScrolledWindow as app.keyCaptureRef:
   ... other stuff....

This looks pretty nice! How does it interact with adders and the insert statement?

[PhilippMDoerner]

The way it currently is implemented I think it doesn't interact with the insert statement at all.
By which I mean you likely can't use this syntax with insert at the moment, that'd require additional changes to what I did in parseGui.

As for with adders, in which sense do you mean?

Edit:
Actually, you won't need extra stuff with insert. You just would split it different, as in, you'd never use the as X syntax together with insert.

Like so:

method view(app: AppState): Widget =
  let isInActiveSearch = app.text != ""
  
  let window = gui:
    ScrolledWindow as app.keyCaptureRef:
      ListBox:
         ... lots of stuff ...
  result = gui:
    Window():
      defaultSize = (600, 400)
      HeaderBar() {.addTitlebar.}:
        insert(app.toAutoFormMenu(ignoreFields = @["filteredItems"])) {.addRight.}
      
        SearchEntry() {.addTitle, expand: true.}:
          keyCaptureRef = app.keyCaptureRef
          ...lots of fields and events...
          
      insert window

This works as expected.

[can-lehmann]

As for with adders, in which sense do you mean?

My question is if using as breaks the adder syntax. Where is the adder specified when using as? Widget {. ... .} as ... or Widget as .... {. ... .}?

[can-lehmann]

I wanted to open up this PR to already show you how I tend to act on every proc I touch when it comes to these to give you the chance to veto any of these habits up front.

I think that such refactoring should be made in a separate PR as they are not really related to adding Refs (or given the current title of this PR, adding refs should be a separate PR).

[PhilippMDoerner]

My question is if using as breaks the adder syntax. Where is the adder specified when using as? Widget {. ... .} as ... or Widget as .... {. ... .}?

Right now this breaks the DSL, should be fixable by adjusting some of the parsing again.

[PhilippMDoerner]

This is going to interact with adder or rather pragmas in general in a really ugly way that I haven't yet found a simple way to solve.

Parsing of pragmas happens in of nnkPragmaExpr: in line 123.
However, adding the possibility of an infix approach, that means expanding that section because you might be parsing a pragma either as part of a "normal" line such as <Widget> {.addRow.} or as part of an infix line such as <Widget> as <someRef> {.addRow.}.

[PhilippMDoerner]

@can-lehmann Fixed the interaction with adders not working, example for that is added to SearchEntry. I likely shouldn't put that there but inspiration didn't hit me on how/where else to demonstrate it.

Other than that I fixed the rest of your comments.

[can-lehmann]

I likely shouldn't put that there but inspiration didn't hit me on how/where else to demonstrate it.

I recently wrote a guide on the GUI DSL. You need to add a short subsection on state refs there anyways, so maybe add it there. Of course add a simple example first and then say "To get the reference of a widget with adders, the following syntax is used: "

[can-lehmann]

Another issue you need to solve is what happens when a widget subsribes to a state ref but is deleted afterwards. Currently the observer still remains registered causing a use after free as far as I can see.

[PhilippMDoerner]

Another issue you need to solve is what happens when a widget subsribes to a state ref but is deleted afterwards. Currently the observer still remains registered causing a use after free as far as I can see.

The obvious and clean way to solve that efficiently architecturally speaking is having a destroy hook.
Now how that's supposed to work I don't even have a concept for.

[can-lehmann]

This just ignores the adder, doesn't it?

If you want to use the syntax Widget {.adder.} as .... Please report an error if the user tries to use Widget as ... {.adder.}.

[PhilippMDoerner]

It does not in fact. Like, I didn't think deeper about that fact, but the DSL combination that gets created by Widget {.somepragma.} as ... appears to run into the else block in line 105.

        else: 
          error("Tried to use 'as' with invalid syntax", node)

At least when I added

          ListBoxRow() {.addRow.} as app.dummyRef:
            Label(text = "Static last")

To the search_entry example it blew up in my face with exactly that error message.

[can-lehmann]

Suggested change

	let hasStateRef = not node.stateRef.isNil()
	if hasStateRef:
	let refVar = node.stateRef
	let refAssignment = quote do:
	`name`.stateRef = `refVar`
	body.add(refAssignment)
	if not node.stateRef.isNil:
	body.add: quote do:
	`name`.stateRef = `node.stateRef`

complexity left over from when you were using optionals.

[PhilippMDoerner]

      `name`.stateRef = `node.stateRef`

You can't do this replacement, this won't compile due to

/home/isofruit/dev/owlkettle/owlkettle/guidsl.nim(144, 20) Error: type mismatch: got 'array[0..-1, empty]' for '[]' but expected 'seq[Node]'

I assume this is quote do being inconsistent in its behavior if you don't have an explicit var that you want to insert.

Edit: Moving not node.stateRef.isNil directly into the if statement I have now refactored.

[PhilippMDoerner]

Any thoughts on how a destroy-hook in owlkettle would be implemented?

[can-lehmann]

You might need to attach it to the gtk widget directly (via destroy notify I believe), as the lifetime of the WidgetState is not necessarily equal to the lifetime of the gtk widget.

[PhilippMDoerner]

Memo to me for later in another PR:
I'll need to see if I can create some procs that make performing this setup easier when I implement a second feature that makes use of this.

[can-lehmann]

You might need to attach it to the gtk widget directly (via destroy notify I believe), as the lifetime of the WidgetState is not necessarily equal to the lifetime of the gtk widget.

The other option would be making sure that the lifetime of the gtk widget is always at least equal (or longer) than the lifetime of the WidgetState (by using g_object_ref/unref correctly). This might actually be a nice thing to do. Then you could use =destroy.

[PhilippMDoerner]

The other option would be making sure that the lifetime of the gtk widget is always at least equal (or longer) than the lifetime of the WidgetState (by using g_object_ref/unref correctly). This might actually be a nice thing to do. Then you could use =destroy.

I'll claim incompetence on that one, this goes over my head on how I'd even make sure that is correct. It also conceptually makes no sense to me because in my head WidgetState persists across various iterations of Widgets, it just gets updated. So how a Widget would outlive WidgetState is beyond me. That's one of the reason why the usage of StateRef requires you assigning it to WidgetState.

Edit:
Therefore the solution so far that makes the most sense to me is implementing a general onDestroy hook, specifically for renderables (because how would I know when a viewable gets "destroyed" ?).
That means on-build of a renderable attaching a callback to the GtkWidget that calls the "onDestroyHook"-proc, or does nothing if no hook proc was defined.

So sth with this syntax:

renderable SomeWidget of BaseWidget:
  ... some fields ...
  hooks:
    onDestroy:
      ... do fascinating things ...

Which means modifying the renderable macro

[PhilippMDoerner]

Okay, so I found it rather easy to add destroy-hooks to be parseable, just add to the HookEnum and to parseHookKind and you're done.

However, generating the necessary code for that I'm massively struggling with.
I know I want to execute something along the lines of this:

`state`.connect(`hook`, "destroy", eventCallback)

per destroy hook. But I'm hitting walls on how to do so.
I'm getting the feeling doing this would be infinitely easier for you than for me because I'm massively struggling even vaguely getting an idea on where to add things and from where to get things.