Apply new notification thresholds only after timeout expires by vcrn · Pull Request #777 · GyulyVGC/sniffnet

vcrn · 2025-04-13T06:56:23Z

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().

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()`.

vcrn · 2025-04-13T07:05:02Z

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 if !self.timing_events.was_just_notifications_edit() will be true for a majority of the time that the app is running.

By the way, when looking at the docs for TickInit and TickRun, it seems like TickInit should be every 5 seconds and TickRun every second, but they both use PERIOD_TICK, which is 1 second. Is that something that should be changed?

src/gui/sniffer.rs

GyulyVGC · 2025-04-20T14:45:13Z

Some remarks from a quick review.

1. this behavior should only apply to threshold changes
2. no need to update notification settings in TickInit since notifications won't be received in the initial page of the app; you can just call the update to notifications inside Sniffer::refresh_data (fixed)
3. no need to store the configurations as part of notifications update; in fact, changes to the configuration file are only done during app shutdown (fixed)

vcrn · 2025-04-20T21:02:02Z

Thank you for the fast review! I'll fix the remaining point you brought up in the coming days and fix the failing test

vcrn · 2025-04-23T08:30:32Z

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 Option<(PacketNotification, BytesNotification)> and Option<Notifications>. I went with the latter since I believe it makes the code somewhat simpler, but there's merit to using the other type as well in case you want me to go that route.

GyulyVGC · 2025-04-24T15:22:06Z

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.

vcrn · 2025-04-24T16:24:11Z

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

GyulyVGC · 2025-04-24T19:56:41Z

Thank you!

vcrn · 2025-05-06T13:40:17Z

If the user changes the ByteMultiple of BytesNotification, do you think that should be applied immediately?

GyulyVGC · 2025-05-06T13:48:43Z

If the user changes the ByteMultiple of BytesNotification, do you think that should be applied immediately?

No. The Byte multiplier is part of the threshold and is set via the text input, so its modification has to be subject to timeout.

vcrn · 2025-05-07T07:30:09Z

I've gone with the other approach of storing the temporary thresholds, in a field of type Option<(PacketNotification, BytesNotification)>.

GyulyVGC · 2025-05-07T12:09:48Z

Is the PR ready for review?

vcrn · 2025-05-07T13:17:23Z

Yes, ready for review :)

GyulyVGC · 2025-05-08T09:02:13Z

src/gui/sniffer.rs

+                // Threshold adjustments are saved in `self.temp_thresholds` and then applied
+                // after timeout, but for simplicity, this field is always updated to whatever the
+                // latest change has been.
+                self.update_sound(notification, emit_sound);


Do we really need to have all these different methods to update notification values? Can't we merge all of them into one that updates everything except thresholds to make the code more compact?

Also, I think these could be an impl of Notifications structs rather than Sniffer struct.

Sure! I tried out different approaches, and went this since although it's quiet verbose, it's very explicit what's going on. Having it in one method looked quiet convoluted in the end when I tried it, but I'll give it another go, I'll probably have some new ideas of how to make it cleaner :)

I've tried having this an an impl of Notifications instead, but I believe it will make things more complicated. The majority of the code in these methods are to unpack and compare notifications settings and finally potentially apply them immediately. If we put it in impl Notifications we won't be able to apply them, which in my mind would cause an extra layer somewhere else in the code. What do you think?

Then you can leave it as Sniffer's impl but try to make it just one method that's less verbose

GyulyVGC · 2025-05-08T09:07:33Z

src/gui/sniffer.rs

    /// Host data for filter dropdowns (comboboxes)
    pub host_data_states: HostDataStates,
+    /// Temporary storage for thresholds while editing.
+    pub temp_thresholds: Option<(PacketsNotification, BytesNotification)>,


I suggest this to be part of the field TimingEvents::threshold_adjust like (std::time::Instant, Option<(PacketsNotification, BytesNotification)>).
This way you can update it when you call threshold_adjust_now, and you can make was_just_threshold_adjust return an Option instead of bool.

Good idea, I'll implement this!

GyulyVGC · 2025-05-17T21:56:02Z

Hey @vcrn how're you doing?

I just wanted you to know that in a few days I'll merge #795 into main.
Since that is a huge PR and would cause many conflicts, I'd like to have this one finished and merged first if possible.

vcrn · 2025-05-18T04:15:50Z

Thanks for the heads-up! I'll try to get something pushed by the end of the day

vcrn · 2025-05-18T09:36:07Z

I have something in place that is a bit rough but it does what it's supposed to do. Since this latest round is more concerning design, I'm considering slightly polishing what I have and pushing it to hear your thoughts on these changes. What do you think?

GyulyVGC · 2025-05-18T09:52:56Z

IIRC the last time I tested the most recent commit, it was already working fine from a functional perspective.
So yeah, the main concern was about making it more compact since I think there was too much code for what we're trying to do.

…g events

vcrn · 2025-05-18T12:06:00Z

Not sure why clippy failed in the pipeline for MacOS but not the others (Linux at least fine when I run it locally, but the build was cancelled in the pipeline, I guess since MacOS failed before that).

Anyway, clippy for MacOS complains about some code I haven't touched, so you better be the judge of whether it should be changed or ignored.

Regarding my latest change, I created one big method that handles all the logic of updating notifications settings and temporary thresholds. Macros could probably decrease the lines of code, but I tried to keep repetition and abstraction layers to a minimum to get your feedback on which way we should go.

I'll also add a comment here on GitHub for a function that I think needs some extra attention.

vcrn · 2025-05-18T12:07:54Z

src/gui/types/timing_events.rs

+    }
+
+    /// If timeout has expired, take temporary thresholds
+    pub fn threshold_adjust_expired_take(


Pretty bad name, but I wanted it to be clear that it mutates the value. Also, the logic is changed, since we want a value to returned/taken if enough time has passed: compared with previous version, where true was returned if not enough time had passed.

I'd call it was_just_threshold_adjust_take.

and turn the logic the way round if possible, otherwise leave it as it is

I don't believe we'll be able to change the behavior of "if timeout has expired, return taken value, else None". If this is the case, I can add a doc string explaining how the method works.

Also, could potentially go with the name was_not_just_threshold_adjust_take, to try and point out the somewhat reversed behavior compared with the other methods since to most people false ~ None, true ~ Some. At the cost of making the name more convoluted.

I'll think to it and will personally take care of it during the review 👍

GyulyVGC · 2025-05-18T12:11:12Z

Thanks will check this out.
Don't worry about clippy, I'll fix it. By the way, it's due to the new clippy lints recently added in the latest version of Rust.

vcrn · 2025-05-18T12:18:50Z

Yeah, after updating my toolchain it's failing on Linux as well. A bit odd that it isn't failing for Windows though, but maybe it isn't up to date

GyulyVGC · 2025-05-18T12:27:22Z

On Windows the CI/CD is skipped in PRs since they need a secret environment variable to be set at the repo level (used to download Npcap).

…is clicked

GyulyVGC · 2025-05-21T22:03:43Z

Finally merged 🎉

Thanks mate, I hope I wasn't too pedantic during the process 🤣

@all-contributors please add @vcrn for code

allcontributors · 2025-05-21T22:03:53Z

@GyulyVGC

I've put up a pull request to add @vcrn! 🎉

vcrn · 2025-05-22T05:05:34Z

Finally merged 🎉

Thanks mate, I hope I wasn't too pedantic during the process 🤣

@all-contributors please add @vcrn for code

Don't worry, you weren't too pedantic, you we're great! 🙂 Really enjoyed this PR and learned some stuff as well.

I'll be on the look out for other issues I can help out with. Thanks again for a great app and for all your work!

allcontributors · 2025-05-22T05:05:38Z

@vcrn

@vcrn already contributed before to code

GyulyVGC added the enhancement New feature, request, or improvement label Apr 13, 2025

GyulyVGC added this to the v1.4.0 milestone Apr 13, 2025

GyulyVGC linked an issue Apr 13, 2025 that may be closed by this pull request

Threshold values for notifications immediately get updated while the user is still typing #658

Closed

vcrn commented Apr 14, 2025

View reviewed changes

src/gui/sniffer.rs Outdated Show resolved Hide resolved

GyulyVGC added 3 commits April 20, 2025 16:17

fix merge conflicts

722a5e9

fmt'd

8e1b3fa

avoid storing configuration at each notifications update

66a03c0

update notification settings as part of Sniffer::refresh_data

825d0b5

Apply non-thresholds immediately

26ab6c1

GyulyVGC assigned vcrn Apr 23, 2025

vcrn changed the title ~~Apply new notifications settings only after timeout expires~~ Apply new notification thresholds only after timeout expires Apr 24, 2025

fix clippy::pedantic lints

791db4b

GyulyVGC marked this pull request as draft May 1, 2025 21:33

vcrn added 2 commits May 7, 2025 08:46

Apply non-thresholds immediately

5ffd961

Fix messed up rebase

1dba64f

vcrn marked this pull request as ready for review May 7, 2025 13:16

Merge branch 'main' into update-finished-typing

27c2474

update CHANGELOG

37b030c

GyulyVGC reviewed May 8, 2025

View reviewed changes

GyulyVGC marked this pull request as draft May 8, 2025 13:14

Update notifications settings in one method, temp thresholds in timin…

a074f6b

…g events

vcrn marked this pull request as ready for review May 18, 2025 12:06

vcrn commented May 18, 2025

View reviewed changes

fix regression about sound not being emitted if current sound button …

952c7a8

…is clicked

GyulyVGC approved these changes May 21, 2025

View reviewed changes

GyulyVGC merged commit 47e219a into GyulyVGC:main May 21, 2025
1 of 3 checks passed

allcontributors bot mentioned this pull request May 21, 2025

docs: add vcrn as a contributor for code #811

Merged

BrewTestBot mentioned this pull request Jun 27, 2025

sniffnet 1.4.0 Homebrew/homebrew-core#228386

Merged

Uh oh!

Comments

Conversation

vcrn commented Apr 13, 2025

Uh oh!

vcrn commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

GyulyVGC commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcrn commented Apr 20, 2025

Uh oh!

vcrn commented Apr 23, 2025

Uh oh!

GyulyVGC commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcrn commented Apr 24, 2025

Uh oh!

GyulyVGC commented Apr 24, 2025

Uh oh!

vcrn commented May 6, 2025

Uh oh!

GyulyVGC commented May 6, 2025

Uh oh!

vcrn commented May 7, 2025

Uh oh!

GyulyVGC commented May 7, 2025

Uh oh!

vcrn commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GyulyVGC May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GyulyVGC commented May 17, 2025

Uh oh!

vcrn commented May 18, 2025

Uh oh!

vcrn commented May 18, 2025

Uh oh!

GyulyVGC commented May 18, 2025

Uh oh!

vcrn commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GyulyVGC commented May 18, 2025

Uh oh!

vcrn commented May 18, 2025

Uh oh!

GyulyVGC commented May 18, 2025

Uh oh!

Uh oh!

GyulyVGC commented May 21, 2025

Uh oh!

allcontributors bot commented May 21, 2025

vcrn commented Apr 13, 2025 •

edited

Loading

GyulyVGC commented Apr 20, 2025 •

edited

Loading

GyulyVGC commented Apr 24, 2025 •

edited

Loading

GyulyVGC May 8, 2025 •

edited

Loading

vcrn commented May 18, 2025 •

edited

Loading