search

adithyaxx / animated-stream-list

A dart package to easily display a list with animated transitions
Other
37 stars 22 forks source link

Isolates #4

Open ericmartineau opened 4 years ago

ericmartineau commented 4 years ago

Thanks for this library - I've been looking for a dart implementation of the myers diff algorithm. I have a few thoughts... I'm planning to publish a fork of this project to accommodate for some of my needs, but I would prefer to not split our efforts if we can avoid it. Here are my thoughts:

  1. I believe the diff portion of this library should be its own library. In my case, I need the diff capabilities independent of the flutter widgets, and as it stands, the underlying diff components aren't publicly accessible (of course I can break convention and import the dart files from lib, but I'd rather not have to do that).

  2. I believe the diff implementation should be exposed without the use of Futures/Isolates - that way, the consumers can execute the diff using whatever async strategy best suits their purposes, for example, the diff might happen as part of a function that's already running in a separate isolate.

  3. In most cases, the diff will (and should) happen in a separate isolate from the main thread, so the library should make careful design considerations of the limitations of isolates. For example, I don't believe the current approach for Equality (setting it as a static variable) will work properly, because static variables do not persist across Isolate instances, and (at least last time I tried), one could not pass function pointers as arguments into isolates. Even if it works when using Isolate.spawn, it will not work when the isolates are created manually.

Because of this, I believe that we should model Equality the same as the collection library does, where it exists as an abstract class instead of a function pointer. It's not as convenient, but will be more reliable.

Similarly, it's possible that the objects contained in the lists are unable to cross Isolate boundaries... in which case, we'll need another strategy, like constructing a delegate for each list item that can be passed into the isolate, and then 'rehydrate' the original list elements in the calling Isolate.

Nice work - I hope you find my feedback useful. I'm working on a deadline, so I'll probably publish the library, but I would love to figure out how to combine efforts. Thanks

feedrdev commented 3 years ago

As stated above by Eric, the use of a static to pass the Equalizer function down to an isolate seems unsafe and unreliable. Can the eq be bundled as an argument instead? Also, is using isolates here really necessary? Would not Future.microtask suffice to make it async. Diffing should not take too much time for the kind of lists we are working with here in the UI.