feat: split complex loyaltyCardViewActivity#3072
feat: split complex loyaltyCardViewActivity#3072NightShiftNexus wants to merge 7 commits intoCatimaLoyalty:mainfrom
Conversation
|
That's a lot of changes, will take me a while to review. I must say though that I'm not a fan of the amount of removed comments. Many of these comments were added to explain not clearly obvious stuff, like " // Detect square-ish screens like the Unihertz Titan". Can you please restore the comments? |
|
@TheLastProject Very good point, I have restored the comments |
TheLastProject
left a comment
TheLastProject
left a comment
There was a problem hiding this comment.
When I saw this huge MR come my way I was honestly a bit skeptical, but I believe you're right in that restructuring the activity like this helps maintainability. My main reason for suggesting to split the fullscreen and non-fullscreen parts into two separate activities was because the most critical part of this is the Android XML being less and less supported by Google and therefore needing Jetpack Compose and those components complicating the XML, but yes, the general view activity was quite a mess already too 😅
I have some nitpicks and some questions, but in general I fully support this restructure and I appreciate you taking the time to do this.
And thanks for fixing the memory leaks with the database Catima has. I'm not sure how necessary that is for the unit tests but hey, I guess it can't hurt to have less warning spam in the tests :)
| LoyaltyCardImageNavigator cardNavigator = new LoyaltyCardImageNavigator(new ArrayList<>(), 0); | ||
| private LoyaltyCardMainImageRenderer mainImageRenderer; | ||
| private final LoyaltyCardViewDialogs dialogs = new LoyaltyCardViewDialogs(); | ||
| private int pendingImageIndex = 0; |
There was a problem hiding this comment.
It seems weird to me to store the index in both the ViewActivity and the CardImageNavigation. Seems like an easy way for things to desync.
What is the reason for this?
There was a problem hiding this comment.
In short - pendingImageIndex as originally written was effectively a second piece of long-lived state next to cardNavigator, and that could indeed drift over time. The only reason a separate value existed at all was state restoration. After recreation we need to remember which image was selected, but we cannot rebuild LoyaltyCardImageNavigator immediately in onCreate(), because its contents depend on card data and available media loaded later from the database.
I changed it to restoredImageIndex, which is now only a temporary bootstrap value. It is read from savedInstanceState, used once when the navigator is rebuilt, and then cleared immediately. After that, cardNavigator is again the single source of truth for the current image index.
Maybe there is a better method. I have some ideas for a possible change to the current logic, the question is whether it is worth it at this point because I sense that the transition to kotlin may change something else in the current structure
| public static final String BUNDLE_TRANSITION_RIGHT = "transition_right"; | ||
|
|
||
| final private TaskHandler mTasks = new TaskHandler(); | ||
| Runnable barcodeImageGenerationFinishedCallback; |
There was a problem hiding this comment.
This seems to have become unused so we should be able to delete this.
There was a problem hiding this comment.
It is currently used as the name transition_right to make it clearer
| public boolean onKeyDown(int keyCode, KeyEvent event) { | ||
| if (settings.useVolumeKeysForNavigation()) { | ||
| if (keyCode == KeyEvent.KEYCODE_VOLUME_UP) { | ||
| // Navigate to the previous card |
There was a problem hiding this comment.
false being previous and true being next card in prevNextCard isn't immediately obvious I think so this comment is better restored I think.
There was a problem hiding this comment.
Instead of false/true I created AdjacentCardDirection enum
| } | ||
| return true; | ||
| } else if (keyCode == KeyEvent.KEYCODE_VOLUME_DOWN) { | ||
| // Navigate to the next card |
There was a problem hiding this comment.
false being previous and true being next card in prevNextCard isn't immediately obvious I think so this comment is better restored I think.
There was a problem hiding this comment.
Instead of false/true I created AdjacentCardDirection enum
| Toast.makeText(this, R.string.barcodeLongPressMessage, Toast.LENGTH_SHORT).show(); | ||
| return; | ||
| default: | ||
| // Empty default case for now to keep the spotBugsRelease job happy |
There was a problem hiding this comment.
Given we don't use SpotBugs anymore may as well just remove this default case fully
| import protect.card_locker.cardview.LoyaltyCardViewActivity; | ||
|
|
There was a problem hiding this comment.
This import seems unnecessary, right?
There was a problem hiding this comment.
Is used for example in line
Intent openIntent = new Intent(this, LoyaltyCardViewActivity.class)
.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
.putExtra(LoyaltyCardViewActivity.BUNDLE_ID, card.id);
| import android.widget.RemoteViews | ||
| import androidx.core.widget.RemoteViewsCompat | ||
| import protect.card_locker.DBHelper.LoyaltyCardArchiveFilter | ||
| import protect.card_locker.cardview.LoyaltyCardViewActivity |
There was a problem hiding this comment.
This import seems unnecessary, right?
There was a problem hiding this comment.
Is used for example in line
val templateIntent = Intent(context, LoyaltyCardViewActivity::class.java).apply {
flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK
}
| import java.util.LinkedList; | ||
|
|
||
| class ShortcutHelper { | ||
| import protect.card_locker.cardview.LoyaltyCardViewActivity; |
There was a problem hiding this comment.
This import seems unnecessary, right?
There was a problem hiding this comment.
Currently is used for example in
Intent intent = new Intent(context, LoyaltyCardViewActivity.class);
I removed import java.util.Collections; because it was unnecessary
| import java.util.Date; | ||
| import java.util.List; | ||
|
|
||
| import protect.card_locker.cardview.LoyaltyCardViewActivity; |
There was a problem hiding this comment.
This import seems unnecessary, right?
| Integer cardId; | ||
|
|
||
| for (boolean newCard : new boolean[]{false, true}) { | ||
| for (boolean newCard : new boolean[]{false, true}) { |
There was a problem hiding this comment.
I guess everything below this line should get indented one more level too :)
2 participants
First part of #2483 - LoyaltyCardViewActivity
This refactor reduces the complexity of LoyaltyCardViewActivity by splitting it along responsibility boundaries rather than into separate normal and fullscreen activities. In the current implementation, fullscreen is still part of the same screen flow: it uses the same card data, the same image navigation, the same actions, and the same lifecycle context.
Creating a second activity for fullscreen would therefore introduce additional state passing and coordination without addressing the real source of complexity.
Instead, the activity was simplified by extracting focused parts of its behavior into smaller, self-contained collaborators. This keeps a single entry point for the screen while making the overall structure easier to understand, maintain, and extend. In my opinion it's better way to reduce current complexity.