search

nutritionfactsorg / daily-dozen-android

Keep track of the foods that Dr. Greger recommends in his NYT's best-selling book, How Not to Die with this Android app
https://play.google.com/store/apps/details?id=org.nutritionfacts.dailydozen&hl=en
Other
274 stars 95 forks source link

Multiple reminders #93

Closed paynemiller92 closed 6 years ago

paynemiller92 commented 6 years ago

Original feature alongside Ali's fixes.

slavick commented 6 years ago

Hi @paynemiller92,

Thank you for the pull request for multiple reminders. This is a feature I have also wanted in Daily Dozen. However, I have found several major issues that must be fixed before this can be merged.

  1. Crash on startup - Users with the app already installed and who have the daily reminder enabled (which is by default), will find that their app will crash upon startup forever. The only fix for them after the update would be to delete the app and reinstall it. I'm guessing most people don't backup their data before they upgrade (most probably upgrade automatically), so this would result in them suddenly losing their data. The issue here is that the SharedPrefs are assumed to be a Set and not a String. So the first thing the app does is try to convert a String into a Set and crash. You should handle the exception that is thrown and convert the String to a Set.

  2. There is no way to remove a reminder once it has been added.

  3. Add enough reminders and the floating action button is pushed off screen. Since reminders cannot be removed, the user will never see this button again. On my Nexus 6, this is 6 reminders, but on a smaller screen phone, this could be 3. The FAB also does not act like a normal FAB. It should be positioned in the bottom right corner and not be moved around when other things on the screen are added or removed. I'm not a huge fan of FABs anyways, so I wouldn't mind if this were instead a button in the status bar next to the title.

  4. Duplicate reminders can be created for the same time. This can be fixed by overriding the equals() method of the UpdateReminderPref class. UpdateReminderPrefs can be considered equal if their times are the same. Then, just put all of the UpdateReminderPref objects into a Set (which will automatically filter out the duplicates) and from there convert to JSON.

I will review the pull request again when you make the above changes.

Thank you

paynemiller92 commented 6 years ago

@slavick Sure thing! I will hop on these, I was unaware this feature covered deletion and duplicates as well. I thought the functionality was only multiple reminders.

slavick commented 6 years ago

@paynemiller92 I'm closing this PR for housekeeping. I'm willing to revisit this if you address the above points.

Thank you for the time you put into working on this.