Skip to content
This repository has been archived by the owner on Feb 8, 2023. It is now read-only.

Fix for days_between_notifications #32

Closed
wants to merge 2 commits into from
Closed

Conversation

aysiu
Copy link
Contributor

@aysiu aysiu commented Jan 22, 2020

This fixes it so that if days_between_notifications is set, it's also respected.

Two pieces to this are:

  1. Using the right comparison variable (os_version instead of os_version_major)
  2. Updating the last_seen pref every time the notification window actually appears

aysiu added 2 commits January 22, 2020 15:47
Right now, the only times `last_seen` is updated are when it's blank or to make it blank if no updates are needed.

In order not to notify if there is `days_between_notifications` set, `last_seen` has to be updated when the actual notification happens.
Otherwise, you're comparing something like `10.15` to `10`.

In order to compare `10.15` to `10.15`, the right side of the comparison has to be `os_version` instead of `os_version_major`.
@aysiu
Copy link
Contributor Author

aysiu commented Jan 23, 2020

As I'm doing further testing, I'm not sure that second commit makes sense. I swear I have a 10.15 machine on which it spits back 10 instead of 10.15, but I'm looking on another machine now where it spits back 10.15 for os_version_major.

Disregard this PR for now. I clearly need to do even more testing...

@aysiu aysiu closed this Jan 23, 2020
@erikng
Copy link
Member

erikng commented Jan 23, 2020

Heh. Welcome to my life.

@aysiu aysiu deleted the patch-4 branch March 12, 2020 16:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants