Feature/reader search results #4096
Conversation
…ss-Android into feature/reader-search-results
…ss-Android into feature/reader-search-results
…dpress-mobile/WordPress-Android into feature/reader-search-results
…dpress-mobile/WordPress-Android into feature/reader-search-results
|
Hi @nbradbury ! I’ve been playing around with it - it’s a great tool, even addictive at times :) in any case it certainly is a long-awaited feature, so I decided to first give it a good amount of smoke testing from the user point of view before getting to the code itself. Here’s my initial feedback:
Nevertheless I went ahead and did test not only on the phone but also on genymotion with a 5.1 image and found some things worth mentioning because of the probable impact (force close level) these might have (if this is exactly what you were referring to with “weird behavior” then just skip these please):
Exception #1 android.database.StaleDataException: Exception # 2: java.lang.IllegalStateException:
https://es.search.wordpress.com/?src=organic&q=pa%C3%B1o It seems the context in which the search is made produces different results. I have tried other words in other languages (german, english, portuguese) and I can see the results are different depending on the “scope” given (i.e., using the So I think this needs a bit of research and thought maybe? As I have configured my phone for Spanish but still the Wordpress mobile app is searching within the context of the English set, to name it in a way as per my tests? (i.e. the results in my english-configured phone using the WordPress Android app are the same as when I use the I’ll take a look at the code for sure, wondering if we should check and hunt for those Cursor problems? let me know your thoughts 👍 |
I wasn't able to repro those cursor problems ( A new cursor for the suggestions adapter is created every time you type in the search box, even if you hit backspace and there wasn't anything there. So if you rapidly backspace on an empty search box fast enough, the cursor is being rapidly recreated which could let to those exceptions. This should be resolved in 6c572bc, which avoids changing the cursor when the filter hasn't changed. |
We're actually hoping to have "live" search at a later point, which would work exactly as you describe. However, at the moment the backend really isn't fast enough to make this work well, and I think it would be more annoying than useful because of this. |
|
👍 2016-05-23 12:50 GMT-03:00 Nick Bradbury notifications@github.com:
|
…dpress-mobile/WordPress-Android into feature/reader-search-results
| @@ -624,7 +635,7 @@ public static void addOrUpdatePosts(final ReaderTag tag, ReaderPostList posts) { | |||
| stmtPosts.bindString(16, post.getFeaturedImage()); | |||
| stmtPosts.bindString(17, post.getFeaturedVideo()); | |||
| stmtPosts.bindString(18, post.getPostAvatar()); | |||
| stmtPosts.bindLong (19, post.timestamp); | |||
| stmtPosts.bindDouble(19, post.timestamp); | |||
There was a problem hiding this comment.
Wondering why this attribute is declared as a double, being that elsewhere the timestamp can be held as a long. The only place I can think of is maybe forcing this is this one https://github.com/wordpress-mobile/WordPress-Android/blob/feature/reader-search-results/WordPress/src/main/java/org/wordpress/android/models/ReaderPost.java#L122 - is that maybe because the API is returning a value there that could grow into a double? if not, then we should probably leave/refactor everything to be treated as long, as per this https://developer.android.com/reference/java/sql/Timestamp.html
There was a problem hiding this comment.
Wondering why this attribute is declared as a double, being that elsewhere the timestamp can be held as a long.
So, until search came along, that timestamp column really was a timestamp, but it's only purpose has been to determine the sort order of posts.
Search results, however, include a "score" - a double which states how relevant each post is to the submitted query - and this determines how posts are sorted. To get this to work within the existing code, I changed the timestamp column to a double and store the "score" in there.
And now that I look at it, I confess to not being proud of it :)
What really should happen is for me to rename that timestamp column (and related field in the ReaderPost model) to better reflect its purpose. If that would address your (perfectly valid) complaint, I'd like to file that as a separate issue to avoid further bloating this PR.
There was a problem hiding this comment.
Definitely! Let's file that as a separate issue then and ref it here. I have to confess I doubted of the actual value of mentioning this at all but... when you write near-to-perfect code, it's easier for the reviewer to point fingers on stuff that don't really matter - LOL
There was a problem hiding this comment.
but... when you write near-to-perfect code, it's easier for the reviewer to point fingers on stuff that don't really matter
Hah! Thanks for the ego boost, but after you've been here for a while you may feel otherwise ;)
Update: About my code, that is!
|
|
Expands upon #4085 by querying the backend for search results and showing them within the main reader list (ie: within the "masterbar" reader). Changes include:
read/searchendpoint to search for postsReaderSearchServiceto perform the searchThis PR turned out to be considerably larger than I anticipated, so if any non-trivial issues are found I may move them to a separate PR to avoid inflating this one further.
Note that the design & wording still aren't final - a design review will occur at a later date, and this can be merged into the feature branch prior to that review.
Update: I meant to mention that I've seen some weird behavior with
SearchViewon Android emulators, so I recommend using a real device to test this.