can-lehmann / owlkettle

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

Add editable label #111

Closed PhilippMDoerner closed 8 months ago

PhilippMDoerner commented 8 months ago

Adds the editable label widget.

NOTE: Setting editing to false from the outside currently does nothing. The main reason for that is that I can not know from within the widget whether editing being set to true means you want to commit the changes or not.

When the user inputs data, I don't have to know that because GTK handles it for me (Default = commit the changes, pressing esc = revert to initial text). When the application changes the editing state, it can't also pass to me the info whether to commit, thus I'm stuck. Therefore it only starts the editing mode on the internalWidget, never stops it.

Further: The only reason I manage to sync back changes to edited to the owlkettle widget is via useage of the notify signal. That one luckily was much easier than expected. It felt like spam and resource wasting to trigger a re-render on just a state change of a widget that mostly handles this kind of stuff itself, so I didn't trigger one. I can of course change that.

PhilippMDoerner commented 8 months ago

Heyho, I don't really have anything else to change here, I'm mostly busying myself with wrapping more widgets in the meantime rather than double checking this one.

Is there anything that still needs to be done here?

can-lehmann commented 8 months ago

NOTE: Setting editing to false from the outside currently does nothing. The main reason for that is that I can not know from within the widget whether editing being set to true means you want to commit the changes or not.

I am a bit concerned with the semantics here, as this way owlkettle's state will get out of sync with GTK's widget tree. Another similar issue occurs here:

let isEditing: bool = gtk_editable_label_get_editing(state.internalWidget).bool
if not isEditing:
  gtk_editable_set_text(state.internalWidget, state.text.cstring)

As EditableLabel inherits from Editable, it should be possible to handle changed in order to fix these issues.

A question on the general workflow: Would you prefer me to merge this first and wait with the refactor PR, or should I merge the refactor PR as soon as possible, so you can get reviews on the other widgets you wrapped? (even if you would have to rebase this PR?)

PhilippMDoerner commented 8 months ago

Regarding Workflow: First this, then the refactor PR.

Regarding the semantics: Do you mean trying to update editing as part of the changedCallback? I don't think that works, as editing gets triggered every keystroke and I can't know if that's the last one you want to make before I throw you out of edit mode. I'd need to listen for something like "activate".

Going over the docs though, I notice that I learned quite a lot these last 9 days because I'm noticing that I literally did not wrap a couple properties that I deem relevant to have "fuller" feature completeness, those being from GtkEditable. Those being enableUndo, widthChars, maxWidthChars, xalign and maybe more. I'll add those in a moment.

can-lehmann commented 8 months ago

Do you mean trying to update editing as part of the changedCallback? I don't think that works, as editing gets triggered every keystroke and I can't know if that's the last one you want to make before I throw you out of edit mode. I'd need to listen for something like "activate".

Sorry, my recommendation from before "As EditableLabel inherits from Editable, it should be possible to handle changed in order to fix these issues." makes no sense in this context, I didn't look at the code closely enough and thought you were currently using some activate-like signal.

I added comments to the two places where I currently see issues with the semantics. Basically you just need to always update the values.

PhilippMDoerner commented 8 months ago

Do you mean trying to update editing as part of the changedCallback? I don't think that works, as editing gets triggered every keystroke and I can't know if that's the last one you want to make before I throw you out of edit mode. I'd need to listen for something like "activate".

Sorry, my recommendation from before "As EditableLabel inherits from Editable, it should be possible to handle changed in order to fix these issues." makes no sense in this context, I didn't look at the code closely enough and thought you were currently using some activate-like signal.

I added comments to the two places where I currently see issues with the semantics. Basically you just need to always update the values.

I think missing the two way binding in the example messed me up, because when I had those changes in there it blocked editing as a whole. With the 2 way binding in place it works perfectly fine. Applied the necessary fixes!

can-lehmann commented 8 months ago

Could you rebuild the docs?

PhilippMDoerner commented 8 months ago

Donezo

can-lehmann commented 8 months ago

Looks good! :smile: