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

feat: Auto-hide Image Viewer toolbar #507

Merged
merged 2 commits into from Mar 10, 2024

Conversation

tinsukE
Copy link
Contributor

@tinsukE tinsukE commented Mar 6, 2024

When viewing an Image, it is not straightforward to understand that tapping on it will hide the toolbar and alt sheet, and those elements might get in the way of them viewing the full image.

Automatically hiding the toolbar and alt sheet after some short time might help the user two ways:

  1. Get out of their way when they're viewing an image with an aspect ratio so that those elements are on top of it.
  2. Showing them that those elements are hide-able, which might nudge into tapping to restore them and, consequently, learn about the "tap to hide/show" feature.

Interacting with the alt sheet cancels the auto-hide.

Fixes #505

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2024

CLA assistant check
All committers have signed the CLA.

@tinsukE
Copy link
Contributor Author

tinsukE commented Mar 6, 2024

I wanted to also cancel the auto-hide behavior in case the toolbar is interacted with. I have those changes here, but I couldn't find a way to listen to opening the overflow menu, so I refrained from adding those changes to this PR: tinsukE#1

@nikclayton
Copy link
Contributor

Thanks for the PR. Left some general comments above.

I wanted to also cancel the auto-hide behavior in case the toolbar is interacted with. I have those changes here, but I couldn't find a way to listen to opening the overflow menu, so I refrained from adding those changes to this PR: tinsukE#1

If you make the Fragment class extend from MenuProvider then do this in onViewCreated:

        requireActivity().addMenuProvider(this, viewLifecycleOwner, Lifecycle.State.RESUMED)

then onPrepareMenu will be called just before the toolbar menu is displayed (https://developer.android.com/reference/androidx/core/view/MenuProvider#onPrepareMenu(android.view.Menu)), and I think that's the right place to cancel the auto-hiding.

@tinsukE tinsukE marked this pull request as draft March 7, 2024 09:16
When viewing an Image, it is not straightforward to understand that tapping on it will hide the toolbar and alt sheet, and those elements might get in the way of them viewing the full image.

Automatically hiding the toolbar and alt sheet after some short time might help the user two ways:
1. Get out of their way when they're viewing an image with an aspect ratio so that those elements are on top of it.
2. Showing them that those elements are hide-able, which might nudge into tapping to restore them and, consequently, learn about the "tap to hide/show" feature.
@tinsukE tinsukE marked this pull request as ready for review March 7, 2024 10:13
@tinsukE
Copy link
Contributor Author

tinsukE commented Mar 7, 2024

@nikclayton I have updated the PR according to the discussions.

@tinsukE
Copy link
Contributor Author

tinsukE commented Mar 7, 2024

If you make the Fragment class extend from MenuProvider then do this in onViewCreated:

        requireActivity().addMenuProvider(this, viewLifecycleOwner, Lifecycle.State.RESUMED)

then onPrepareMenu will be called just before the toolbar menu is displayed (https://developer.android.com/reference/androidx/core/view/MenuProvider#onPrepareMenu(android.view.Menu)), and I think that's the right place to cancel the auto-hiding.

@nikclayton Thanks for the tip! I found that MenuProvider#onPrepareMenu(android.view.Menu) is also called when the Toolbar is being setup, and I wouldn't want to cancel the auto-hiding then, but I managed a workaround creative solution for it.

Let me know if If you'd prefer to leave that change as a second PR for when this is merged, or if I should batch them together.

@nikclayton
Copy link
Contributor

Let me know if If you'd prefer to leave that change as a second PR for when this is merged, or if I should batch them together.

I'll get this merged (so it can go out in the next Pachli Current) and if you can do the followup in a new PR that'd be great. Thanks for the PR!

@nikclayton nikclayton merged commit bdf2d93 into pachli:main Mar 10, 2024
6 checks passed
@tinsukE tinsukE deleted the 505-hide-image-toolbar branch March 11, 2024 18:48
nikclayton added a commit that referenced this pull request Mar 13, 2024
Cancel the auto-hide behavior in case the toolbar menu items are
interacted with.

Improves #505,
#507

---------

Co-authored-by: Nik Clayton <nik@ngo.org.uk>
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.

Image Viewer toolbar doesn't automatically hide
3 participants