[GSoC] Implemented New notification preference UI #12635
Conversation
Notification preference datastore is used to store all the data related to notification in the preference datastore.
NotificationPreferenceAdapter is used to display list of decks for notification preference.
|
Message to maintainers, this PR contains strings changes.
Read more about updating strings on the wiki, |
prateek-singh-3212
added
Review High Priority
prateek-singh-3212
force-pushed
the
Deck_Notification_UI
branch
from
7460a85 to
0bebebd
Compare
|
This doesn't build. Could you please re-link the video or screenshots, they're mentioned in the PR header, but I don't see them |
BrayanDSO
added
the
Needs Author Reply
The build is failing because of Timepicker 'currentHour' method is depricated above API 23. but I was unable to find the correct replacement for that... |
prateek-singh-3212
removed
the
Needs Author Reply
|
|
david-allison
added
the
Needs Author Reply
|
Move the property to our |
prateek-singh-3212
force-pushed
the
Deck_Notification_UI
branch
2 times, most recently
from
74b86bc to
254fca5
Compare
New Notification Preference will show the list of decks as per my GSoC Proposal and dissucced on issue (ankidroid#4944)
prateek-singh-3212
force-pushed
the
Deck_Notification_UI
branch
from
254fca5 to
5db556d
Compare
prateek-singh-3212
force-pushed
the
Deck_Notification_UI
branch
from
5db556d to
d42a289
Compare
|
@ankidroid/gsoc-mentor I implemented the complete system. You can check this branch BRANCH I am attaching the video of the same. The timezone change is also working (Please let me know If you want me to add a timezone change video too) sa.mp4NOTE: use this tag
4 Logs when System is triggering the notification for This is the system which is built so far. Please let me know the additional thinks which should be added in the basic notification system i.e things required in the first relase. |
There was a problem hiding this comment.
I haven't had a chance to run the code yet, so I'm not commenting on the behavior or design here. The code has some major issues, and I think parts of this PR are overengineered (too complicated when a simpler solution is possible). I suggest not serializing, and also removing most of async stuff or at least limiting it a few isolated spaced.
| import com.ichi2.libanki.Decks | ||
|
|
||
| /** | ||
| * Notification details of particular deck. |
There was a problem hiding this comment.
This comment isn't very helpful
| /** | ||
| * Notification details of particular deck. | ||
| * */ | ||
| data class DeckNotification( |
There was a problem hiding this comment.
I'd prefer the name to reflect that this is about reminding to study. It's hard to give this an exact name but maybe something like RemindToStudy. Also I'd ditch enabled and rely on presence or absence of instances of this class.
Also, a matter of personal preference, I'd say deckId. In my opinion, did is a mistake not worth following.
| ) { | ||
|
|
||
| // Empty constructor. Required for jackson to serialize. | ||
| constructor() : this( |
There was a problem hiding this comment.
I think this is way too simple to serialize.
Serialization is fine for possibly complex objects such as encryption keys, or for objects that aren't supposed to be long-lived. Serialization is nice until you need to change how you store your data, or until some weird thing changes and you can't deserialize. For preferences, especially in such simple cases like this, I'd strongly prefer something more manual. E.g. just encode all of enabled decks and their times in a string.
Also, why jackson istead of kotlinx.serialization?
| * @param started Day on which deck started. | ||
| * @param completed Deck is done for the day or not. | ||
| * */ | ||
| data class UserNotificationPreference( |
There was a problem hiding this comment.
I have no idea what this entire class is about.
| /* | ||
| * We use TimeOfNotification as key of json object. So it must be string. | ||
| */ | ||
| typealias TimeOfNotification = String |
There was a problem hiding this comment.
Why does this need a new type? Epoch milliseconds should be simply a Long, or EpochMillis if you wish.
Also, surely jackson can deal with non-string keys? kotlinx does it via storing maps as lists.
Also, when I hear “time of notification” I think of stuff like "HH:MM". This is clearly not it.
| } | ||
|
|
||
| // Save Deck Notification | ||
| private fun saveDeckNotification(notification: DeckNotification) { |
There was a problem hiding this comment.
Rename this method after renaming DeckNotification
P.S. Another useful comment right here ↑
There was a problem hiding this comment.
Hey @oakkitten.
First, thanks for the review. Generally a lot of good point. And especially when creating a new system, it's clearly a good idea to have it right on first try. Or maybe second try, given that we are replacing the legacy notification system that does not work properly.
However, I would require you to please abstain from this kind of derisory comments (here and above). I double-checked with other reviewer, and we agree it's not the kind of tone we would want in review.
I agree that the comment here is not useful, and that it's repeating the function name. No problem at all with a request to remove it (or at least improve it), and potentially an explanation of why you want this change. But we would like to really keep being welcoming and helping contributors grow as developer, and I feel like this PS is not going in the right direction. Admittedly, the limit of what is welcoming is hard to know, since, for example, the cat picture could also be a nice joke when it is sent to someone you already know well and appreciate this kind of teasing. However, I'd just state that in doubt, I'd prefer in this repo that we keep on constructive comments.
(And I'm explicitly trying here an example of what I request. Still trying to be welcoming by acknowledging the work, the good intentation, the point I agree with, and explaining - or at least trying to - the request I'm making)
There was a problem hiding this comment.
I meant absolutely no derision. I see how, if reading it in isolation, there could be a chance that it could be taken as such, but do you really think that that I wanted to ridicule anyone given the overall tone of my comments?...
| } else { | ||
| UIUtils.showThemedToast( | ||
| context, | ||
| "Unable to save, Please Retry", |
There was a problem hiding this comment.
Under what circumstances should this fail? If it's disk space, does AnkiDroid survive full disk in the first place? If not, I suggest just crashing
| fun onDialogueDismiss(position: Int) | ||
| } | ||
|
|
||
| // Delete Deck Notification |
There was a problem hiding this comment.
Hello stray comment, are you lost?
| @@ -436,4 +436,8 @@ | |||
| <string name="toggle_browser_mode_help">Toggle Showing cards or notes in the browser</string> | |||
| <string name="truncate_content_help">Truncate the height of each row of the Browser to show only first 3 lines of content</string> | |||
| <string name="browser_options_dialog_heading">Browser Options</string> | |||
|
|
|||
| <string name="notification_sheet_subdeck">Include Sub Deck</string> | |||
There was a problem hiding this comment.
As a user, I'm not sure what exactly this means.
Also, I Suggest Using Sentence Capitalization Because This Is Not A Song Title.
| @@ -436,4 +436,8 @@ | |||
| <string name="toggle_browser_mode_help">Toggle Showing cards or notes in the browser</string> | |||
| <string name="truncate_content_help">Truncate the height of each row of the Browser to show only first 3 lines of content</string> | |||
| <string name="browser_options_dialog_heading">Browser Options</string> | |||
|
|
|||
| <string name="notification_sheet_subdeck">Include Sub Deck</string> | |||
| <string name="notification_sheet_sticky">Sticky Notification</string> | |||
There was a problem hiding this comment.
As a user, I don't know what's a sticky notification. As a dev, I'm not sure if this complexity is worth it.
|
Thanks, @oakkitten for reviewing this PR. I will do these UI changes shortly. Actually, the feature that we want to provide to the user is not yet decided completely. I will do all the implementation changes that you requested and once the feature is decided then I will write this as production ready. |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Codebase changed a lot since this PR was opened, and looks abandoned. It's best to create another one if work is restarted |
Pull Request template
Purpose / Description
The user is not able to manage the notification preference easily and we have to provide an additional feature to the notification System and make notification system bug free.
GSoC Proposal: https://docs.google.com/document/d/1ezQsKJxXm_-q4d5Kpafb5WHKuy3hApcPo0RXO7BULIc/edit?usp=drivesdk
WhatsApp.Video.2022-10-13.at.9.22.32.AM.mp4
Fixes
Issue: #4944
Approach
I have implemented a new notification preference UI to manage all notification settings related to a single deck in one place
How Has This Been Tested?
Tested on Samsung M52
Note: Please ignore the first 2 commits as they are part of PR: #12234
Checklist
Please, go through these checks before submitting the PR.