can-lehmann / owlkettle

A declarative user interface framework based on GTK 4
https://can-lehmann.github.io/owlkettle/README
MIT License
383 stars 12 forks source link

Add `Scale` widget #83

Closed can-lehmann closed 1 year ago

can-lehmann commented 1 year ago

Gtk's Scale widget is currently missing. See https://docs.gtk.org/gtk4/class.Scale.html

PhilippMDoerner commented 1 year ago

I've been taking a look at this out of curiosity: I don't quite get how the hooks know how to connect signals.

For example here:

renderable Entry of BaseWidget:
  text: string
  placeholder: string ## Shown when the Entry is empty.
  width: int = -1
  maxWidth: int = -1
  xAlign: float = 0.0
  visibility: bool = true
  invisibleChar: Rune = '*'.Rune

  proc changed(text: string) ## Called when the text in the Entry changed
  proc activate() ## Called when the user presses enter/return

  hooks:
    beforeBuild:
      state.internalWidget = gtk_entry_new()
    connectEvents:
      proc changedCallback(widget: GtkWidget, data: ptr EventObj[proc (text: string)]) {.cdecl.} =
        let text = $gtk_editable_get_text(widget)
        EntryState(data[].widget).text = text
        data[].callback(text)
        data[].redraw()

      state.connect(state.changed, "changed", changedCallback)
      state.connect(state.activate, "activate", eventCallback)
    disconnectEvents:
      state.internalWidget.disconnect(state.changed)
      state.internalWidget.disconnect(state.activate)

Owlkettle knows based on the fact that procs were defined on the renderable that there's a changed and an activate "field" on the type which can be connected to the GTK signals so that these procs get called.

But what are "changedCallback" and "eventCallback" and how does one know when to use which?

The only pattern I somewhat recognize across the file is that you're apparently supposed to always write:

state.connect(state.<procName>, "<signal-name>", <procName>EventCallback

with procName in the above examples being changed and activate to the signal-names "changed" and "activate", though the entire eventCallback for activate kinda throws off the pattern. Does what I type as the last parameter just not matter?

can-lehmann commented 1 year ago

The last parameter refers to the proc that is passed directly to GTK for handling the event. Its job is to:

  1. Read back any state changes from GTK
  2. Call the owlkettle event handler (and pass any arguments it might have)
  3. Call redraw to update the UI after the event was handled.

E.g. for the changed event, which is called when the user input some text, we need to read back the new text and update the Entry.text field with it. Then we call owlkettle's Entry.changed handler and finally we call redraw to update the UI. changedCallback is responsible for doing that.

Other events like activate are simpler, because we do not need to read back state & pass arguments to the owlkettle event handler. This is why there is also a general handler called eventCallback.

In case of the Scale widget, you will need to implement a custom valueChangedCallback, that actually reads back the new position of the scale.

PhilippMDoerner commented 1 year ago

How can you know what the signature of the callback for a given gpointer for a given signal is? For example for the ColorButton it is defined:

void
color_set (
  GtkColorButton* self,
  gpointer user_data
)

You wrapped that as:

      proc colorSetCallback(widget: GtkWidget, data: ptr EventObj[proc (color: tuple[r, g, b, a: float])]) {.cdecl.} =
        var gdkColor: GdkRgba
        gtk_color_chooser_get_rgba(widget, gdkColor.addr)
        let color = (gdkColor.r.float, gdkColor.g.float, gdkColor.b.float, gdkColor.a.float)
        ColorButtonState(data[].widget).color = color
        data[].callback(color)
        data[].redraw()

So somehow you: 1) Figured out that gpointer in this context translates to ptr EventObj and 2) Figured out that the EventObj contains a proc with the signature proc (color: tuple[r, g, b, a: float])

How did you figure that out? Mostly relevant as similarly I need to figure out what's hidden behind the gpointer for valueChanged.

can-lehmann commented 1 year ago

The type of data is just a pointer to an EventObj that contains your owlkettle callback. Look at the definition of connect: https://github.com/can-lehmann/owlkettle/blob/main/owlkettle/widgetutils.nim#L56

PhilippMDoerner commented 1 year ago

Check. The way I currently see it I don't think it makes sense to wrap https://docs.gtk.org/gtk4/ctor.Scale.new.html as it requires a GtkAdjustment and I don't quite understand what to do with that one.

Wrapping https://docs.gtk.org/gtk4/ctor.Scale.new_with_range.html and using that for instantiation seems "correcter" to me, any thoughts?

can-lehmann commented 1 year ago

I would personally use gtk_scale_new and pass nil as the Adjustment (see docs), so that it creates its own adjustment internally. The min/max can then be set in the correct hooks via the methods the Range parent class.

PhilippMDoerner commented 1 year ago

Hmm I'll try that out once I've got something functional. I've got a compileable example for scale at least. But it seems immutable and I can't seem to make it mutable. As in, the slider is frozen at the left-hand side of the scale.

I think this has to do with the Accessible Role: https://docs.gtk.org/gtk4/property.Accessible.accessible-role.html

I can't seem to find a way to define it initially, have you had to deal with that before?

PhilippMDoerner commented 1 year ago

Above you can see the first "stab" I've taken at the entire thing. I've so far figured out that accessible relates to general a11y aka accessibility for screen readers etc. not accessing values.

I keep getting stuck at this state of the widget: image

As you can see its greyed out. It does not move, ever. Similarly the changedValue proc never gets triggered. But the build-hook for the marks field does trigger and show that echo $(state.internalWidget.gtk_widget_is_sensitive()).bool is true. I don't know what's going wrong here.

can-lehmann commented 1 year ago

Are you sure that you set the min/max values for the slider / range? I would assume that if the interval has length 0, it might also trigger this state. You might also need to set a step/page size (if there is not special method for that, then using a method on the adjustemnt obtained via get_adjustment).

Edit: Sorry did not see the code in your fork. It looks like you set the min max, but not the step/page size.

can-lehmann commented 1 year ago

I just had a look at your branch and believe that I found the issue. It seems like your gtk bindings in gtk.nim are wrong. C's double type should be cdouble in nim. If you change the types at the appropriate locations, your code seems to work:

image

PhilippMDoerner commented 1 year ago

image

Yep, that was quite a dumb mistake :laughing: Thanks for pointing that out, that helped quite a fair bit and got me the basics, now to figure out all the hooks etc.

PhilippMDoerner commented 1 year ago

Hmmm somewhat stuck on 2 separate problems now: 1) GTK is behaving super weirdly with the window if the scale has marks on it. Compile the example as it is right now and if you slide the slider around up to near the max value, the window will start expanding. No clue where the hack that is coming from or if that's a GTK bug. See the screencast for an example.

Screencast from 2023-09-27 22-12-51.webm

2) I can't seem to update currentValue properly. I want to have a field that is accessible from the outside that always depicts the current value of the scale. But I don't quite get why, it just doesn't update the value. It always remains the value of "currentValue" that the parent-widget passes on. Not sure how to fix that one.

Screencast from 2023-09-27 22-18-02.webm

can-lehmann commented 1 year ago

For 2., that is expected given that you set currentValue = 0.5 in your example. It is always immediately reverted to 0.5 after being updated in the event handler. Since you also don't have a hook for currentValue, the GTK widget is never updated to reflect the owlkettle widget state.

PhilippMDoerner commented 1 year ago

Ahhh right. Okay, got an update and a build-hook now, which means I do get initial values.

I am not entirely sure if the hooks updating all three of GTK (via gtk_range_set_value) and the state (via valueChangedEventCallback where it is set via ScaleState(data[].widget).currentValue = scaleValue) and the widget (via update-hook, widget.valCurrentValue = state.currentValue) is the right way to go, but it works for now.


  hooks currentValue:
    build:
      gtk_range_set_value(state.internalWidget, widget.valCurrentValue)
      echo "Build: ", widget.valCurrentValue, " - ", state.currentValue
    update:
      gtk_range_set_value(state.internalWidget, state.currentValue) # This seemed more correct as state.currentValue keeps being the one thing that 
      widget.valCurrentValue = state.currentValue # Necessary as otherwise widget.valCurrentValue remains at 0.5, meaning out of sync with the value in state
      echo "Update: ", widget.valCurrentValue, " - ", state.currentValue

I think looking back having a "currentValue" field might have been the wrong approach for what I wanted. What I want is for the user to be able to access the currentValue of a Widget whenever they want (e.g. the Scale might be used inside a Form and when somebody clicks the Okay button you want to extract the values of your individual input widgets). Having a currentValue field I thought might've been the correct move, but I think in fact what I want is an initialValue field and after that a proc getValue(): float64 that you can call on the Scale-Widget to call gtk_range_get_value.

Is that doable? Or is that even the right approach?

PhilippMDoerner commented 1 year ago

I might be facing an actual problem with the Scale being actually "2 way" in its bindings. By that I mean, I want there to be a field on the Scale widget called value (previously called "currentValue").

1) If I update value from the application, the value as it is depicted in the rendered widget should update to reflect that. 2) If the user uses the widget to update the value, then the value in the widget-state should be updated.

However, 1) requires me to have an update field-hook on the value field which updates the widget-state with the value within the value field, like so:

  hooks value:
    update:
      echo "Updated value to ", widget.valValue
      gtk_range_set_value(state.internalWidget, widget.valValue)

If I do that though, then changes to the widget will be impossible because they also trigger the update field-hook on value during the redraw and that will override the value-change from the user using the widget.

I can't seem to figure that one out. You can provide an initial value, but programmatically after that basically only the user can update the value =/

PhilippMDoerner commented 1 year ago

Welp, I managed to fix the "window expands infinitely" issue at least: Whenver the update-hook of "Marks" was called, it added new marks, but never cleared them. So basically after the update hook ran 4 times, I told GTK to add 16 marks in total. By changing the hook to include state.internalWidget.gtk_scale_clear_marks() like so:

  hooks marks:
    (build, update):
      state.internalWidget.gtk_scale_clear_marks()
      let hasScaleMarks = widget.hasMarks and widget.valMarks.len > 0
      if hasScaleMarks:
        for mark in widget.valMarks:
          let label: string = if mark.label.isSome(): mark.label.get() else: $mark.value
          gtk_scale_add_mark(state.internalWidget, mark.value , mark.position.toGtk(), label.cstring)

It fixes the issue

can-lehmann commented 1 year ago

I might be facing an actual problem with the Scale being actually "2 way" in its bindings.

Have a look at how it works for Entry. The text field just has a property hook that updates the GTK.Entry's text. In your example code, the valueChanged callback needs to update the value in app, so that a two way binding actually exists.

On a more general note: You should have property hooks for all fields of the widget, not handle them in beforeBuild. The hook for marks can also be a property hook.

can-lehmann commented 1 year ago

You should be able to use this as a guide: https://github.com/can-lehmann/owlkettle/blob/main/owlkettle/widgets.nim#L949

PhilippMDoerner commented 1 year ago

Applied moving to the property hook for most things, it works fine for almost all the values, not quite for value though.

You should be able to use this as a guide: https://github.com/can-lehmann/owlkettle/blob/main/owlkettle/widgets.nim#L949

Just tried this:

    connectEvents:
      proc valueChangedEventCallback(
        widget: GtkWidget, 
        data: ptr EventObj[proc(newValue: float)]
      ) {.cdecl.} =
        let scaleValue: float64 = gtk_range_get_value(widget).float64
        ScaleState(data[].widget).value = scaleValue # This should be updating ScaleState
        data[].callback(scaleValue)
        data[].redraw()

      state.connect(state.valueChanged, "value-changed", valueChangedEventCallback)

    disconnectEvents:
      state.internalWidget.disconnect(state.valueChanged)

  hooks value:
    property:
      state.internalWidget.gtk_range_set_value(state.value.cdouble)
    read:
      state.value = state.internalWidget.gtk_range_get_value().float64

