Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change minimum time between replies #107

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Conversation

pixincreate
Copy link
Contributor

Reduce interval

private final int DELAY_BETWEEN_REPLY_IN_MILLISEC = 20 * 1000;
private final int DELAY_BETWEEN_NOTIFICATION_RECEIVED_IN_MILLISEC = 60 * 1000;
private final int DELAY_BETWEEN_REPLY_IN_MILLISEC = 10 * 1000;
private final int DELAY_BETWEEN_NOTIFICATION_RECEIVED_IN_MILLISEC = 50 * 1000;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aweseome @pixincreate ! 😃

private final int DELAY_BETWEEN_NOTIFICATION_RECEIVED_IN_MILLISEC = 50 * 1000;

This variable/member is not named well which might have confused you. What this actually does is to check time between the notifications sent time and actual received time. And if the notification is older than 60 seconds, it will not reply. This prevents replying to older notifications.

This should actually be 60 or more otherwise there is a possibility that it will miss new notifications. If you can, may be change it to 120 as shown below or leave it as is.

private final int DELAY_BETWEEN_NOTIFICATION_RECEIVED_IN_MILLISEC = 120 * 1000;

Otherwise its okay, I will change it and accept the PR.

Thank you for your first contribution 🎉
I will make necessary changes (or you can too) and will test it and merge in a few hours.

@adeekshith adeekshith linked an issue Feb 22, 2021 that may be closed by this pull request
@adeekshith adeekshith changed the title Update NotificationService.java Change minimum time between replies Feb 22, 2021
@adeekshith adeekshith changed the base branch from main to issue/106/reduce-auto-reply-min-sleep-time February 22, 2021 23:35
@adeekshith adeekshith self-requested a review February 22, 2021 23:36
Copy link
Owner

@adeekshith adeekshith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed base to a local branch where the issue with DELAY_BETWEEN_NOTIFICATION_RECEIVED_IN_MILLISEC can be fixed before merging

@adeekshith adeekshith merged commit 5262f68 into adeekshith:issue/106/reduce-auto-reply-min-sleep-time Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-Reply Doesn't work when 2 same messages are received
2 participants