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

WIP incremental loading #1

Open
wants to merge 58 commits into
base: new_observablecollection_merge
Choose a base branch
from

Conversation

xperiandri
Copy link
Owner

@TysonMN I started the implementation of incremental loading support.
What do you think of naming and API shape?

@TysonMN
Copy link

TysonMN commented May 2, 2021

Looks interesting, but I don't think I fully understand. Is there a sample that demonstrates this new behavior?

@xperiandri
Copy link
Owner Author

I expect to update some sample tomorrow (one way seq or submodel seq).
But you can get an idea form the second commit.

Now I struggle with the issue dotnet/fsharp#11516
Do you have any ideas?

@xperiandri
Copy link
Owner Author

@TysonMN, the OneWaySeq sample is updated to include ISupportIncrementalLoading

@TysonMN
Copy link

TysonMN commented May 4, 2021

When I try to compile the branch (currently at commit e4df967), there are ~90 compile errors. I was able to avoid them by unloading Elmish.Uno.Tests and Samples.Skia.Wpf.Host.

I was able to run the OneWaySeq sample. I know what you mean now by incremental loading. The idea seems great.

As for the design, I am a bit put off by the use of TaskCompletionSource<_>. However, I don't immediately see a better way to handle this.

Incremental loading is not a top priority for me at the moment, but I don't want to forget about it either, so I just created elmish/Elmish.WPF#384.

@xperiandri
Copy link
Owner Author

xperiandri commented May 4, 2021

WPF requires GTK to be installed as far as I know

@xperiandri xperiandri force-pushed the incremental_load branch 2 times, most recently from d51f144 to e9810db Compare May 16, 2021 22:46
@xperiandri
Copy link
Owner Author

@TysonMN how would you recommend covering this new functionality with tests?

@TysonMN
Copy link

TysonMN commented May 18, 2021

Given my limited knowledge of this feature, I would want to spend more time on the API. I still hope that uint * TaskCompletionSource<uint> -> unit can be replaced with something more idiomatic like uint -> Async<uint>. Then I expect that this improved API would simplify testing.

Is it possible to replace uint * TaskCompletionSource<uint> -> unit with uint -> Async<uint>?

@xperiandri xperiandri force-pushed the incremental_load branch 2 times, most recently from 2adf622 to 7348396 Compare August 24, 2021 14:18
@xperiandri
Copy link
Owner Author

Probably but I don't know how.

| LoadMore (count, tcs) ->
    let intCount = int count
    let builder = m.IncrementalLoadingNumbers.ToBuilder()
    let max = FlatList.last m.IncrementalLoadingNumbers
    for i = max + 1 to max + intCount do
      builder.Add(i)
    tcs.SetResult(count)
    { m with IncrementalLoadingNumbers = builder.ToImmutable() }

If I replace TaskCompletionSource with Async<uint> I can change this into something like:

type Msg =
  | LoadMore of count: uint * complete: (uint -> Async<unit>)
  | LoadedMore of items: FlatList<int>

let asyncLoadItems (complete: uint -> Async<unit>) (items: FlatList<int>) count = async {
  let intCount = int count
  let builder = items.ToBuilder()
  let max = FlatList.last items
  for i = max + 1 to max + intCount do
    builder.Add(i)

  do! complete count
  return LoadedMore <| builder.ToImmutable ()
 }

let update msg m =
  match msg with
  | LoadMore (count, complete) -> m, asyncLoadItems complete m.IncrementalLoadingNumbers count |> Cmd.OfAsync.result
  | LoadedMore items -> { m with IncrementalLoadingNumbers = items }, Cmd.none

Is it better?

@TysonMN
Copy link

TysonMN commented Aug 24, 2021

I think so, but I would need to play with it.

I am making steading progress (something nearly every day) on Elmish.WPF, so I hope that v4 is not far off. Then I can focus on other ideas like this one.

Keep up the good work! :)

@xperiandri
Copy link
Owner Author

@TysonMN I've created a new branch with simplification and necessary explanation comments
https://github.com/xperiandri/Elmish.Uno/tree/incremental_load_simplified
Have a look at the latest commit in it, please

@TysonMN
Copy link

TysonMN commented Sep 18, 2021

(Sorry for the delay)

Overall, it looks better.

Would it be possible to add incremental loading in a single commit? I think that would make it easier for me to follow the changes.

@xperiandri xperiandri force-pushed the incremental_load branch 2 times, most recently from 738d2d2 to c89a7bc Compare October 16, 2021 11:11
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