Added overflow menu to open card image in image gallery

[KamiDaga]

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]

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]

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]

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]

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.

[TheLastProject]

It kinda makes sense to go for something like this, but I don't think it'll really scale. You're putting all possible logic in the CatimaTopAppBar element, which will completely balloon dealing with hundreds of options at some point.

It's probably cleanest to just pass the actions Compose code directly to the CatimaTopAppBar, that will scale much better, especially because actions will generally be unique per activity anyway.

[KamiDaga]

Yeah I agree with you, it's way better to just treat CatimaTopAppBar as a simple runner or it will become huge. Putting the actions Compose code directly in the individual activities will also avoid non necessary validation of the parameters on which the actions are based.

[KamiDaga]

Applied the change in the latest commit.
I couldn't find a way to reuse the actions building logic without resulting in an exploding method again, but given the fact that actions are generally unique per activity I thought that initializing the menu in each activity that will need one with it's own entries would be fitting.