Add notification tags #768

nikitin-da · 2017-02-26T10:13:05Z

Closes: #663

codecov-io · 2017-02-26T10:23:52Z

Codecov Report

Merging #768 into master will increase coverage by 0.79%.
The diff coverage is 96.89%.

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage   96.05%   96.85%   +0.79%     
==========================================
  Files          87       87              
  Lines        2460     2638     +178     
  Branches      267      296      +29     
==========================================
+ Hits         2363     2555     +192     
+ Misses         68       50      -18     
- Partials       29       33       +4

Impacted Files	Coverage Δ
.../com/pushtorefresh/storio/sqlite/StorIOSQLite.java	`100% <100%> (ø)`	⬆️
...shtorefresh/storio/sqlite/queries/DeleteQuery.java	`100% <100%> (ø)`	⬆️
...qlite/operations/delete/DefaultDeleteResolver.java	`100% <100%> (ø)`	⬆️
...shtorefresh/storio/sqlite/queries/InsertQuery.java	`100% <100%> (ø)`	⬆️
...torio/sqlite/operations/put/PreparedPutObject.java	`96.87% <100%> (+3.77%)`	⬆️
...com/pushtorefresh/storio/sqlite/queries/Query.java	`100% <100%> (+2.06%)`	⬆️
.../pushtorefresh/storio/sqlite/queries/RawQuery.java	`100% <100%> (ø)`	⬆️
...pushtorefresh/storio/internal/InternalQueries.java	`100% <100%> (ø)`	⬆️
...efresh/storio/sqlite/operations/put/PutResult.java	`100% <100%> (ø)`	⬆️
.../java/com/pushtorefresh/storio/sqlite/Changes.java	`100% <100%> (ø)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2218a19...f2b48a3. Read the comment docs.

artem-zinnatullin

Few design questions, otherwise LGTM!

artem-zinnatullin · 2017-02-26T15:34:13Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/Changes.java

@@ -36,7 +62,7 @@ private Changes(@NonNull Set<String> affectedTables) {
     */
    @NonNull
    public static Changes newInstance(@NonNull Set<String> affectedTables) {
-        return new Changes(affectedTables);
+        return new Changes(affectedTables, Collections.<String>emptySet());


omg, this looks so weird after almost year of writing in kotlin style Collections.emptySet<String>(). I haven't been really writing in java for a long time now…

artem-zinnatullin · 2017-02-26T15:34:59Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/Changes.java

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Changes changes = (Changes) o;

-        return affectedTables.equals(changes.affectedTables);
+        if (!affectedTables.equals(changes.affectedTables)) return false;


Glad we have EqualsVerifier!

artem-zinnatullin · 2017-02-26T15:36:36Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/StorIOSQLite.java

@@ -144,6 +145,110 @@
    }

    /**
+     * Allows observe changes of required tags.
+     * <p/>
+     * Tags are the addition level of notifications that provides


Tags are optional meta information that you can attach to Changes object to have more fine-grained control over observing changes in the database.?

artem-zinnatullin · 2017-02-26T15:37:15Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/Changes.java

+     * Immutable set of affected tags.
+     */
+    @NonNull
+    private final Set<String> affectedTags;


Btw, what if we make Set<Object> affectedTags? Or that would be overkill?

I thought I will be less convenient for the most frequent case with Strings.
But I agree tags it is not about common cases, so flexibility should be the main purpose.
@rsinukov also votes for object.
Let's do it=)

Well, I do agree that Strings will probably be most used and we'll have to force these users to class-cast them for no good reason, let's wait for @geralt-encore opinion then!

+1 for Object. In my understanding, you can use as tag whatever you want and if String will be the most frequently used it doesn't mean that tag should be restricted to it. Also, View tag in Android is Object as well.

artem-zinnatullin · 2017-02-26T15:37:32Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/StorIOSQLite.java

+        return observeChangesOfTags(Collections.singleton(tag));
+    }
+
+


artem-zinnatullin · 2017-02-26T15:38:31Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/StorIOSQLite.java

+     * @return {@link rx.Observable} of {@link Changes} subscribed to changes of required tags.
+     */
+    @NonNull
+    public abstract Observable<Changes> observeChangesOfTablesAndTags(


I don't think we need combination methods, user can observe raw changes and filter whatever combination he/she needs (using RxJava, plain java, etc)?

If so, we should open ChangesFilter to make it visible for Query

Lets make it public and keep comment that it is for internal usage only?

I'd prefer to keep as small amount of public methods/classes/API as possible if current ones already allow you to do what you want without too much boilerplate…

+1 for removing it. It can get out of hands pretty quick if we need to introduce one more way of filtering changes. observeChangesOfTablesAndTagsAndWhatever plus combination look ugly.

artem-zinnatullin · 2017-02-26T15:38:40Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/StorIOSQLite.java

+     * @return {@link rx.Observable} of {@link Changes} subscribed to changes of required tag.
+     */
+    @NonNull
+    public Observable<Changes> observeChangesOfTableAndTag(@NonNull String table, @NonNull String tag) {


Same as above?

artem-zinnatullin · 2017-02-26T15:42:14Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/queries/UpdateQuery.java

+         * @see com.pushtorefresh.storio.sqlite.StorIOSQLite#observeChangesOfTag(String)
+         */
+        @NonNull
+        public CompleteBuilder tag(@Nullable String tag) {


tags? or overload with Set<String>? Same for other queries

artem-zinnatullin · 2017-02-26T15:43:38Z

storio-sqlite/src/test/java/com/pushtorefresh/storio/sqlite/queries/RawQueryTest.java

@@ -65,6 +65,16 @@ public void observesTablesShouldNotBeNull() {
    }

    @Test
+    public void observesTagsShouldNotBeNull() {
+        RawQuery rawQuery = RawQuery.builder()
+                .query("lalala I know SQL")


nobody knows!

I won't tell @thevery and Stasic! (If you see this message it's a GitHub's bug obviously).

geralt-encore

Nice work! 👍

geralt-encore · 2017-02-27T09:28:55Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/Changes.java

+     * Immutable set of affected tags.
+     */
+    @NonNull
+    private final Set<String> affectedTags;


+1 for Object. In my understanding, you can use as tag whatever you want and if String will be the most frequently used it doesn't mean that tag should be restricted to it. Also, View tag in Android is Object as well.

geralt-encore · 2017-02-27T09:37:12Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/Changes.java

-        return new Changes(Collections.singleton(affectedTable));
+        final Set<String> tags = affectedTag == null
+                ? Collections.<String>emptySet()
+                : singleton(affectedTag);


It makes sense to reuse static import for singleton next line or remove it completely.

geralt-encore · 2017-02-27T09:40:49Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/StorIOSQLite.java

+     * @return {@link rx.Observable} of {@link Changes} subscribed to changes of required tags.
+     */
+    @NonNull
+    public abstract Observable<Changes> observeChangesOfTablesAndTags(


+1 for removing it. It can get out of hands pretty quick if we need to introduce one more way of filtering changes. observeChangesOfTablesAndTagsAndWhatever plus combination look ugly.

geralt-encore · 2017-03-02T17:09:08Z

@nikitin-da just one comment. I think that it might be a good idea to change signature of varargs methods which are part of public API to T firstTag, T... tags instead of just T...tags since with current implementation it is possible to forget to provide any tags which lead to very obscure bugs for users.

nikitin-da · 2017-03-02T23:05:42Z

@geralt-encore good idea!
For queries it will be quite simple (if someone will decided to reuse existed builder and clear tags, he can call tags((Collection<?>) null)).
But for changes and operation results it will increase overloaded methods =(
We can't remove methods with signature like newInstance(@NonNull Set<String> affectedTables) in minor release, because it is breaking change.
And creating separate method will increase class complexity (PutResult now 8 methods ->will be 12 😿 )
@artem-zinnatullin, what is your opinion?

nikitin-da · 2017-03-04T11:00:18Z

...ite/src/main/java/com/pushtorefresh/storio/sqlite/operations/execute/PreparedExecuteSQL.java

+
+        final Observable<Object> observable;
+        if (!observesTables.isEmpty() || !observesTags.isEmpty()) {
+            observable = ChangesFilter.applyForTablesAndTags(storIOSQLite.observeChanges(), observesTables, observesTags)


@artem-zinnatullin It seems we forgot to subscribe on changes here =(

Please correct me if I'm wrong, but it was our design decision, since executeSql is basically a side effect and does not return data from database, same as we do for Put and Delete, we don't subscribe them to db changes!

Yep, I agree that it is the side effect operation and it will not be clean to subscribe on some changes with it.
But current RawQuery.observesTags possibility can be misleading.
Maybe we should note this feature in doc, because it may look as unexpected behavior =(

Yes, I definitely remember discussing that, and I'm pretty sure we have docs about it :)

I didn't find it for asRxObservable, for RawQuery and in the doc too=)

nikitin-da · 2017-03-07T05:02:20Z

@artem-zinnatullin PTAL please, to prevent guys from merge conflicts ;)

artem-zinnatullin

ExecuteSql should not observe changes in db! Otherwise LGTM, though I kinda liked idea of String tags since they're easier to use and understand cc @thevery do you have an opinion here?

artem-zinnatullin · 2017-03-13T21:35:21Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChangesFilter.java

@@ -16,28 +17,62 @@
 * <p>
 * Hides RxJava from ClassLoader via separate class.
 */
-final class ChangesFilter implements Func1<Changes, Boolean> {
+public final class ChangesFilter implements Func1<Changes, Boolean> {


We can't keep it package private?

No, because it used in com.pushtorefresh.storio.sqlite.impl for DefaultStorIOSQLite#observeChangesInTables and com.pushtorefresh.storio.sqlite.operations.execute for asRxObservable

artem-zinnatullin · 2017-03-13T21:40:03Z

...ite/src/main/java/com/pushtorefresh/storio/sqlite/operations/execute/PreparedExecuteSQL.java

+
+        final Observable<Object> observable;
+        if (!observesTables.isEmpty() || !observesTags.isEmpty()) {
+            observable = ChangesFilter.applyForTablesAndTags(storIOSQLite.observeChanges(), observesTables, observesTags)


Please correct me if I'm wrong, but it was our design decision, since executeSql is basically a side effect and does not return data from database, same as we do for Put and Delete, we don't subscribe them to db changes!

thevery · 2017-03-13T21:56:42Z

@artem-zinnatullin objects are easier to fail (hashCode/equals override) and I don't know useful examples of objects instead of strings, especially with the possibility to pass several tags.

artem-zinnatullin · 2017-03-13T22:11:28Z

Yeah, I have similar thoughts, moreover, string tags can be more understandable for those who remember how web 2.0 was growing: tag clouds on websites, tags in blog posts/articles, tags in twitter and so on. @geralt-encore?

nikitin-da · 2017-03-14T00:23:14Z

It seem there is equality of votes for now, so what type of tag will be final?)
@geralt-encore, @rsinukov, @StanisVS voted for objects 🎉 ,
@artem-zinnatullin and @thevery and I - strings ❤️

nikitin-da · 2017-03-14T00:23:29Z

There was also question about first required tag with vararg #768 (comment)
@artem-zinnatullin what is your opinion?

artem-zinnatullin · 2017-03-14T19:48:39Z

@nikitin-da I've changed emojis to ❤️ and 🎉 to remove negativity of 👎 variant haha, so please ask guys from your team to vote here so we could count here, cc @geralt-encore, I'll also ask community to vote.

Regarding overloads — I like @geralt-encore's suggestion about T tag, T… otherTags, so if that seems possible now (since we decided to remove observeChangesOfTablesAndTags() — let's implement that for methods that accept tags only. I didn't really get the note about removingnewInstance(@NonNull Set<String> affectedTables), sorry…

nikitin-da · 2017-03-18T13:44:49Z

String variant won 6:5 )

Add notification tags

456d877

nikitin-da added the enhancement || feature label Feb 26, 2017

nikitin-da added this to the v1.13.0 milestone Feb 26, 2017

nikitin-da self-assigned this Feb 26, 2017

nikitin-da requested review from artem-zinnatullin and geralt-encore February 26, 2017 10:13

artem-zinnatullin requested changes Feb 26, 2017

View reviewed changes

geralt-encore requested changes Feb 27, 2017

View reviewed changes

geralt-encore approved these changes Mar 2, 2017

View reviewed changes

nikitin-da commented Mar 4, 2017

View reviewed changes

artem-zinnatullin requested changes Mar 13, 2017

View reviewed changes

Fix comments

23312e4

nikitin-da force-pushed the issue-663 branch from ce0f381 to 23312e4 Compare March 18, 2017 13:43

Increase coverage

ad2751f

nikitin-da force-pushed the issue-663 branch from a6a133f to ad2751f Compare March 20, 2017 05:55

artem-zinnatullin approved these changes Mar 26, 2017

View reviewed changes

Merge branch 'master' into issue-663

f2b48a3

nikitin-da merged commit f6f35fe into master Mar 28, 2017

nikitin-da deleted the issue-663 branch March 28, 2017 05:46

Add notification tags #768

Add notification tags #768

Conversation

nikitin-da commented Feb 26, 2017

codecov-io commented Feb 26, 2017 • edited Loading

Codecov Report

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geralt-encore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geralt-encore commented Mar 2, 2017

nikitin-da commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitin-da commented Mar 7, 2017

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thevery commented Mar 13, 2017

artem-zinnatullin commented Mar 13, 2017

nikitin-da commented Mar 14, 2017 • edited by artem-zinnatullin Loading

nikitin-da commented Mar 14, 2017

artem-zinnatullin commented Mar 14, 2017

nikitin-da commented Mar 18, 2017 • edited Loading

codecov-io commented Feb 26, 2017 •

edited

Loading

nikitin-da commented Mar 14, 2017 •

edited by artem-zinnatullin

Loading

nikitin-da commented Mar 18, 2017 •

edited

Loading