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

Update to XF 3.4 complete. #257

Merged
merged 24 commits into from
Jan 15, 2019
Merged

Conversation

rbrogan-git
Copy link

@rbrogan-git rbrogan-git commented Dec 15, 2018

Issue #243
I am not an expert in paket but I think this is all that is needed, correct? I ran the builds and tests after this update successfully. I also launched the tic tack toe and all controls samples for android and all is working fine.

Let me know if this is good or of you need anything else.

Thanks!
-Roger

@slang25
Copy link

slang25 commented Dec 15, 2018

There's also the paket.lock file which should be updated too. You can do it by running paket install, that will update it and reflect the changes you've made to the dependencies file.

@rbrogan-git
Copy link
Author

@slang25 Thanks that is what I wanted to know. Added the file.

@rbrogan-git
Copy link
Author

rbrogan-git commented Dec 16, 2018

@slang25 Thanks! Found a another place with hard coded string versions. Any reason we can't pull this from paket with the build script?

@PureWeen
Copy link

I think the wrappers also need to be updated with this

Here's an API diff. Not too many on existing controls and then there's a new ImageButton control
https://www.fuget.org/packages/Xamarin.Forms/3.4.0.1008975/lib/netstandard2.0/diff/3.1.0.697729/

@rbrogan-git
Copy link
Author

@PureWeen Thanks! I has thinking some of that might show up as a compiler error. I will look at the wrappers.

@TimLariviere
Copy link
Member

@rbrogan-git Sorry, should have been more explicit in the issue. Just bumping the version number is not enough. Fabulous requires a minimum version of Xamarin Forms (currently 3.1) but devs are free to update on their own.

What really interests us here is, like @PureWeen said, to update https://github.com/fsprojects/Fabulous/blob/master/tools/Generator/Xamarin.Forms.Core.json with the wrappers around the new controls, the new properties, the changed features, etc. between Xamarin.Forms 3.1 and 3.4.

You can take a look at a previous PR which updated from 3.0 to 3.1 : #178

@slang25
Copy link

slang25 commented Dec 16, 2018

Could this be a 2 step process? Update the dependencies fixing any breaking changes, then PRs to lift through new changes. Or does that risk missing some of the apis?

I presume the json was semi-automated and now a manual task to add to?

@TimLariviere
Copy link
Member

The json file is fully manual for now (which causes some issues like forgotten controls or properties).
If we can find a way to automatically generate the json file and then customize it by hand, that would be awesome. (And open a way for 3rd parties)

I don’t feel like XF 3.4 is a major enough update to require a 2 step process.

Like you said, it would be hard to track every new features if we do it in separate PRs.

Also people might expect full support if we require them to update to XF 3.4.

@rbrogan-git
Copy link
Author

rbrogan-git commented Dec 17, 2018

@TimLariviere I understand the ask. I reviewed the pull request to see what was changed from 3.0 to 3.1.
I would have been surprised if it was just a version bump.
I also think it is a great idea to automatically generate the wrappers. But in the mean time do you have any conceptual docs on x.f.core.fs and x.f.core.json? 12,000 lines is a lot of boiler plate to weed through just to get the high level "what" is going on. I can eventually reason it out on my own but the conceptual architecture would speed it up quit a bit. Just so you understand the ask, I have 20 years of Java and C# but I am relatively new to F#. I love the elegance of F# and am looking to get more experience which is why I have been poking around here.

@TimLariviere
Copy link
Member

TimLariviere commented Dec 17, 2018

@rbrogan-git Unfortunately no, we don't have that kind of documentation...
I'm currently refactoring the Generator, so it's a good time to write it.

The idea here is to add/update/remove any control/property/event accordingly in the Xamarin.Forms.Core.json file that have changed between XF 3.1 and 3.4.
This file is read at build time by the Generator which will generate the Xamarin.Forms.Core.fs file.
Xamarin.Forms.Core.fs is quite big and hard to read, you don't need to go into it.

(Tip: You can run the generator quickly with the command .\build.cmd RunGenerator)

The way the JSON file is defined is as such:

{
  "assemblies": [
    // Paths to dlls that will be loaded to do some reflection on it
    "packages/neutral/Xamarin.Forms/lib/netstandard1.0/Xamarin.Forms.Core.dll",
    (...)
  ],
	"outputNamespace": "Fabulous.DynamicViews",

        // List of controls for which to generate a Fabulous wrapper
	"types": [
		{
                        // Name of the actual implementation
			"name": "Xamarin.Forms.Element",

                        // List of properties or events of that control that we want to add to the wrapper
			"members": [ 
                                
				{
					"name": "ClassId",        // Name of the property
					"defaultValue": "null"    // Default value
                                },
                                (...)
                        ]
                },
               (...)
          ]
}

name and defaultValue are the strict minimum you need to add. The generator will try to infer everything else regarding ClassId using Reflection in the class Xamarin.Forms.Element. (Is it a property, an event? What type is it, bool, string? Does it inherit from another control? etc.)
If the property doesn't exist, the Generator will throw an error (non-existing properties are available, but its an advanced concept).

To enable more customization, there's a bunch of other attributes.

"name": "ItemsSource",
"defaultValue": "[]",

// Useful when there's multiple controls that have the same property name but are not related (different types)
"uniqueName": "ListViewGrouped_ItemsSource",

// Short name that will be used instead of the "name" in lower Pascal Case
"shortName": "items",

// What we expect people to provide in the View.MyControl(myProperty = value)
"inputType": "(string * ViewElement * ViewElement list) list",

// How we actually store the value (for performance reasons. Especially useful when we ask a list, but actually store it has an array)
"modelType": "(string * ViewElement * ViewElement[])[]",

// How we convert from "inputType" to "modelType"
"convToModel": "(fun es -> es |> Array.ofList |> Array.map (fun (g, e, l) -> (g, e, Array.ofList l)))",

// Custom update logic if the simple check between old and new value is not enough
"updateCode": "updateListViewGroupedItems"

updateCode refers to a function found here: https://github.com/fsprojects/Fabulous/blob/master/src/Fabulous.Core/ViewConverters.fs

If we take an example:
From the link of @PureWeen, we can see that there's a new CornerRadius property on BoxView, so we can go to the declaration of BoxView in the json file and add a new entry:

{
	"name": "Xamarin.Forms.BoxView",
	"members": [
		{
			"name": "Color",
			"defaultValue": "Xamarin.Forms.Color.Default"
		},
		{
			"name": "CornerRadius",
			"defaultValue": "Unchecked.defaultof<Xamarin.Forms.CornerRadius>"
		}
	]
},

I used Unchecked.defaultof in defaultValue because CornerRadius is a struct (no null) and doesn't have a default value if we check the implementation in Xamarin.Forms
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/CornerElement.cs

Another example:
WebView has a new event ReloadRequested, so we can add it in the json file

{
	"name": "Xamarin.Forms.WebView",
	"members": [
		{
			"name": "Source",
			"uniqueName": "WebSource",
			"defaultValue": "null"
		},
		{
			"name": "Navigated",
			"defaultValue": "null",
			"inputType": "Xamarin.Forms.WebNavigatedEventArgs -> unit",
			"convToModel": "(fun f -> System.EventHandler<Xamarin.Forms.WebNavigatedEventArgs>(fun _sender args -> f args))"
		},
		{
			"name": "Navigating",
			"defaultValue": "null",
			"inputType": "Xamarin.Forms.WebNavigatingEventArgs -> unit",
			"convToModel": "(fun f -> System.EventHandler<Xamarin.Forms.WebNavigatingEventArgs>(fun _sender args -> f args))"
		},
		{
			"name": "ReloadRequested",
			"defaultValue": "null",
			"inputType": "System.EventArgs -> unit",
			"convToModel": "(fun f -> System.EventHandler(fun _sender args -> f args))"
		}
	]
},

I hope it's clear.

@rbrogan-git
Copy link
Author

@TimLariviere yes this is immensely helpful! Just what I needed to see. Glad the x.f.core.fs is generated. Makes much more sense now.

@rbrogan-git
Copy link
Author

@TimLariviere This is my first pass on fixing up the json file. I tried to add the TitleView property but was not successful so I kept it out for now. I am unclear on how to setup the convert function for View.

Suggestions welcome. Let me know if I missed anything.
Thanks!

@rbrogan-git
Copy link
Author

@TimLariviere I know I am missing the TitleView property but other than that I think I got all the additions.
If you can advise me on setting up the convert function for View I will add the TitleView back. @PureWeen if you have any advise I would appreciate it.

Thanks and Happy New Year!

