Skip to content
Snippets Groups Projects

Extend merge request !310

1 unresolved thread

This changeset consists of 20 commits.

The first seven address issues discussed in merge request !310 (closed). These issues improve the UI changes for the NowPlayingBottomSheet. These changes are:

  • 781152d0 - Suppress BottomSheet close on show.
  • b2701a2a - Prevent BottomSheet tap leaking to nav panels.
  • 4c1ba726 - Give notification tap action semantic meaning.
  • d4f5fe1f - Show BottomSheet upon notification tap.
  • f50c513d - Increase player controls touchpoint size.
  • 08df4d4c - Persist BottomSheet open/close state.
  • c1e4e891 - Remove redundant LinearLayout.

The next three commits fix crashes when switching between portrait and landscape views. The concern for these crashes were raised in this comment from !310 (closed). The crashes are further described in issue #145 (closed). These changes are:

  • 752542fb - Do not create unnecessary Picasso objects.
  • 2708f67a - Refactor CoverArt.withContext().
  • 4ca60bec - Fix landscape view induced MainActivity leak.

The final ten commits clean up warnings reported by AndroidStudio. These are not stability or usability issues, but most stem from changes made in !310 (closed) and this merge request. These changes are:

  • ba5574dd - Cleanup: remove unused imports.
  • fabd78de - Cleanup: remove unused function.
  • 8d69fcc1 - Cleanup: mark internal fields private.
  • 3bbdd3f9 - Cleanup: minor tweaks suggested by AndroidStudio.
  • 1b197626 - Cleanup: do not create unused field.
  • 09b6a27e - Cleanup: Avoid shadowing "it" object.
  • 8890e3c9 - Cleanup: "cascading if" -> "when".
  • b263aa78 - Cleanup: use ContextCompat to load drawables.
  • d949f9fe - Cleanup: SuppressLint adjustments.
  • 96edf761 - hdasch/bottom-sheet-refinement Add a changelog entry.

With these changes, I believe !310 (closed) deserves review and further testing. It should be considered a merge candidate.

Merge request reports

Checking pipeline status.

Approval is optional

Closed by Hugh DaschbachHugh Daschbach 1 year ago (Nov 5, 2023 6:31pm UTC)

Merge details

  • The changes were not merged into develop.
  • Auto-merge enabled

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Hugh Daschbach mentioned in merge request !310 (closed)

    mentioned in merge request !310 (closed)

  • Hugh Daschbach added 13 commits

    added 13 commits

    • 96edf761...3ca45e0b - 3 earlier commits
    • 9b024cc2 - Cleanup: remove unused imports.
    • f84fff01 - Cleanup: remove unused function.
    • fbb6969e - Cleanup: mark internal fields private.
    • 477b2e26 - Cleanup: minor tweaks suggested by AndroidStudio.
    • 4582aa01 - Cleanup: do not create unused field.
    • faecaecf - Cleanup: Avoid shadowing "it" object.
    • d920cb97 - Cleanup: "cascading if" -> "when".
    • 65fa7226 - Cleanup: use ContextCompat to load drawables.
    • 24670282 - Cleanup: SuppressLint adjustments.
    • 254af232 - Add a changelog entry.

    Compare with previous version

  • Author Contributor

    I have removed commit:

    • c1e4e891 - Remove redundant LinearLayout.

    from the chageset, as it introduced a regression. Without the nested LinearLayout selecting an artist from the "Artists" nav panel presented a view with cover art, but not a list of albums. Similarly, selecting an album from the "Albums" nav panel displayed cover art, but not a track list.

  • Hi,

    We're posting this message because this merge request meets the following criteria:

    • No activity in the past 60 days (since 2023-08-30T05:37:17.165Z)

    We'd like to ask you to help us out and determine how we should act on this merge request:

    • is it still a WIP?
    • is it still relevant?
    • is it missing some reviews?
    • do you need help or guidance to proceed?

    Some community members have been pinged and will have a look at it too.

    Thanks for your help!

    /cc @funkwhale/steering

  • Hi,

    We're posting this message because this merge request meets the following criteria:

    • No activity in the past 60 days (since 2023-08-30T05:37:17.165Z)

    We'd like to ask you to help us out and determine how we should act on this merge request:

    • is it still a WIP?
    • is it still relevant?
    • is it missing some reviews?
    • do you need help or guidance to proceed?

    Some community members have been pinged and will have a look at it too.

    Thanks for your help!

    /cc @funkwhale/steering

  • Hugh Daschbach added 21 commits

    added 21 commits

    • 254af232...6472a374 - 9 commits from branch funkwhale:develop
    • 6472a374...fbbd9011 - 2 earlier commits
    • 822adcac - Fix overlap between main fragment and player bottom bar
    • b924a0c6 - Fix bottom sheet being hidden in certain conditions
    • 056e3a4d - Use MotionLayout to animate bottom sheet opening
    • 1a050c2d - Fixes form peer review
    • 31908b61 - Fix buffering progress bar display
    • c86f81b4 - Do not create unnecessary Picasso objects.
    • 9cd8826e - Refactor CoverArt.withContext().
    • ab3205c0 - Prevent BottomSheet tap leaking to nav panels.
    • eaf3c550 - Increase player controls touchpoint size.
    • b2490c61 - Fix landscape view induced MainActivity leak.

    Compare with previous version

    • Author Contributor

      I have pushed an update consistent with the current state of !310 (closed). In doing so, I have reduced the scope of changes. There are now just five commits addressing four issues:

      • c86f81b4 Do not create unnecessary Picasso objects.
      • 9cd8826e Refactor CoverArt.withContext().
      • ab3205c0 Prevent BottomSheet tap leaking to nav panels.
      • eaf3c550 Increase player controls touchpoint size.
      • b2490c61 Fix landscape view induced MainActivity leak.

      The first two (c86f81b4 and 9cd8826e) fix the "Too many resources" crash.

      The next two (ab3205c0 and eaf3c550) address minor UX issues.

      The last (b2490c61) fixes a memory leak that crashes the app after ten or so orientation changes when landscape mode is enabled. Testing with landscape enabled uncovers issues not seen with the previous version of !310 (closed), described below, so it is left disabled.

      In the previous version of !310 (closed), rotating between portrait and landscape modes worked well, modulo a couple crashes that are fixed in this changeset. However, currently, when the NowPlayingBottomSheet is displayed, changing configuration between portrait and landscape orientations closes the NowPlayingBottomSheet. That seems clumsy, so landscape mode is left disabled.

    • Thanks! Would you mind pushing the branch into the upstream repo and create a new Merge Request so we can see the pipeline run?

    • Author Contributor

      I tried. I do not seem to have access:

      remote: ========================================================================
      remote:
      remote: You are not allowed to push code to this project.
      remote:
      remote: ========================================================================
      remote:
      fatal: Could not read from remote repository.
      
      Please make sure you have the correct access rights
      and the repository exists.

      Or perhaps I do not have the correct URL.

      Is this the correct push URL? git@dev.funkwhale.audio:funkwhale/funkwhale-android.git

    • @hdasch Sorry, I messed up the rights management. You should be good to go now!

    • Author Contributor

      Done.

    • Please register or sign in to reply
  • Author Contributor

    Superseded by !335 (merged).

  • closed

  • Hugh Daschbach resolved all threads

    resolved all threads

Please register or sign in to reply
Loading