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

Support for Streams from query objects #88

Merged
merged 24 commits into from
Oct 13, 2020

Conversation

Buggaboo
Copy link
Contributor

@Buggaboo Buggaboo commented Dec 25, 2019

@greenrobot-team Please review.

@Buggaboo Buggaboo changed the title WIP - Added support for _obx_observe_ and _obx_observe_single_type_ WIP - Added support for obx observers (single and multiple types) May 17, 2020
@Buggaboo Buggaboo changed the title WIP - Added support for obx observers (single and multiple types) WIP - Added support for obx observers (1 & >1 types) May 17, 2020
entity objects have changed, how did java/swift/go do this?
@Buggaboo Buggaboo changed the title WIP - Added support for obx observers (1 & >1 types) Help needed - Added support for obx observers (1 & >1 types) May 24, 2020
@Buggaboo Buggaboo changed the title Help needed - Added support for obx observers (1 & >1 types) Help required - Added support for obx observers (1 & >1 types) May 24, 2020
@Buggaboo Buggaboo changed the title Help required - Added support for obx observers (1 & >1 types) Help required: Support for obx observers May 24, 2020
@greenrobot-team
Copy link
Member

greenrobot-team commented Jun 29, 2020

@Buggaboo Thanks. Chiming in here: similar to the other bindings (e.g. Java https://docs.objectbox.io/data-observers-and-rx#data-observers-basics) observers are meant to notify that there was a change to a box (not what has changed) or more importantly return updated query results if there was a change to a box.

The internal APIs only provide ways to listen that there was a change to a single box or all boxes, not which changes. The binding should then add wrapper code so the user can subscribe to e.g. the latest query results (if there was a change, re-run the query and return the new results). Again, see the Java binding for examples.
https://docs.objectbox.io/data-observers-and-rx#data-observers-basics

Hope this helps.

@Buggaboo
Copy link
Contributor Author

@Buggaboo Thanks. Chiming in here: similar to the other bindings (e.g. Java https://docs.objectbox.io/data-observers-and-rx#data-observers-basics) observers are meant to notify that there was a change to a box (not what has changed) or more importantly return updated query results if there was a change to a box.

The internal APIs only provide ways to listen that there was a change to a single box or all boxes, not which changes. The binding should then add wrapper code so the user can subscribe to e.g. the latest query results (if there was a change, re-run the query and return the new results). Again, see the Java binding for examples.
https://docs.objectbox.io/data-observers-and-rx#data-observers-basics

Hope this helps.

Thanks, I'll look into it again, and try to follow the existing syntax.

@Buggaboo Buggaboo changed the title Help required: Support for obx observers Support for obx observers Jul 17, 2020
@Buggaboo Buggaboo changed the title Support for obx observers Support for dart Streams from query objects Jul 17, 2020
@Buggaboo Buggaboo changed the title Support for dart Streams from query objects Support for Streams from query objects Jul 17, 2020
@vaind vaind changed the base branch from dev to main July 17, 2020 10:34
Copy link
Contributor

@vaind vaind left a comment

Choose a reason for hiding this comment

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

Only had a very quick look, will need to dive deeper and play with the code to understand it fully before reviewing properly.

static final any = <int, Any>{}; // radix? > tree?

// sync:true -> ObjectBoxException: 10001 TX is not active anymore: #101
static final controller = StreamController<int>.broadcast();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that comment on an issue that needs looking at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that requires some attention

Copy link
Member

Choose a reason for hiding this comment

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

What does the reference of #101 mean here?

Copy link
Contributor Author

@Buggaboo Buggaboo Sep 28, 2020

Choose a reason for hiding this comment

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

It was generated here, see nativeCode.
GitHub issue number was triggered.

lib/src/query/query.dart Show resolved Hide resolved
### Streams

Streams can be created from queries.
The streams can be extended with [rxdart](https://github.com/ReactiveX/rxdart);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why rxdart? Are there other alternatives you've considered?

Copy link
Contributor Author

@Buggaboo Buggaboo Jul 17, 2020

Choose a reason for hiding this comment

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

It seems to be the most popular one, and it supports Stream which is more idiomatic, by way of extensions. I expose a stream property from an observable query for that reason.

@greenrobot-team
Copy link
Member

@Buggaboo A general remark: please look at the Dart Analysis issues and resolve them if possible (e.g. compare to changes made to your last PR).

Pointer.asFunction complained that the typedef parameter was not proper.
});

box.put(TestEntity(tString: 'Hello world'));
await Future.delayed(Duration(seconds: 0)); // ffi explodes without
Copy link
Member

Choose a reason for hiding this comment

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

The docs for Future.delayed contain

If the duration is 0 or less, it completes no sooner than in the next event-loop iteration, after all microtasks have run.

I don't know much about Dart, but this sounds like the observer callback is held up from executing while this "thread" is active. Might this cause issues in an actual app where the callback execution is held back too long? Is it possible to run them on a separate "thread" so they are not held up by other code?

Copy link
Member

@greenrobot-team greenrobot-team Oct 5, 2020

Choose a reason for hiding this comment

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

Hm, reading the StreamController.broadcast docs on using sync is false mode (recommended) this actually sounds like expected behavior (events may be delivered at any point after the code adding the event has completed). LGTM then.

Copy link
Member

@greenrobot-team greenrobot-team Oct 5, 2020

Choose a reason for hiding this comment

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

Maybe remove those ffi explodes without comments then and clarify regarding delayed event delivery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this.


void dispose() {
_query.close();
_store.unsubscribe();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be part of close(). This feels like something very easy to forget.

Copy link
Contributor Author

@Buggaboo Buggaboo Oct 5, 2020

Choose a reason for hiding this comment

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

It's called by close iirc. I didn't want to expose the BLoC/VM's internals to the Widgets. Leaky abstractions and such.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, should have been more clear: was talking about not even exposing unsubscribe() to the user and doing it as part of close().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@greenrobot-team
Copy link
Member

@Buggaboo Thanks for your latest changes and 👍 for also improving our example app! I had some more questions, but this LGTM now.

@vaind Can you look at observable.dart and check if using store.prt as user_data when creating an observer is fine? My knowledge is lacking there. (Note: is on holiday, so this might take a while.)

@greenrobot-team
Copy link
Member

Sorry, forgot one thing: look at Dart Analysis issues and run the formatter.

@greenrobot-team greenrobot-team merged commit 164a646 into objectbox:main Oct 13, 2020
@greenrobot-team greenrobot-team added this to the 0.8 milestone Oct 13, 2020
@greenrobot-team greenrobot-team added the enhancement New feature or request label Oct 13, 2020
@greenrobot-team
Copy link
Member

Squashed and merged. I fixed the analysis and formatting issues myself (0a941a5).

Thanks again.

@Buggaboo Buggaboo deleted the feature/future-stream branch October 13, 2020 13:43
@vaind
Copy link
Contributor

vaind commented Oct 21, 2020

@vaind Can you look at observable.dart and check if using store.prt as user_data when creating an observer is fine? My knowledge is lacking there. (Note: is on holiday, so this might take a while.)

Should be fine to the best of my knowledge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants