Improve ShortcuHelper.updateShortcuts to take all actions into account #2919
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
TheLastProject
left a comment
TheLastProject
left a comment
There was a problem hiding this comment.
I feel the general refactor feels correct, nice that it fixes 2 issues at once. Haven't tested yet, so just commenting on the general code.
| DBHelper.updateLoyaltyCardArchiveStatus(database, loyaltyCardId, 1); | ||
| Toast.makeText(LoyaltyCardViewActivity.this, R.string.archived, Toast.LENGTH_LONG).show(); | ||
|
|
||
| ShortcutHelper.removeShortcut(LoyaltyCardViewActivity.this, loyaltyCardId); |
Member
There was a problem hiding this comment.
Shouldn't this still call update at least?
|
|
||
| DBHelper.deleteLoyaltyCard(database, LoyaltyCardViewActivity.this, loyaltyCardId); | ||
|
|
||
| ShortcutHelper.removeShortcut(LoyaltyCardViewActivity.this, loyaltyCardId); |
Member
There was a problem hiding this comment.
Same here, shouldn't this call update to ensure it disappears?
|
|
||
| DBHelper.deleteLoyaltyCard(mDatabase, this@MainActivity, loyaltyCard.id) | ||
|
|
||
| ShortcutHelper.removeShortcut(this@MainActivity, loyaltyCard.id) |
| for (loyaltyCard in mAdapter.getSelectedItems()) { | ||
| Log.d(TAG, "Archiving card: " + loyaltyCard.id) | ||
| DBHelper.updateLoyaltyCardArchiveStatus(mDatabase, loyaltyCard.id, 1) | ||
| ShortcutHelper.removeShortcut(this@MainActivity, loyaltyCard.id) |
| * manually modified. We use -1 here as a default value to check if | ||
| * the value has been set either manually by the test scenario or | ||
| * automatically in the `updateShortcuts` function. | ||
| * It's actual value will be set based on the maximum amount of shortcuts |
Member
There was a problem hiding this comment.
Not a native speaker but I believe this should be "Its"
Suggested change
| * It's actual value will be set based on the maximum amount of shortcuts | |
| * Its actual value will be set based on the maximum amount of shortcuts |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, following our discussion on #2810 here is a PR for the changes, let me know what you think.
This actually fixes #2810 and #2811.
The function now updates the shortcuts list by searching the database for the most recently used items.
Fetching this data from the database via the existing cursor function had the side effect of getting favorite/starred items first. Which is why I also tackled #2811 in this PR as well.
The maximum number of shortcuts is now dependant on the launcher's maximum possible shortcuts and is editable during testing. This needs to be tested on actual hardware, the emulator reported 16 available slots when calling
getMaxShortcutCountPerActivitywhich sounds a bit much to me.I added 2 tests for the ShortcutHelper class for this new behavior. Hopefully these are useful, this part of the code wasn't tested.