Allow users to turn off moveset info #919
Conversation
|
There are a couple of things I'd like to review, but I'm busy at the moment. |
Agilo3
force-pushed
the
feature/set-visibility-for-moveset-info
branch
from
1e66c84
to
03093b5
Compare
|
Did a quick rebase onto master just to keep things tidy. Also, I did test the clipboard functionality when turning the option on/off. I'll be interested to hear your review. This pull request is the first Android development I've done so far so I value your feedback; thanks! I'm also available on the Discord channel for any direct messages (either PM or in #dev). |
There was a problem hiding this comment.
There are some minor fixes here and there, the only change I'd request is to not remove moveset clipboard tokens from the tokens configuration UI while I require that the ClipboardTokenCollection.getSamples return value don't change based on user preferences.
Overall a good implementation!
| @@ -323,6 +325,10 @@ public int onStartCommand(Intent intent, int flags, int startId) { | |||
| return START_REDELIVER_INTENT; | |||
| } | |||
|
|
|||
| private boolean isMovesetEnabled() { | |||
There was a problem hiding this comment.
This new method can be avoided if possible, I'd prefer using GoIVSettings.getInstance(this).isMovesetEnabled() directly!
| super.onResume(); | ||
|
|
||
| if (viewPager != null) | ||
| { |
There was a problem hiding this comment.
This curly bracket should be on the previous line ;)
| @@ -59,6 +59,21 @@ public View onCreateView(@NonNull LayoutInflater inflater, | |||
| return view; | |||
| } | |||
|
|
|||
| @Override | |||
| //Pressed return button - in the case when coming back from settings screen after movesets have been enable/disabled | |||
| public void onResume() { | |||
There was a problem hiding this comment.
Avoid all this behavior, all this added complexity just to remove the moveset clipboard tokens from the list is overkill.
If a user have deactivated the moveset functionality in the settings, he would probably just ignore the movesets tokens knowing that they won't be effective.
Also, moveset tokens should already work when the movesets info isn't present (i.e. when the moveset scan fails).
| tokens.add(new MovesetInitialsToken(2)); | ||
|
|
||
| //enable/disable visibility based on setting | ||
| if (GoIVSettings.getInstance(context).isMovesetEnabled()) { |
There was a problem hiding this comment.
This method and the list it returns shouldn't be changed by the user's preferences because this same list is used to read/write tokens configuration into sharedpreferences.
This list should never change its contents (aside from new token additions when a new app version is released) or the token configuration logic would fail.
| moveFast = moveset.first; | ||
| moveCharge = moveset.second; | ||
|
|
||
| String moveFast = ""; |
There was a problem hiding this comment.
If the moveset scan fails moveFast and moveCharge should be null, otherwise ScanData / ScanResult data structures would behave like the moveset scan was successful. Please initialize this variables to null instead of empty String
| @@ -59,4 +59,6 @@ | |||
| <string name="token_pokemonname">Jméno Pokémona</string> | |||
| <string name="clipboard_modifier_preview">Náhled</string> | |||
| <string name="position_handler_toast">Přemísti okno táhnutím tohoto držáku.</string> | |||
| <string name="moveset_enabled_setting_summary" /> | |||
There was a problem hiding this comment.
Please do not declare translation keys without values
|
Hi @Agilo3, I'm not very active on Discord lately but I'd like to meet you there soon :) |
|
I'd like to point out that, while the code is high quality, its for a feature that I've rejected in #908 discussion. Thearaks seems to have started a discussion about changes for this PR with you, but in the future it'd be highly preferred if you discussed in Github issue threads or in the discord dev channel before creating PRs. I'm however impressed that you managed to understand the GoIV codebase well enough to actually make this change. |
…sabled" This reverts commit c983f88.
Agilo3
force-pushed
the
feature/set-visibility-for-moveset-info
branch
from
1768b23
to
d70beb4
Compare
This will give users the option to hide moveset info. Saves a bit of resources and allows people to simply turn it off if they don't care about the integration in GoIV.