Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor playground to not use toListFormField + Remove unused imports from examples #131

Merged
merged 21 commits into from
Nov 5, 2023

Conversation

PhilippMDoerner
Copy link
Contributor

@PhilippMDoerner PhilippMDoerner commented Oct 25, 2023

This takes care of the refactoring we had discussed for playground and for which I had written up a reminder in #106 : #106 (comment)

Does exactly what the heading states.

This PR should be mostly independent from the other 10. I distantly recall one of the other 10 requiring me to implement another toListFormField proc, but I can adjust that.

While I was at it I also noticed a couple examples importing modules they no longer need, so just cleared that up to remove those warning messages.

I also did a slight change to the Scale example, as it really does not look pretty when you add a mark with position top/right to it if it takes up the entire Window. So I put it in a Box() widget with expand: false.

This changes how this all works.
Previously we generated expressions that act directly on `state`
to manipulate its values.

Now we fetch a pointer to each field on state or
each field of an instance in a seq and act on that.
This gets rid entirely of "toFormField", which can now be used
for an individual field as well as a field of an instance in a seq.
Nim 1.6.12 appears to not like the `ptr enum` type.
ptr[enum] seems okay though, so it just doesn't like the notation.
@PhilippMDoerner
Copy link
Contributor Author

PhilippMDoerner commented Oct 26, 2023

Fixed all outstanding remarks.

Sidenote, 1 more addition I made was:

Apparently range is not covered by current toFormField procs. So I made one that works for all ranges. That required introducing a Range concept in playground though, because apparently proc x(r: range) is not a valid parameter typeclass to make a generic for all ranges. proc x[T: range](r: T) is not workable as that clashes with proc x(r: auto). I am confused and not happy I had to introduce concept, but at least the code works 🤷

This requires introducing a `Range` concept as
`range` is not a valid typeclass to use as a generic parameter.
This is done in order to avoid any of the following
proc bodies accidentally missing out
on a proc that was provided befor e it.
This helps with not needing to care about the order that the
procs are defined in.
@PhilippMDoerner
Copy link
Contributor Author

I had a eureka moment and this did suddenly inspire me to play around again with complete generic form generation for any type.
I succeeded!

We can now fully generate the form for any given type with fields while still having the dummy-widget fallback for all those unknown types e.g. type Mine = distinct string.
This brings with it basically for free support for Option types.

See here a generic version of ScaleMark where I commented out the custom proc we had before (it also demonstrates it working with Option as the label of a ScaleMark is optional):
Screenshot from 2023-10-26 12-32-16
Screenshot from 2023-10-26 12-32-22
Screenshot from 2023-10-26 12-32-34

The expanding "delete" button on the right is an issue I couldn't quite get rid of and stems from the toFormField of seq[T].
I think that's an acceptable sacrifice.

@PhilippMDoerner
Copy link
Contributor Author

PhilippMDoerner commented Oct 26, 2023

And just like that I think I cracked the last piece.
We can now also generate forms for distinct types due to the newest toFormField proc.

Therefore we can fully auto-generate forms for the vast majority of nim-types.
And if anybody wants to customize it, they can by simply overriding toFormField.

I guess any type that nim provides via magic would not be possible to represent like this.
Further circular objects might just blow the entire thing up (I haven't tried yet, but I'd assume so).
But those are special cases I'd say. Particularly for our current purposes.
If we want to make this available for others we might have to deal with that.

@can-lehmann can-lehmann added this to the Owlkettle 3.0.0 milestone Oct 26, 2023
@PhilippMDoerner
Copy link
Contributor Author

PhilippMDoerner commented Oct 26, 2023

Err maybe just for reference: I do not plan on doing anything else on this unless there's feedback, I'm happy with the current level this is at and can't see anything right now myself that I'd change.

Not sure if that was clear or not... I guess it makes sense to ask what typically indicates to you that a PR is not getting any further work done on unless there's feedback requests it 😅

@PhilippMDoerner
Copy link
Contributor Author

Anything else?

examples/widgets/picture.nim Show resolved Hide resolved
owlkettle/playground.nim Outdated Show resolved Hide resolved
owlkettle/playground.nim Show resolved Hide resolved
This import is relevant when -d:pixbufAsync is set
This adds a new overload for toFormField to specifically
handle object variants.

Object Variants are hard to handle and thus by default shall receive
the placeholder field that reminds the user to define their own
toFormField proc for their variant.

The Overload works via having a ObjectVariant concept, which catches all
toFormField invocations for object variants.
It simply checks on a technicality if a type-field can be assigned
to itself. This is never true for kind fields on object variants as it
results in compiletime errors.
The new name is ObjectVariantType.
This serves as an important distinction that his is a concept that
covers **types**, not **instances**.
The doc comment was adjusted to clarify this as well.
Copy link
Owner

@can-lehmann can-lehmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref object variant types do not work correctly.

To do so the proc that checks it just needs to also accept
ref objects and use the `getIterator` template
to decide on whether or not to deref before using fieldPairs.
@can-lehmann
Copy link
Owner

Great work! 😄

@can-lehmann can-lehmann merged commit c34ceb9 into can-lehmann:main Nov 5, 2023
3 checks passed
@PhilippMDoerner PhilippMDoerner deleted the refactor-playground branch November 18, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants