Skip to content
This repository has been archived by the owner on Aug 17, 2020. It is now read-only.

Support more fine-grained notifications for queries #10

Closed
iomu opened this issue Feb 20, 2015 · 8 comments
Closed

Support more fine-grained notifications for queries #10

iomu opened this issue Feb 20, 2015 · 8 comments

Comments

@iomu
Copy link

iomu commented Feb 20, 2015

As it stands now, a Observable<Query> emits a new query object for every change in the table, even if the corresponding rows of the query haven't changed.

It would be great if means of further filtering the notifications were added to avoid these unnecessary updates (and database queries).

Maybe something like tags for queries and updates/inserts/deletes:

// Tag the query with a string
Observable<Query> users = db.createQuery("users", "tag", "SELECT * FROM users WHERE company=?", "Square");
// This observable would now only be refreshed with database updates that are tagged with "tag"

// Tag an insert with the same tag
db.insert("users", createUser("jw", "Jake Wharton", "Square"), "tag"); // users gets notified

db.insert("users", createUser("lp", "Larry Page", "Google")); // the tag is optional, users wouldn't get notified
@JakeWharton
Copy link
Member

This has not yet been a problem in practice. I'm hesitant to add any kind of granular system without a more concrete example of this actually being a problem.

@iomu
Copy link
Author

iomu commented Feb 20, 2015

The scenario I was imagining was the following:

Say you have an app that can display arcticles and comments. The comments are fetched when an article detail view is opened and shown below the article when loaded. You can now swipe between article detail views (via a viewpager). The offscreen pages would now preload their comments and the observable on the visible page would be refreshed if offscreen comments are loaded. I haven't actually tried it but this might introduce stutter/flickering when you change the cursor of a recyclerview/listview when it's currently scrolling.

In that case I guess onecould defer the changing of the cursor until the scrolling has stopped but I still would prefer to eliminate this check.

But yeah, I can totally understand if you don't consider this as important, I might just have to maintain a fork or work around it after verifying that this actually is a problem.

@JakeWharton
Copy link
Member

I didn't mean to convey that it's not a problem. It most certainly is a problem. What I meant was that we have not seen this as a problem in our usage of the library based on our usage patterns and that I was hesitant to make any decisions around solving the problem without a sample application that clearly demonstrates the problem in practice.

There have been discussions around this already. I just wanted to make sure any approach we taking to trying to solve it were not speculative in any way.

The example that you describe would make a good sample app that I would like to see contributed so that we have a proving ground for any fixes directly in the repo.

@iomu
Copy link
Author

iomu commented Feb 20, 2015

Alright, I'll try to throw something together whenever I get a bit of free time (at the earliest tuesday).

@yesitskev
Copy link

Instead of querying a table for notification updates, perhaps we could use something similar to a rest resource. This could solve iomu's and my problem (and possibly a few others). As it stands, we can requery upon tablet(s). On some apps with crazy data structures, a more granular approach would be best suited. For example, reloading only edited comments found on /blogs/2/comments.

I'm currently using a library called simplecontentprovider which does this, except that if you were now to edit the comments table straight, queries pointing towards /blogs/2/comments will not requery. This is a massive problem.

It's understandable to not implement such a mechanism if it only helps a small handful of people. I'll fork and make a working sample as soon as possible.

@JakeWharton
Copy link
Member

I'm skeptical since SQL is set based which doesn't map to a hierarchical classification well. However, this might cover a vast majority of use cases while also providing the ability to simply fall back to the current table-based approach.

I'll have to think about it more. I'd be happy to see some code in a fork or pull request which investigated this approach much more in practice.

@spark85
Copy link

spark85 commented Feb 1, 2016

Iomu, have you come up with a solution for this problem? I haven't gone through the code implemention yet, but maybe we could add a chainable method for defining operations to notify for. e.g. .on(SomeOperationType p). Jake, would this be easy enough for someone to implement on their own?

@JakeWharton
Copy link
Member

For what it's worth, 0.8.0 shipped support for triggering insert/update/execute on multiple "table"s. The reason I put table in quotes, is that you can literally use any string and then match on that from createQuery.

Frankly, this is as far as we're going to go here. SQL Delight will eventually know which insert/update/delete statements affect which other queries and it will generate custom strings for use with these methods so it will be 100% transparent to your application.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants