Skip to content

Added overflow menu to open card image in image gallery#3219

Open
KamiDaga wants to merge 4 commits into
CatimaLoyalty:mainfrom
KamiDaga:feature/overflow-menu-to-image-gallery
Open

Added overflow menu to open card image in image gallery#3219
KamiDaga wants to merge 4 commits into
CatimaLoyalty:mainfrom
KamiDaga:feature/overflow-menu-to-image-gallery

Conversation

@KamiDaga

Copy link
Copy Markdown
Contributor

Hello!
This PR contains changes to fix #3209.

My idea was to change the TopAppBar utilized by the card detail activity with another one containing the overflow menu, which takes in input the image simple path defined by the Utils class method, and creates a readable URI to the image. This URI is then used as the input of the Intent that will eventually open the image gallery app.

I ran the following commands to ensure nothing broke on the surface:

  • $ ./gradlew testFossDebugUnitTest
  • $ ./gradlew lintFossRelease
  • $ ./gradlew testGplayDebugUnitTest
  • $ ./gradlew lintGplayRelease

In case any log is needed, please let me know!

After executing these commands, I manually checked that the behaviour is the desired one with a Oneplus Nord 3 and with the embedded Android Studio emulator.

@TheLastProject

Copy link
Copy Markdown
Member

Thanks for trying. I sadly am quite busy lately so I don't really have much time to guide people in general, so don't take this personally please. But basically this code isn't really usable and needs quite some changes.

The main problems are:

  • Lack of DRY ("Don't Repeat Yourself"): The CatimaTopAppBarWithOverflowMenuToImageGallery is basically a full copy of CatimaTopAppBar with some extra features. It would be much better for CatimaTopAppBar to be more flexible (for example: by making it possible to pass some actions) instead of having 2 functions doing 90% the same thing as the last is very hard to maintain and won't scale.
  • Your indenting is a bit all over the place and not very consistent with the rest of the codebase :)
  • Passing path in 3 places is a bit silly, as we can just calculate it in the one place it matters

@KamiDaga

Copy link
Copy Markdown
Contributor Author

Hi! Thank you so much for answering, I never take personally good and constructive feedback.

Is it okay if I try again by fixing the problems you underlined?

@TheLastProject

Copy link
Copy Markdown
Member

Sure that's fine, sorry for closing it so quickly yesterday I was having kind of a bad day :) If you have the patience to keep trying I don't mind doing a few review rounds :)

@KamiDaga

KamiDaga commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the extra chance!

This time my approach was a little different as I tried to fix the main problems:

  • CatimaTopAppBar now receives another optional parameter overflowMenuActions which represents a set of entries and parameters for these entries. The idea is to support future overflow entries by creating new enum values in OverflowMenuEntry and OverflowMenuParameter, and then manage them wherever needed. With this approach, it is also possible to just show specific entries and not all of them;
  • CatimaTopAppBarWithOverflowMenuToImageGallery has been removed and its menu creation logic has been moved to CatimaTopAppBar with a more generalistic approach;
  • Fixed indenting by adapting it to current structure (I hope I didn't mess it up again)

I tested it with the same commands and devices as last time, making sure that these changes don't mess up the About Activity since it also uses CatimaTopAppBar, but without any overflow action.

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.

Add overflow menu to image viewer to open image in image gallery

2 participants