@TimLariviere
Copy link
Member

TimLariviere commented Jan 2, 2019

@rbrogan-git Thanks! I'll take a look soon.
GitHub has some troubles to correctly diff the json file, due to the change of formatting.
To help with that, I've added a new build step that forces a specific formatting (independent of the IDEs).

Could you rebase your PR on the last commit on master please?
After that, do a commit after running the command ./build.sh -- .\build.cmd on Windows.
This will simplify the reviewing of your pull request. :)

@rbrogan-git
Copy link
Author

@TimLariviere I wondered why the diff got so weird. Updated. Thanks!

Copy link
Member

@TimLariviere TimLariviere left a comment

Choose a reason for hiding this comment

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

Awesome work! A lot of the new features are covered.

In additions to my review comments, here is a list of missing properties/controls that I found.

  • Button
    • Padding (Thickness) It is already here, I missed it
  • ImageButton
    • Command (unit -> unit) -- Same as Command on Button
    • IsLoading (bool) -- Settable via this.SetIsLoading(bool)
    • IsPressed (bool) -- Settable via this.SetIsPressed(bool)
      Finally, not sure it's a good idea to add IsLoading/IsPressed as their setter methods are marked EditorBrowsableState.Never
  • NavigationPage
    • TitleView -- Settable via NavigationPage.SetTitleView(this, view)
  • SwipeGestureRecognizer
    • Command (unit -> unit) -- Same as Command on Button
    • Direction (SwipeDirection / default enum<SwipeDirection>(0))
    • Threshold (uint32 / default 100)
    • Event Swiped (EventHandler<SwipedEventArgs>)
  • WebView

tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
tools/Generator/Xamarin.Forms.Core.json Outdated Show resolved Hide resolved
@TimLariviere
Copy link
Member

For TitleView, I still need to find the correct way to do it.
I'll keep you posted.

@TimLariviere
Copy link
Member

Oops, sorry for the conflict.

TimLariviere and others added 9 commits January 4, 2019 12:45
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
Co-Authored-By: rbrogan-git <roger.brogan@gmail.com>
@TimLariviere
Copy link
Member

TimLariviere commented Jan 9, 2019

@rbrogan-git Here's a way to implement TitleView.

In the json file, we declare the property like usual but Xamarin.Forms is expecting a Xamarin.Forms.View and we only have a ViewElement.
So we will implement a custom update logic to convert our ViewElement to View (and implement incremental update too).

{
  "name": "TitleView",
  "defaultValue": "null",
  "inputType": "ViewElement",
  "updateCode": "updateNavigationPageTitleView"
},

We then need to add this update function into https://github.com/fsprojects/Fabulous/blob/master/src/Fabulous.Core/ViewConverters.fs
There, we will take the previous ViewElement (if any), the new ViewElement (if any) and the NavigationPage to update.
With the help of some existing functions, we can also implement incremental update.

let updateNavigationPageTitleView (prevOpt: ViewElement voption) (currOpt: ViewElement voption) (target: Xamarin.Forms.NavigationPage) =
    match prevOpt, currOpt with
    | ValueSome prev, ValueSome curr when identical prev curr -> ()
    | ValueSome prev, ValueSome curr when canReuseChild prev curr ->
        updateChild prev curr (NavigationPage.GetTitleView(target))
    | _, ValueSome curr ->
        NavigationPage.SetTitleView(target, (curr.Create() :?> Xamarin.Forms.View))
    | ValueSome _, ValueNone ->
        NavigationPage.SetTitleView(target, null)
    | _, _ -> ()

Might be some errors in this code. I haven't tested it yet

@rbrogan-git
Copy link
Author

@TimLariviere thanks for all the information and help with this pull request. I learned a lot about this project. Sorry you ended up finishing it yourself. I hope the work I did was helpful.

@TimLariviere
Copy link
Member

@rbrogan-git Your work was more than helpful. You did the most complex and time-consuming part by adding the new properties. :)
I want to include your PR in the next release (0.30.0), so I took the initiative to finish the few requested changes.
Thanks again!

@TimLariviere TimLariviere reopened this Jan 15, 2019
@TimLariviere TimLariviere merged commit 4c5a7f3 into fabulous-dev:master Jan 15, 2019
@TimLariviere TimLariviere mentioned this pull request Jan 15, 2019
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.

4 participants