-
Notifications
You must be signed in to change notification settings - Fork 1
Search #21
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
base: master
Are you sure you want to change the base?
Search #21
Conversation
* missing parser fixes (more than 999 results)
|
Great to see work on this big one! Unfortunately, I currently do not have much spare time for a proper review now. I hope to have a detailed look until next week. Just two points based on a quick look:
|
* translates SearchableActivity.java to SearchableActivity.kt
|
Regarding case sensitivity: the goal was to imitate the blogs |
|
I still did not have to time for a closer look. Maybe I understood your solution wrong. I thought about searching the list of stored post (since we currently have no way of accessing older posts). So it is simply a change of the SQL query to search case-insensitive. |
|
The proposed solution searches the blog and stores results in the app db, from where they are then fetched and displayed by the result list view. |
|
Sorry for letting you wait so long with a proper review. It took way longer than expected. I see, I got the idea wrong. That makes it of course impossible to query case insensitive. To be honest, the case sensitive thing that always broke my neck when searching for a specific historic post. Therefore, my idea when writing the issue was to keep everything local so you can have a quick search while you type functionality. Nevertheless, I am in principle fine with a online search as you already put the work into it. What however makes me a bit wonder whether we actually should implement it like this is issue #19. It is something that was already often requested and even noted by reviews in the Play Store. My idea of tackling this issue was to fetch older post month by month (as one can fetch them this way) if the reader hits the bottom of the list. Unfortunately, this would not be possible anymore since storing those post from the search result in the DB makes it way harder if not impossible to actually detect which months are fully stored locally. Do you have any ideas how to tackle this problem? |
| import java.util.concurrent.Executor | ||
| import java.util.concurrent.Executors | ||
|
|
||
| class SearchableActivity : PostListActivity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The activity has no back arrow in the bar. I would like to keep it consistent with the DetailsActivity.
| } | ||
| val query = intent.getStringExtra(SearchManager.QUERY) ?: throw RuntimeException() | ||
|
|
||
| title = String.format("Suchergebnisse: %s", query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move static text to translatable strings
| <item | ||
| android:id="@+id/menu_unread_filter" | ||
| android:title="@string/menu_unread_filter" | ||
| android:icon="@drawable/ic_unread" | ||
| app:showAsAction="ifRoom"/> | ||
| app:showAsAction="never"/> | ||
|
|
||
| <item | ||
| android:id="@+id/menu_bookmark_filter" | ||
| android:title="@string/menu_bookmark_filter" | ||
| android:icon="@drawable/ic_bookmark" | ||
| app:showAsAction="ifRoom"/> | ||
| app:showAsAction="never"/> | ||
|
|
||
| <item | ||
| android:id="@+id/menu_mark_read" | ||
| android:title="@string/menu_mark_read" | ||
| android:icon="@drawable/ic_mark_read" | ||
| app:showAsAction="ifRoom"/> | ||
| app:showAsAction="never"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep the ifRoom so the menu is not so cluttered if there is room?
| android:title="@string/menu_search" | ||
| android:icon="@drawable/ic_baseline_search_24" | ||
| app:showAsAction="ifRoom" | ||
| app:actionViewClass="android.widget.SearchView"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know whether it is possible to hide the activity name when the search expands? It looks a bit odd as it kind of cuts the name. So far I have not seen such a behavior in other apps (at least I did not notice).
|
Hi @hubwoop, do you have any ideas on the issue? |
|
HI @tbolender, thank you for the review :)
What do you think about adding a flag to the database entry for each post that would identify it as fetched via search query? |
|
That sounds like a great idea! This way, we could filter the stored search results in the full list view until we have full past entries fetching. |
Some example queries:
"volfefe": 1 result
"covfefe": 4 results
"Zwei dinge": 2 results