Fix #4006: More advanced notification options #4450
Conversation
|
This need some extra work. However, are you ok with the idea? |
| import android.content.SharedPreferences; | ||
| import android.app.AlarmManager; | ||
| import android.app.PendingIntent; | ||
| import android.content.*; |
There was a problem hiding this comment.
Maybe you've set up your editor to do this automatically, but there is no reason to change these imports to wildcards in this PR.
| + "'reminder': {" | ||
| + "'enabled': False," | ||
| + "'time': [0, 0]" | ||
| + "}," |
There was a problem hiding this comment.
Don't add this to the default config. We try to avoid changing libanki code where it deviates from the original python, and in this case the feature will work without it since we already handle cases where it's missing.
| break; | ||
| } | ||
| } | ||
| if ((null == deckDue) || (deckDue.newCount <= 0)) { |
There was a problem hiding this comment.
I think it's better to use deckDue.revCount + deckDue.lrnCount + deckDue.newCount here, instead of just newCount, because sometimes there are no "new" cards, only reviews.
| final long deckId = intent.getLongExtra(ReminderService.EXTRA_DECK_ID, 0); | ||
| Sched.DeckDueTreeNode deckDue = null; | ||
|
|
||
| for (Sched.DeckDueTreeNode node : CollectionHelper.getInstance().getCol(this).getSched().deckDueList()) { |
There was a problem hiding this comment.
deckDueList() does not work for nested decks well, I should use deckDueTree() here
|
Are you ready for re-review? |
| mValues.put("reminderEnabled", Boolean.toString(reminder.getBoolean("enabled"))); | ||
| mValues.put("reminderTime", String.format("%1$02d:%2$02d", reminderTime.get(0), reminderTime.get(1))); | ||
| } else { | ||
| mValues.put("reminderEnabled", "false"); |
There was a problem hiding this comment.
Is it necessary to insert the preference value even when it's not enabled? I.e., can we get away with not adding the value into the user's collection unless they explicitly turn on the feature first?
There was a problem hiding this comment.
It makes logic simpler in cases where user already had reminders enabled and want to disable them.
| final AlarmManager alarmManager = (AlarmManager) getSystemService(ALARM_SERVICE); | ||
| final PendingIntent reminderIntent = PendingIntent.getBroadcast( | ||
| getApplicationContext(), | ||
| (int) mDeck.getLong("id"), |
There was a problem hiding this comment.
Will this cast (and all the other ones) cause a problem?
There was a problem hiding this comment.
Technically, yes, it can cause a problem. E.g. if you have two decks, one with id equal to 2147484647 (Integer.MAX_INT + 1000) and the other with -2147482649, then both (int) 2147484647 and (int) -2147482649 will be the same value, -2147482649. But the expected argument type here is int, so data loss is inevitable, whether it is casting or bit shifts.
I just hope that users usually don't have thousands of decks and the probability of a collision is low.
There was a problem hiding this comment.
It shouldn't be problem since we aren't writing any data back into the collection.
|
|
||
| <string name="reminder_title">Do not forget to study today!</string> | ||
| <plurals name="reminder_text"> | ||
| <item quantity="one">%1$s deck has %2$d new card to review</item> |
There was a problem hiding this comment.
I think you can reword this:
20 new cards to review in Japanese
The word "deck" shouldn't be necessary.,
| 0 | ||
| ); | ||
|
|
||
| alarmManager.cancel(reminderIntent); |
There was a problem hiding this comment.
I'm not sure if this belongs here.
Would it make more sense if the service, on handling an alarm, checks first to see if its deck still exists, and then cancel the alarm if it doesn't?
|
Okay, I think this is fine now. If you could squash this down to a single commit I will merge it. |
|
Would you be able to add some documentation to the manual? |
|
Ok, I will. |
|
Created a PR in the docs repo. |
|
@madhead https://groups.google.com/forum/#!topic/anki-android/qgURUxwTFLg |
No description provided.