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

Better type-safety for View API #341

Closed
10 tasks
Zaid-Ajaj opened this issue Feb 20, 2019 · 9 comments
Closed
10 tasks

Better type-safety for View API #341

Zaid-Ajaj opened this issue Feb 20, 2019 · 9 comments
Labels
s/ready This issue is ready to be implemented t/enhancement New feature or request
Milestone

Comments

@Zaid-Ajaj
Copy link

Zaid-Ajaj commented Feb 20, 2019

I have been playing with Fabulous lately and I got many bugs that could have been avoided if the View API had better type-safety, this is what simple-elements is trying to solve but even then it's still flaky,

Related to #139

F# 5.0 is not needed to fix these issues

ViewElement composition

Right now, you can use ViewElement virtually everywhere but you will get runtime exceptions: "The specified cast is not allowed" if you used a ViewElement in an "incorrect" place which doesn't tell you where it went wrong and is very problematic:

  • Using normal elements (Button, Label etc.) as GestureRecognizers
    • Use an interface IGestureRecognizer and have specific gesture elements implement it
    • gestureRecognizers attribute will then accept type IGestureRecognizer list
  • incorrectly nesting ContentPage elements (using ContentPage as the content of another ContentPage)
    • Use an interface IPage to for page element
    • View.NavigationPage(...) returns IPage
    • View.ContentPage(...) returns IPage
    • pages of View.NavigationPage must accept IPage list
    • content of View.ContentPage must accept ViewElement list
  • Using View.SwitchCell inside the children of View.StackLayout (probably other types of Cell descendants)
  • probably many more I haven't encountered yet

Attribute type-safety: remove obj attributes

  • margin and padding as Thickness
  • source for View.Image as ImageSource
  • rows in View.Grid as RowDefinition list
  • columns in View.Grid as ColumnDefinition list
  • remove commandParamter attribute because it doesn't do anything
  • many more

Restrict ViewElement extension methods

All ViewElement values share the same extension methods which is really weird, for example you can have:

View.Button(text="What am I?").HasNavigationBar(true)

I am not sure if this gives exceptions but I am pretty sure it is incorrect

@dsyme
Copy link
Collaborator

dsyme commented Feb 20, 2019

Three thoughts on this

  • I agree we need to tighten some things here

  • I haven't found the first set of problems to be that bad. But it's good to get anecdote and evidence!. Question - Did you really use ContentPage in ContentPage, and Button for gestureRecognizer? I'm generally opposed to fine grain type distinctions and hierarchies but perhaps a Page/View/Cell/GestureRecognizer granular distinction would be appropriate and feasible without trashing type inference

  • The second set of problems are real and add we should address them.

@Zaid-Ajaj
Copy link
Author

Button for gestureRecognizer?

No I didn't but it is an example of the API indicating something valid that isn't

Did you really use ContentPage in ContentPage

Yes I actually did and it took me a while to figure out what was wrong, I had this code:

let layout = 
  StackLayout.stackLayout [ 
     (* bunch of stuff in here*)
  ]

NavigationPage.navigationPage [
    NavigationPage.Pages [
        ContentPage.contentPage [ 
            ContentPage.Content layout 
        ] 

        (* more pages here *)
    ]
]

then I started refactoring layout into something complex, broke it down even further and it turned into ContentPage I forgot that I already was using one in the navigation page. When I wanted to see the changes I made, I only got a cryptic runtime error saying something went wrong but not what exactly

@TimLariviere
Copy link
Member

I agree it would be better if we remove most of the obj types.
A Page/View/Cell/GestureRecognizer granular distinction seems to be a good way to do this. It's more or less how Xamarin.Forms sees controls.
We might want to add Layout in the mix as well.

margin and padding as Thickness
(...)

I also agree this one is problematic.
Fabulous tries to give the same abilities than it's XAML equivalent.
So Margin could be a single number (uniform margin), same with GridRow/GridColumn.

<Button Margin="10" />
is the same than
button.Margin = new Thickness(10);

<ColumnDefinition Width="Auto" />
is the same than
columnDefinition.Width = GridLength.Auto;

<RowDefinition Width="5*" />
is the same than
rowDefinition.Width = GridLength(5, GridUnitType.Star);

---
Fabulous

View.Button(margin = 10.0)
View.Grid(cols = [ box "auto"; box "5*"  ])

But I think it's doing Fabulous a disservice.
We want the most strongly-typed virtual UI as possible, but end up with a loosely-typed one due to supporting the same thing than XAML.
That's why I initially opened #139

In the case of Thickness, it's acceptable to write View.Button(margin = Thickness(10.0)).
GridLength is a bit lenghty, so maybe a discriminated union would be better View.Grid(cols = [ Star 5; Auto; Px 150 ]

source for View.Image as ImageSource

For some cases like this one, I think we shouldn't use the underlying type as a constraint.
View.Image(source="some/path/to/image.png") is definitely a good way to use an image.
But this keeps the obj type around until F# 5.0...

@Zaid-Ajaj
Copy link
Author

@TimLariviere

But I think it's doing Fabulous a disservice

Totally agree with you one this one

source for View.Image as ImageSource
For some cases like this one, I think we shouldn't use the underlying type as a constraint.

IMHO correctness > API ergonomics: I know a string is easier to work with but it is just as easy if someone would write a simple function to get an ImageSource if one finds himself/herself needing this too often:

let source path = ImageSource.FromFile(path)
View.Image(source = source "some/path/to/image.png")

if you want to improve usage with F# 5.0 then you probably want string | Uri | Stream and again ImageSource as the type would still be better because string doesn't say if the ImageSource is coming from a file or an embedded resources (both take string as input)

@TimLariviere
Copy link
Member

@Zaid-Ajaj

IMHO correctness > API ergonomics

Agree.

I think we should review each property and decide if it's worth providing some built-in tools for them.
For this specific example of ImageSource, I think it's essential to provide this conversion with Fabulous.
But I'm not a fan of loose functions that are not discoverable, a discriminated union would be better here.

@dsyme
Copy link
Collaborator

dsyme commented Feb 26, 2019

I'm ok with you guys making changes here, as long as the docs give lots and lots of snippets (most don't write this code by hand, they just cut and paste samples). Also please assess the changes with beginners in mind.

IMHO correctness > API ergonomics

Both are important.

@TimLariviere
Copy link
Member

A bunch of the changes have been included in #530

Didn't tackle the ViewElement composition problem for now.
To do that, I think we should add a generic ViewElement and generate the View API a bit differently. (much like FuncUI)
Using covariance should do the trick.

@TimLariviere
Copy link
Member

@TimLariviere TimLariviere modified the milestones: Fabulous 2.0.0, Fabulous for Xamarin.Forms 2.0.0, Fabulous.XF vNext (2.0), vNext (2.0) Feb 10, 2020
@TimLariviere TimLariviere added s/ready This issue is ready to be implemented and removed a/controls labels Feb 18, 2020
@TimLariviere
Copy link
Member

v2 comes with a strongly-typed DSL now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s/ready This issue is ready to be implemented t/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants