Sync separate #17417
Closed
Sync separate #17417
Conversation
This file contains 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
Comment on lines
+184
to
+213
| fun updateSyncIconFromState(menu: Menu) { | ||
| val state = syncMenuState | ||
| if (state == null) { | ||
| return | ||
| } | ||
| val menuItem = menu.findItem(R.id.action_sync) | ||
| val provider = MenuItemCompat.getActionProvider(menuItem) as? SyncActionProvider | ||
| ?: return | ||
| val tooltipText = when (state.syncIcon) { | ||
| SyncIconState.Normal, SyncIconState.PendingChanges -> R.string.button_sync | ||
| SyncIconState.OneWay -> R.string.sync_menu_title_one_way_sync | ||
| SyncIconState.NotLoggedIn -> R.string.sync_menu_title_no_account | ||
| } | ||
| provider.setTooltipText(activity.getString(tooltipText)) | ||
| when (state.syncIcon) { | ||
| SyncIconState.Normal -> { | ||
| BadgeDrawableBuilder.removeBadge(provider) | ||
| } | ||
| SyncIconState.PendingChanges -> { | ||
| BadgeDrawableBuilder(activity) | ||
| .withColor(activity.getColor(R.color.badge_warning)) | ||
| .replaceBadge(provider) | ||
| } | ||
| SyncIconState.OneWay, SyncIconState.NotLoggedIn -> { | ||
| BadgeDrawableBuilder(activity) | ||
| .withText('!') | ||
| .withColor(activity.getColor(R.color.badge_error)) | ||
| .replaceBadge(provider) | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this DeckPicker specific?
There was a problem hiding this comment.
Currently, it is specific to the deck picker.
One of the point I mentioned in the full PR is that I don't know whether I'd want the badge to appear in the reviewer or not. I'd expect the "full sync needed" icon to be very important. The normal sync, as it will appear after the first review, it may be too much of a distraction.
I'd argue that, even if we decide to let it be used in the deck picker only, it's still nice to have the code in Sync, because the DeckPicker is already very big
david-allison
added
Needs Author Reply
This class deals with the entire Sync part of the deck picker.
Ensuring there is no typo when using deck id in a reviewer intent
The enum was used only in one function. Adding an intermediary step does not add clarity to the code. Each method were also only used once. I think the resulting code is easier to read, thanks to less layer of abstractions.
Arthur-Milchior
force-pushed
the
sync_separate
branch
from
040f71c
to
db07e78
Compare
Arthur-Milchior
removed
the
Needs Author Reply
|
Closing it, because I was able to deal with both reviewer, and had to do change to this system |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is a preamble for #17399. It contains everything related to cleaning the sync code and separating it from the DeckPicker.
I moved most of the code in a single file, that can be reused by the reviewer. I remove SyncStatus which added extra abstraction layer without , I believe, adding any clarity to the code. And replaced a literal by a constant