-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
Rework ViewElement to open new usages #809
Rework ViewElement to open new usages #809
Conversation
Introduced So this should have a great impact on the memory allocation due to the sheer numbers of |
type ViewElementHolder(viewElement: ViewElement) = | ||
let ev = new Event<_,_>() | ||
type ViewElementHolder(viewElement: IViewElement, programDefinition: ProgramDefinition) = | ||
let ev = Event<_,_>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also want to populate this event object on-demand (mutable, null initially)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will take a look if that's possible. Not sure if Xamarin.Forms expects PropertyChanged
right after the creation.
src/Fabulous/IViewElement.fs
Outdated
Trace: TraceLevel * string -> unit } | ||
|
||
and IViewElement = | ||
abstract Create: ProgramDefinition -> obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess ///
comments on each of these would be good :-) Given the central nature of the interface :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm planning to put ///
comments on all public surface APIs.
src/Fabulous/Runner.fs
Outdated
member x.SetDispatchThunk v = dispatchImpl <- v | ||
|
||
/// Program type captures various aspects of program behavior | ||
type RunnerDefinition<'arg, 'msg, 'model, 'externalMsg> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH this should really have always been an interface I think. I made it a record out of deference to the Elmish/Elm origins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The record did not bother me because it's easy to extend with optional and/or custom behaviors through Program.with...
.
An interface could do it too I guess, but might not be as simple to use.
I need to think about it.
@TimLariviere I love what you're doing here |
Thanks! |
This PR also reworks the way we print some debug info to the console, by using a built-in tracing tooling. |
ec443e5
to
a82456d
Compare
a82456d
to
aaeb9da
Compare
0839cd5
to
9790f28
Compare
Runner + Tracing XF Program WIP Registrar/Handler AttachTarget/DetachTarget Fix ViewConverters/ViewUpdaters Fix CodeGen Fix CodeGen 2 Fix CodeGen 3 Working CounterApp & FabulousWeather samples Fix other samples except Shell AllControls ComponentViewElement Working external msg for component Fix merge Fix merge
9222ddf
to
01a8be6
Compare
As part of the allocations reduction process, I'm reworking the
ViewElement
class to become an interfaceIViewElement
.This will allow for different implementation such as
ComponentViewElement
(see TimLariviere#25 for more information).In this example,
ComponentViewElement
would drastically reduce the number of allocated ViewElement at each update.