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

Fixed updating app bar color #6390

Merged
merged 9 commits into from
Sep 12, 2024
Merged

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Sep 2, 2024

Closes #6374

Why is this the best possible solution? Were any other approaches considered?

It turns out that the issue is not limited to recording audio in form entries, as we initially thought and as described in the issue. It's a broader problem that can affect any screen where a scrolling view is placed directly beneath the AppBarLayout. I've detailed this in my comment:

     When scrolling, the top app bar fills with a contrasting color to create a visual separation.
     This works automatically if your scrolling view (e.g., `RecyclerView`, `ListView`) is placed directly
     beneath the `AppBarLayout`. However, if the scrolling view is nested within another view
     (such as a `ConstraintLayout`, which is common in this app), you need to help the app bar determine
     whether it should lift by setting `app:liftOnScrollTargetViewId` to the ID of the scrolling view.
     Since this `AppBarLayout` is used throughout the app with various scrolling views, it’s best to
     use a shared ID like `scrollable_container`.
     If the scrollable view is added programmatically or it is displayed in a `ViewPager` with a
     shared id, it may not work as expected anyway, and `app:liftOnScrollTargetViewId` might
     need to be updated programmatically after adding such a view.
     The `ODKView` and its `odk_view_container` or `DeleteFormsActivity` are good examples of this scenario.

To summarize, in cases of more complex views, we need to explicitly specify which view the app bar should track for scrolling, which can be done in XML, but in some instances, it needs to be handled programmatically (e.g., ODKView, ViewPager). I attempted to rework the form entry layout to prevent the scrollable container from being added dynamically each time a new screen is displayed, which would eliminate the need to set the view programmatically. However, this approach proved challenging and would require some complex and potentially messy code changes, so I decided not to proceed.

I opted for a hybrid approach, setting the view ID in XML wherever possible and handling it programmatically only when the XML solution doesn't work. Although setting IDs programmatically could be done throughout, I prefer to avoid it whenever possible, as I don't like it.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This change requires testing how the app bar color changes during scrolling. Since each screen behaves differently, we need to test all occurrences of the app bar throughout the app.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review September 3, 2024 07:41
@grzesiek2010 grzesiek2010 requested a review from seadowg September 3, 2024 07:42
<!--
When scrolling, the top app bar fills with a contrasting color to create a visual separation.
This works automatically if your scrolling view (e.g., `RecyclerView`, `ListView`) is placed directly
beneath the `AppBarLayout`. However, if the scrolling view is nested within another view
Copy link
Member

Choose a reason for hiding this comment

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

Is it "beneath" or is it if the two are children in the same CoordinatorLayout? It might be worth also linking to the docs here: https://developer.android.com/reference/com/google/android/material/appbar/AppBarLayout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it "beneath" or is it if the two are children in the same CoordinatorLayout?

Both 😄 They are children in the same CoordinatorLayout and that layout works in the way that displayed the app bar above and the scrolling view beneath. If the scrolling view is there without any wrapper everything is fine out of the box.

It might be worth also linking to the docs here: https://developer.android.com/reference/com/google/android/material/appbar/AppBarLayout

This document does not emphasize the importance of the aspect mentioned in the comment at all, so I believe it is useless.

@@ -42,6 +44,8 @@ class DeleteBlankFormFragment(
}
)
}
private var appBarLayout: AppBarLayout? = null
Copy link
Member

Choose a reason for hiding this comment

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

These fields should be removed, and the views should be accessed via OfflineMapLayersPickerBinding.bind(requireView()) (or just view in onViewCreated) so we don't leak anything after onDestroy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't do that as appBarLayout is not a part of the layout used in this fragment (the fragment is included using viewpager) and the same with the ist that should be watched. I also can't move this to onViewCreated because: #6390 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

The interaction between ViewPager2 and AppBarLayout here are really annoying! It feels like this is probably something that's missing from the MenuProvider system right now. So to sum it up: we want to set the RecyclerView child of a fragment (which is in a child fragment) as the scroll target view for an AppBarLayout when the fragment is shown in the view page right? I think we can do that generally in the onResume of MultiSelectListFragment:

override fun onResume() {
    super.onResume()
    
    val list = MultiSelectListBinding.bind(requireView()).list
    val appBarLayout =
        requireActivity().findViewById<AppBarLayout>(org.odk.collect.androidshared.R.id.appBarLayout)
    appBarLayout?.setLiftOnScrollTargetView(list)
}

We could probably make this a bit more configurable by passing in an optional app bar layout id to MultiSelectListFragment's constructor, but I think this sort of approach will work well with the "configuration over convention" choice to always have appBarLayout use the same id.

I think there's also ways we could go further to generalize it using lifecycle observers similarly to how MenuHost is implemented on ComponentActivity, but I think we can leave that to Google.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@grzesiek2010 grzesiek2010 requested a review from seadowg September 5, 2024 11:44
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

@grzesiek2010 grzesiek2010 requested review from lognaturel and seadowg and removed request for seadowg September 11, 2024 18:07
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

I've verified that the last change suggested by Callum has been addressed.

@lognaturel lognaturel dismissed seadowg’s stale review September 11, 2024 19:18

Last comments addressed

@grzesiek2010 grzesiek2010 merged commit ab2857a into getodk:master Sep 12, 2024
6 checks passed
@WKobus
Copy link

WKobus commented Sep 12, 2024

Tested with Success

Verified on device with Android 14

Verified cases

@dbemke
Copy link

dbemke commented Sep 12, 2024

Tested with Success

Verified on device with Android 10

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

Successfully merging this pull request may close these issues.

App bar flashes when audio is being played/recorded and the user scrolls down
5 participants