Improve search results #1327

pandasoft0 · 2019-06-18T06:35:42Z

connyduck

The tabs are blue with white font for me, should be Backgroundcolor + textcolor, blue only for selected tab
I like the setup with the SearchFragment that handles common functionality
it should probably not be the SearchFragment that inherits from SFragment, but only the SearchStatusFragment?
The error handling needs some improvement. The snackbar only retries one of the requests, but i feel like it should retry all 3. Maybe the sad elephant fullscreen is be a better way to show the error?

connyduck · 2019-06-21T13:52:54Z

app/src/main/java/com/keylesspalace/tusky/interfaces/AnchorActivity.kt

+
+import android.view.View
+
+interface AnchorActivity {


Without comments its absolutely unclear what this interface is doing. I think you can remove it, Snackbar should also work with another view.

It worked not correct because activity has a scrollable toolbar and fragments have CoordinatorLayout as root. When app searches the layout for snackbar it founds the fragment's CoordinatorLayout. But root CoordinatorLayout hides the bottom of the fragment (with Snackbar) when toolbar shown. I've removed child CoordinatorLayout, so, Snackbar always uses the Activity's CoordinatorLayout

app/src/main/res/layout/activity_search2.xml

connyduck · 2019-06-21T13:57:38Z

app/src/main/java/com/keylesspalace/tusky/entity/HashTag.kt

+data class HashTag(
+
+        @field:SerializedName("name")
+        val name: String,


@SerializedName is not necessary when the property has the same name

We must not forget to add this file to proguard rules in this case. I prefer to set the SerializedName and do not worry about proguard.

app/src/main/java/com/keylesspalace/tusky/entity/HashTag.kt

app/src/main/java/com/keylesspalace/tusky/di/NetworkModule.kt

connyduck · 2019-06-21T14:00:52Z

app/src/main/java/com/keylesspalace/tusky/components/search/fragments/SearchStatusesFragment.kt

+    }
+
+    override fun onReblog(reblog: Boolean, position: Int) {
+        //TODO("not implemented") //To change body of created functions use File | Settings | File Templates.


Don't forget to implement these methods

Implemented. I decided to do a trick.
ViewModel stores loaded statuses at the separate list.
When user update the status - app updates the status at this list and invalidate DataSource. DataSource uses this precached list as initial load data. When user changes the search phrase, app clear this list and app uses the network request to load data.
Not sure that this is the best solution, but it allow to use standard paging libraries.

app/src/main/java/com/keylesspalace/tusky/components/search/SearchViewModel.kt

pandasoft0 · 2019-06-26T15:33:17Z

Tabs style updated
I left the SFragment because all 3 fragments has the similar setup and functionality. Yes, i thought about something like a
SearchBasicFragment<T>: Fragment(), Injectable, LinkListener{}
for a Hashtag and Account fragments, and SFragment for SearchStatusFragment, but most code will be the same and from another side - SFragment is lightweight.
As another solution - i think we may not to use the SFragment at all, but copy-paste some methods from SFragment to a SearchStatusFragment: SearchBasicFragment<...>(), StatusActionListener. Please, let me know if this is a better solution and i will implement it. It's not a problem.

pandasoft0 · 2019-06-26T15:43:39Z

About error handling.
Search fragment may show the error not only on initial load but during a scroll also.
Also, it calls 3 different search request for every page.
Yes, we may show the elephant for initial load error, but i prefer to show it per fragment. And refresh content only for that fragment where user clicked the refresh button. So, app may return success on first requests, but error on load next data request for statuses and we don't need to refresh all pages because data on other pages are valid. Also, we must not repeat all 3 requests even if initial request failed for all 3 pages. If user wants to find a hashtag and search failed, he will click repeat on hashtag page and app will do a single request but will not to do 2 other unneeded requests...
But if you want to repeat all failed requests - not a problem for me. I will do.

connyduck · 2019-06-29T17:07:37Z

app/src/main/java/com/keylesspalace/tusky/components/search/adapter/SearchAccountsAdapter.kt

+
+    public override fun getItem(position: Int): Account? {
+        return super.getItem(position)
+    }


this override is unnecessary, it just calls super

connyduck

This is really well done, nice work.

Can you give the tabs the same color as the search bar please (in all themes)? I do not care if you make the tabs the color of the bar or the other way.
there is still a problem with error handling, retrying does not work for me sometimes. Seems like all snackbars just reload the first tab or something like that. I can send you a video if you want.
(optional) pull to refresh would actually be really nice inside the tabs

connyduck · 2019-06-29T17:10:13Z

app/src/main/java/com/keylesspalace/tusky/components/search/adapter/SearchHashtagsAdapter.kt

+
+    public override fun getItem(position: Int): HashTag? {
+        return super.getItem(position)
+    }


unnecessary override

connyduck · 2019-06-29T17:10:47Z

app/src/main/java/com/keylesspalace/tusky/components/search/adapter/SearchStatusesAdapter.kt

+
+    public override fun getItem(position: Int): Pair<Status, StatusViewData.Concrete>? {
+        return super.getItem(position)
+    }


unnecessary overrride

This one is required in order to change visibility of the method from protected to public , used at:

SearchStatusesFragment.kt:78,

SearchStatusesFragment.kt:84,

SearchStatusesFragment.kt:90,

SearchStatusesFragment.kt:96.

connyduck · 2019-06-29T17:16:45Z

app/src/main/java/com/keylesspalace/tusky/entity/HashTag.kt

+import com.google.gson.annotations.SerializedName
+
+data class HashTag(
+        @field:SerializedName("name")


SerializedName does nothing if the property has the same name anyway.

connyduck · 2019-06-29T17:17:35Z

app/src/main/java/com/keylesspalace/tusky/components/search/fragments/SearchFragment.kt

+
+    }
+
+    protected fun showNoData(isEmpty: Boolean) {


can be private

connyduck · 2019-06-29T17:19:26Z

app/src/main/java/com/keylesspalace/tusky/components/search/fragments/SearchFragment.kt

+            if (it == NetworkState.LOADING) {
+                searchProgressBar.show()
+            } else
+                searchProgressBar.hide()


there is a extension function visible so you can do these checks shorter
searchProgressBar.visible(it == NetworkState.LOADING)

app/src/main/java/com/keylesspalace/tusky/components/search/fragments/SearchStatusesFragment.kt

connyduck · 2019-06-29T17:22:25Z

app/src/main/java/com/keylesspalace/tusky/components/search/fragments/SearchStatusesFragment.kt

+
+        val loggedInAccountId = viewModel.activeAccount?.accountId
+
+        val popup = PopupMenu(context!!, view)


view.context is always non-null

connyduck · 2019-06-29T17:23:25Z

app/src/main/java/com/keylesspalace/tusky/components/search/fragments/SearchStatusesFragment.kt

+        if (activity == null) {
+            return
+        }
+        AlertDialog.Builder(activity!!)


activity?.let{ makes this shorter and you don't have to use !!

noln · 2019-07-05T19:24:37Z

@connyduck could you provide the video showing the weird retry behaviour that you mention above please? I've not been able to replicate it.

connyduck · 2019-07-07T12:18:05Z

Do you have a new GitHub account?

connyduck · 2019-07-07T12:40:19Z

Here is a video https://connyduck.at/device-2019-07-07-143455.mp4
Notice how only the first tab shows data after going back into WiFi and clicking retry.

noln · 2019-07-08T11:21:18Z

@connyduck Nope, different person. I'm leading on Roma for Android now, so aiming to tidy up and close out any remnant PRs from the fork before starting any new ones, and thanks for the vid!

…agments/SearchStatusesFragment.kt Co-Authored-By: Konrad Pozniak <connyduck@users.noreply.github.com>

noln · 2019-07-16T23:21:39Z

@connyduck

Themes and colours are now showing as requested.
That retry bug is fixed too.
I've added pull-to-refresh for each tab too.

One thing to note is that searching hashtags results in infinite paging, full of the same result, which looks like a bug. However that's actually what the V2 API returns from server-side when the offset
parameter is changed, so the client-side code is implemented correctly. Therefore the issue will go away when the server returns the correct result.

I think that satisfies all of the prior comments, but would welcome further review and feedback.

Update: Oh and it's no longer WIP, but I don't own the PR so can't rename it. I didn't want to create a new one and lose the review conversation.

connyduck

Looks good now, thx. Awesome work.

Just one more thing from a UX perspective: What if there are no results on the first tab? Should we somehow indicate that there is more on other tabs? Maybe don't display tabs that have no results? Or reorder them?

I think I would go with merging this and asking users what they think?

noln · 2019-07-19T17:53:21Z

Good point, hadn't thought of that! A few approaches come to mind:

Show a couple of tappable TextViews that tell the user that there are other results on the other tabs, which takes the user straight there on tap.
Snackbar to do the same as above.
Update the tab titles to show the number of results returned, or "99+" for large numbers of results.

User feedback would definitely be a good guide for that; could even make it switchable behaviour.

connyduck · 2019-07-19T18:10:14Z

I merge this now so more people can try it out, lets see.

Update the tab titles to show the number of results returned

Would be nice, but I think we don't get this info from the api?

Thx again

connyduck · 2019-07-19T18:16:22Z

Oh no, found another bug: The account list does not stop loading and shows the same account over and over again:

@noln

noln · 2019-07-19T18:28:59Z

@connyduck 😵

Looking at that, it seems that it's instance-specific so perhaps a server-side issue, as it doesn't occur on the (admittedly Pleroma) instances I've tried with the head of master.

Could you raise an issue against me for that, with details of which instance version you're using please?

connyduck · 2019-07-19T19:03:24Z

Ok, i tried debugging this and it looks like it is a bug in Mastodon and only happens when searching for ones own account 🤣
Like, I can send in offset=100 and will still get back my own account

I will open a ticket on their repo

connyduck · 2019-07-19T19:23:01Z

mastodon/mastodon#11365

connyduck added this to the Tusky 9 milestone Jun 21, 2019

connyduck requested changes Jun 21, 2019

View reviewed changes

connyduck mentioned this pull request Jun 24, 2019

Excessive amount of profiles in search results on Pleroma instances #1350

Closed

1 task

pandasoft0 force-pushed the upstream/1081/improve_search_results branch from 79eced7 to 77d610a Compare June 26, 2019 10:26

connyduck reviewed Jun 29, 2019

View reviewed changes

connyduck requested changes Jun 29, 2019

View reviewed changes

noln force-pushed the upstream/1081/improve_search_results branch from 7f51a3c to 8c5b21a Compare July 5, 2019 18:31

joelpyska and others added 14 commits July 16, 2019 19:00

Add entities and request for search APIv2

c8281c8

Implement search adapter and fragment

c7e11d5

Fix issue with snackbar

eb6512a

Implement search accounts fragment

bdf1412

Implement generic search fragment

e5ebec2

Remove unneeded import

0a7def3

Implement "status" actions, fix issues

a4b175a

Remove SFragment dependency

68feaeb

Update app/src/main/java/com/keylesspalace/tusky/components/search/fr…

84259b6

…agments/SearchStatusesFragment.kt Co-Authored-By: Konrad Pozniak <connyduck@users.noreply.github.com>

Clean-up post review suggestions

5e95ac7

Make TabLayout background colour match search bar

40fef6b

Corrected method call syntax

414308b

Added SwipeRefreshLayout to SearchFragment

7b04088

Fixed refresh to update all three tabs

635e9a4

noln force-pushed the upstream/1081/improve_search_results branch from e9972dd to 635e9a4 Compare July 16, 2019 23:04

connyduck changed the title ~~[WIP] Improve search results~~ Improve search results Jul 19, 2019

connyduck approved these changes Jul 19, 2019

View reviewed changes

connyduck merged commit 3b1288e into tuskyapp:master Jul 19, 2019

connyduck mentioned this pull request Sep 3, 2019

Improve our search results #1081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve search results #1327

Improve search results #1327

pandasoft0 commented Jun 18, 2019

connyduck left a comment

connyduck Jun 21, 2019

pandasoft0 Jun 26, 2019

connyduck Jun 21, 2019

pandasoft0 Jun 26, 2019

connyduck Jun 21, 2019

pandasoft0 Jun 26, 2019

pandasoft0 commented Jun 26, 2019 •

edited

Loading

pandasoft0 commented Jun 26, 2019

connyduck Jun 29, 2019

connyduck left a comment

connyduck Jun 29, 2019

connyduck Jun 29, 2019

noln Jul 5, 2019

connyduck Jun 29, 2019

connyduck Jun 29, 2019

connyduck Jun 29, 2019

connyduck Jun 29, 2019

connyduck Jun 29, 2019

noln commented Jul 5, 2019

connyduck commented Jul 7, 2019

connyduck commented Jul 7, 2019

noln commented Jul 8, 2019 •

edited

Loading

noln commented Jul 16, 2019 •

edited

Loading

connyduck left a comment

noln commented Jul 19, 2019 •

edited

Loading

connyduck commented Jul 19, 2019

connyduck commented Jul 19, 2019

noln commented Jul 19, 2019

connyduck commented Jul 19, 2019 •

edited

Loading

connyduck commented Jul 19, 2019


		val loggedInAccountId = viewModel.activeAccount?.accountId

		val popup = PopupMenu(context!!, view)

Improve search results #1327

Improve search results #1327

Conversation

pandasoft0 commented Jun 18, 2019

connyduck 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

pandasoft0 commented Jun 26, 2019 • edited Loading

pandasoft0 commented Jun 26, 2019

Choose a reason for hiding this comment

connyduck 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

noln commented Jul 5, 2019

connyduck commented Jul 7, 2019

connyduck commented Jul 7, 2019

noln commented Jul 8, 2019 • edited Loading

noln commented Jul 16, 2019 • edited Loading

connyduck left a comment

Choose a reason for hiding this comment

noln commented Jul 19, 2019 • edited Loading

connyduck commented Jul 19, 2019

connyduck commented Jul 19, 2019

noln commented Jul 19, 2019

connyduck commented Jul 19, 2019 • edited Loading

connyduck commented Jul 19, 2019

pandasoft0 commented Jun 26, 2019 •

edited

Loading

noln commented Jul 8, 2019 •

edited

Loading

noln commented Jul 16, 2019 •

edited

Loading

noln commented Jul 19, 2019 •

edited

Loading

connyduck commented Jul 19, 2019 •

edited

Loading