fix internal image zoom#3184
Conversation
|
Any chance the new file could be in Kotlin? Java is being deprecated by Google so I'd rather not accept more new Java code where it can be easily prevented. I haven't had time to review the actual change yet, but it seems this adds even more complexity to the view activity? Given Google is reducing support for Android XML and the view activity still depends on Android XML (and still needs to be migrated, see #2483), I will need to look closely if this won't make migrating the view activity harder. I'll get back later after a basic review, this might need its own view to prevent the already overly complex view activity to become even harder to migrate. |
|
thanks for coming back so quickly yes will do, but I'll need the next weekend for it . maybe its wise for me to rework this first, before you spend time looking at it but if youhave the time now that would be great of course too I will think about the issues and implications of the view activity and the #2483 thing as well; thanks for steering me toward this one update: just changed to a draft |
|
Had some time to test it. The zooming looks good, but I do worry about the extra complexity. Maybe the zooming can have its own UI, just a very minimal Jetpack Compose UI screen (look at AboutActivity for that) and we can just launch that from the view screen? I think that may be better for maintainability. |
ilyaZar
force-pushed
the
fix/internal-image-zoom
branch
from
a6c89ce to
27bcb6c
Compare
|
Hi , thanks for your comments again @TheLastProject let me know what else you'd like to have in there, whenever you have time |
TheLastProject
left a comment
TheLastProject
left a comment
There was a problem hiding this comment.
I'll have to take a deeper look soon, but the general flow seems reasonable. Noticing one consistency-with-rest-of-codebase issue. I'll do a test and deeper review later :)
| @Composable | ||
| fun LoyaltyCardImageScreen( | ||
| bitmap: Bitmap, | ||
| contentDescription: String, | ||
| onBack: () -> Unit, | ||
| ) { |
There was a problem hiding this comment.
Please put the ui elements in the compose directory as done with other activities.
There was a problem hiding this comment.
yea true, thx, see commit below
- keep new compose ui with existing compose code
2 participants
fix #2156
per the discussion; happy to change the PR with changes related to other issues (though I only digged up the 2156 one)
thanks for the great app, I really like it !