Skip to content
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

Search in Library sections #1566

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

felipeerias
Copy link
Collaborator

Add a search bar for Bookmarks, History, and Downloads.

Each of these views keeps a cached version of the list of items. When the user enters a search term, we filter this cached list and update the Adapter. This avoids having to query the backend each time.

Currently this implementation updates the search results as the user types each character, but if this does not have good performance we can change it later on.

There is a change to the OnScrollListener to ensure that the RecyclerView only requests the focus when it scrolls as a result of user interaction, which will close the on-screen keyboard.

This PR also contains a couple of related changes to our RecyclerView lists.

First, we set the overScrollMode to never by default and remove those instances where it had been manually set. This removes the bouncing rubber band effect when scrolling the list.

Secondly, we remove CustomLinearLayoutManager, a workaround for an old bug in the RecyclerView library which is no longer necessary.

Add a search bar for Bookmarks, History, and Downloads.

Each view keeps a cached version of the list of items. When the
user enters a search term, we filter this cached list and update
the Adapter. This avoids having to query the backend each time.

Currently this implementation updates the search results as the
user types each character, but if this does not have good performance
we can change it later on.

There is a change to the OnScrollListener to ensure that the
RecyclerView only requests the focus when it scrolls as a result
of user interaction, which will close the on-screen keyboard.
Set the "overScrollMode" to "never" by default and remove those instances
where it had been manually set.
Remove CustomLinearLayoutManager, a workaround for an old bug
in the RecyclerView library and which is no longer necessary.
@@ -122,6 +123,22 @@ public void onDestroy() {
mBinding.downloadsList.removeOnScrollListener(mScrollListener);
}

@Override
public boolean supportsSearch() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repetition of the same pattern in all the views makes me wonder why we don't move everything to the LibraryView class.

@@ -56,7 +56,6 @@
android:layout_marginEnd="20dp"
android:divider="@android:color/transparent"
android:smoothScrollbar="true"
android:overScrollMode="never"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question here is why. There is no explanation why we are doing that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always nice to remove code, but can we add a reference to the old bug that is no longer needed? Or a reference to the PR that added it, we need traceability here.

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

Successfully merging this pull request may close these issues.

2 participants