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.

Sign up for GitHub

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

Collapse browser toolbar on scrolling the card list #18037

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Collapse browser toolbar on scrolling the card list #18037

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 47 additions & 32 deletions AnkiDroid/src/main/res/layout/card_browser.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,46 +6,61 @@
android:layout_height="match_parent"
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
xmlns:app="http://schemas.android.com/apk/res-auto"
>

<LinearLayout
<com.google.android.material.appbar.AppBarLayout
android:background="@null"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical" >

<include layout="@layout/toolbar" />
android:layout_height="wrap_content"
>

<!-- Note: AppBarLayout itself is a vertical LinearLayout, so strictly speaking
Copy link
Member

@lukstbit lukstbit Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this comment is needed(maybe in the commit message).
We shouldn't look into optimizing by tying to specific implementations. We already had several crashes due to using specific LayoutParams(although a more general parent was available) and then changing the layout structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this information as a comment as having a vertical LinearLayout as a child of AppBarLayout is weird, and whoever reads the code later on will eventually wonder why it is here. While this could be in a commit message, it takes time and effort to go through the blame to find anything useful, and more often than not the explanation just isn't there.

(As per the tutorial, you would be setting app:layout_scrollFlags on the individual children of AppBarLayout.)

here it is not necessary to have another LinearLayout as a child.
Instead we could be setting app:layout_scrollFlags on the individual children.
However, the <include> tag can only override a very limited set of arguments,
so the alternatives here would be not using the <include> tag,
Copy link
Member

@BrayanDSO BrayanDSO Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

my two cents: the alternative of not using <include> is super reasonable. Most of the new screens don't use it, if that matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick search says there are 15 files saying <include layout="@layout/toolbar" in the XML

Copy link
Member

Choose a reason for hiding this comment

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

Most of the new screens don't use it

A quick search says there are 15 files saying <include layout="@layout/toolbar" in the XML

Yes, it is still used in some places, but most of the "new" ones use MaterialToolbar instead of including the toolbar.xml layout, which has some extra sutff that normally is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. I just went this way for the self-contained minimal change, otherwise I personally have no opinion on which way is the best one. 🤷‍♀️

or having app:layout_scrollFlags directly in the included layout. -->
<LinearLayout
android:id="@+id/browser_column_headings"
android:layout_width="match_parent"
android:background="@drawable/browser_heading_bottom_divider"
android:orientation="horizontal"
android:paddingVertical="2dp"
android:layout_height="wrap_content"
android:longClickable="true">

</LinearLayout>
android:layout_height="match_parent"
android:orientation="vertical"
app:layout_scrollFlags="scroll|enterAlways"
>

<com.google.android.material.progressindicator.LinearProgressIndicator
android:id="@+id/browser_progress"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:indeterminate="true"
android:visibility="gone"
/>
<include layout="@layout/toolbar" />

<androidx.recyclerview.widget.RecyclerView
android:id="@+id/card_browser_list"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="?android:attr/colorBackground"
android:overScrollFooter="@color/transparent"
android:clipToPadding="false"
android:paddingBottom="72dp"
android:drawSelectorOnTop="true"
tools:listitem="@layout/card_item_browser"
/>
<LinearLayout
android:id="@+id/browser_column_headings"
android:layout_width="match_parent"
android:background="@drawable/browser_heading_bottom_divider"
android:orientation="horizontal"
android:paddingVertical="2dp"
android:layout_height="wrap_content"
android:longClickable="true"
/>
</LinearLayout>
</com.google.android.material.appbar.AppBarLayout>

</LinearLayout>
<androidx.recyclerview.widget.RecyclerView
android:id="@+id/card_browser_list"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="?android:attr/colorBackground"
android:overScrollFooter="@color/transparent"
android:clipToPadding="false"
android:paddingBottom="72dp"
android:drawSelectorOnTop="true"
tools:listitem="@layout/card_item_browser"
app:layout_behavior="@string/appbar_scrolling_view_behavior"
/>

<com.google.android.material.progressindicator.LinearProgressIndicator
android:id="@+id/browser_progress"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:indeterminate="true"
android:visibility="gone"
android:layout_gravity="bottom"
/>
</androidx.coordinatorlayout.widget.CoordinatorLayout>
Loading