I can't quite see where the Scale differs from Text, but it doesn't quite seem work. See example video at the bottom. As you can see in valueChangedEventCallback I update the value field on ScaleState - Yet it still never changes if the user manually changes the value of the widget via interacting with the widget.

The most recent commit reflects this "broken" way of doing things (I compile the examples with nim r --path:. ./examples/widgets/scale.nim for reference).

Screencast from 2023-09-28 23-35-14.webm

can-lehmann commented 1 year ago

In your example code, the valueChanged callback needs to update the value in app

Like e.g. here: https://github.com/can-lehmann/owlkettle/blob/main/examples/dialogs/custom_dialog.nim#L77

can-lehmann commented 1 year ago

Your example currently sets the current value of the scale to a constant immediately after it is updated.

PhilippMDoerner commented 1 year ago

Waaaaait a second, so the cycle is: Input from App Widget --> Scale Widget State --> Scale Widget Rendered on screen --> User interacts with Widget --> valueChanged callback --> update App State of parent widget --> Rerender trigger from Scale Widget event callback --> Rerender passes on value via input to Scale Widget State --> Scale Widget Rendered on screen with updated value

So any user of Scale would have to write a callback like this in order for interacting with the widget to work properly:

          proc valueChanged(newValue: float64) =
            app.value = newValue

?

can-lehmann commented 1 year ago

Not sure what exactly you mean by the first couple of points, but generally it sounds correct.

can-lehmann commented 1 year ago

If a user sets the value field, they also need to keep it in sync with the actual state of the widget. This is by design, because it means that the user never needs to use something like Scale.getValue() (→ state is managed by the user, not by the GUI framework). The idea is pretty similar to what you might do in JS frameworks like react or mithril.js.

PhilippMDoerner commented 1 year ago

If a user sets the value field, they also need to keep it in sync with the actual state of the widget. This is by design, because it means that the user never needs to use something like Scale.getValue() (→ state is managed by the user, not by the GUI framework). The idea is pretty similar to what you might do in JS frameworks like react or mithril.js.

That was where I had my hangup, because I don't think it's that way in Angular. In angular (which is what I do work in) you basically set up your 2 way-binding between HTML-input und JS-backend within the component once via defining a field and putting that into the input via <input type="text" value=[(value)]/> and you're done, the user no longer has to do anything. The user can just manipulate value from the JS-side to get input to show the new value and vice versa, no setup required.

That was my holdup. If that's different design philosophies then that was simply me not quite getting that, thanks for clearing it up!

PhilippMDoerner commented 1 year ago

Okay, I think I got most functionality that is configurable from Range and Scale itself covered. Now I could also go through GtkWidget, GObject, GtkAccessible and GtkOrientable to look for more set procs that could be configured via properties on Scale.

Is that within the scope of the ticket? Do you maybe have a list of properties for any of those that any renderable widget should cover? Basically, where should I draw the line?

Off the cuff, I could make out just as an overview the following function that might be of interest and possibly worth wrapping:

GTK_Accessible:

GTK.Orientable:

GTK.Widget:

can-lehmann commented 1 year ago

Since you inherit from BaseWidget, all relevant fields from GtkWidget should already be available (in case anything should be missing, that is probably a separate PR). Since you have a orient field, adding a property hook for it that uses gtk_orientable_set_orientation makes sense. To be honest, I have not looked into GTK's accessability APIs yet, so since they are currently not supported by any other widget, adding support is definitely out of scope for this PR.

I think that feature wise, this issue is mostly complete (other than the orient field). Please open a PR, so we can discuss the remaining steps.

PhilippMDoerner commented 1 year ago

This issue thus closed?

can-lehmann commented 1 year ago

Yes :smile: