Apply new notification thresholds only after timeout expires #777
Conversation
Changes to notifications settings are stored temporarily in temp_notifications field. These are saved to configs only after a timeout has expired. The checking of this timeout and whether there's any new notifications settings to store are done in `TickInit` and `TickRun` arms of `fn update()`.
|
This should close #658. I tried some different approaches but finally decided to put this one up for review since this implementation is the most straightforward one to me, and the one that is the most consistent with existing design. Note that it's not just the notifications thresholds that are instantly applied, but all notifications settings. What do you think? Some things that bother is that the checking whether to update the notifications settings is done for every Tick, and that the very first if-state By the way, when looking at the docs for |
src/gui/sniffer.rs
Outdated
| if !self.timing_events.was_just_notifications_edit() { | ||
| if let Some(temp_notifications) = self.temp_notifications.take() { |
There was a problem hiding this comment.
if let Some(temp_notifications: Notifications) = self.temp_notifications {
if !self.timing_events.was_just_notifications_edit() {
self.temp_notifications = None;
Turning the logic around and adding one extra line would make this code slightly more effective, since
if !self.timing_events.was_just_notifications_edit()
is true for the majority of the time that the app is running. This isn't the case however for
if let Some(temp_notifications: Notifications) = self.temp_notifications.
There was a problem hiding this comment.
I wouldn't be too worried about this since it only happens once per second and it only consists in updating the value of a Mutex, there's no big overhead.
However, yeah, make it to do the least number of checks possible.
|
Some remarks from a quick review.
|
|
Thank you for the fast review! I'll fix the remaining point you brought up in the coming days and fix the failing test |
|
Now the thresholds are the only notifications settings that aren't applied right away. I went a bit back and forth between what type to store the not yet applied thresholds as, experimenting between using |
vcrn
changed the title
|
Haven't re-reviewed the code yet, but I notice an issue by simply testing via running Sniffnet: if I click on the checkbox to enable/disable a specific notification kind, the corresponding notifications are enabled/disabled with some latency... hence I suspect that enabling/disabling is still subject to the timeout. |
|
Thanks! I'll get it fixed and get back to you. I'll see if I can expand the testing to check for this scenario |
|
Thank you! |
Changes to notifications settings are stored temporarily in temp_notifications field. These are saved to configs only after a timeout has expired. The checking of this timeout and whether there's any new notifications settings to store are done in
TickInitandTickRunarms offn update().