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

Enabling/disabling Mini/Long Breaks resets the breaks #601

Closed
wants to merge 1 commit into from
Closed

Enabling/disabling Mini/Long Breaks resets the breaks #601

wants to merge 1 commit into from

Conversation

balazsnasz
Copy link
Contributor

Issue: #503

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • issue was opened to discuss proposed changes before starting implementation.
  • during development, node version specified in package.json was used (ie using nvm).
  • package versions and package-lock.json were not changed (npm install --no-save).
  • app version number was not changed.
  • all new code has tests to ensure against regressions.
  • npm run lint reports no offenses.
  • npm run test is error-free.
  • README and CHANGELOG were updated accordingly.

Description of the Change

Enabling/disabling Mini/Long Breaks resets the breaks (#503)

Verification Process

Other information

@hovancik
Copy link
Owner

Let me check because there are few possible places to do this ;]

@hovancik
Copy link
Owner

hovancik commented Jun 18, 2020

I think I would like to have more complex solution. First thoughts:

  • it should be in main.js ipcMain.on('save-setting', function (event, key, value) { where I do specific stuff for specific settings, so it's kept in same place as other special things
  • I think we should probably have special class in utils to handle changes, so that we can test it, depends how much code it ends up having (and if there's even a way how we could test it), or maybe in scheduler? I don't know

Some quick pseudo (could be some methods that we call in save-settings)

- when enable/disable break is changed
  - check if next scheduled is enabled  
    and remove from scheduler, schedule next enabled if needed
- when enable/disable notification is changed
   - check what's next and schedule next break with/without notification

with scheduling new break I would like to consider how much time has already passed, so if next scheduled break is Mini in 5 minutes, and we are disabling notification, it will be in 5 mins but without notification

What do you think?

@hovancik hovancik closed this Jul 18, 2020
@balazsnasz
Copy link
Contributor Author

Are you sure you want to make this so complex?
Change such settings are not a too regular thing.

@hovancik
Copy link
Owner

Well, I am not sure :D But I think it's expected.

Let's say when I am 1 minute to Mini and I disable Long, it's unexpected that breaks are reset.

@balazsnasz balazsnasz deleted the reset-breaks branch July 26, 2023 23:56
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.

2 participants