-
-
Notifications
You must be signed in to change notification settings - Fork 475
adds support for time dependent DND mode #602
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
Conversation
Hello @Harkishen-Singh , thank you for working on this. Before reviewing this could you please do the following -
|
@abhigyank , sure, i will put the two functions in a single file. Work Flow On start of the application, the database is check with respect to the dndSwitchOff collection, and if no record is found, a null is returned, which skips the further checks, hence the UI part is not prepended to the actions-container. If the record is found, then it check the nature of the record, which would satisfy if it contains the object containing the 3 parameters: hr, min, carry. The variables hr, min have the function as their names go. However, the carry parameter is included for a particular case; if the user select a time out for the DND mode which lies after the 12 am or 0:00, then the carry is added by 1, and the hour is saved as ( hour - 24 ). The reason or increasing the carry to one is, to pass all the time checks, which are occurring every minute by checkDNDstate function. As soon as the time passes 12am or 0:00, the check.carry is check that if it has the value greater than 0 and the day has changed, then re-initialized back to 0 for normal functional of the program. For the part "handling if a user shuts the app down during a dnd time period" , whenever the user starts the application, a check is made from the main.js renderer process on whether the previous DND mode existed ( will return an object of params: hr, min, carry ) or not ( will return null ). This clarifies, the decision on whether the tag of time left and the per minute time count would be executed or not. Edit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick look through the changes suggests the the logic of implementation is correct and would work correctly.
However, I don't think this is an optimal implementation of the feature as the code doesn't look very clean and could be prone to issues. Also there has to a lot more changes to UI to make it look in coherence to app UI. Some ordering and occurrences in the function calls in the main.js
seems confusing and maybe redundant.
@Harkishen-Singh For other members to review this you should probably add some comments describing the functions and variables use since the implementation seems a bit unusual to me. Also could you squash all the commits into one?
@abhigyank , I have made all the changes suggested. Please have a look, if it meets the requirements. |
@Harkishen-Singh Thanks for working on this. I have not looked deep into the code but I would suggest following changes -
I would suggest discussing the code/implementations/designs etc in https://chat.zulip.org/#narrow/stream/16-desktop and then make changes according to what we decide so that you don't need to make changes every now and then. |
@Harkishen-Singh Thanks for working on this! I have couple of suggestions:
|
@priyank-p , thanks for the suggestions! |
Heads up @Harkishen-Singh, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
Heads up @Harkishen-Singh, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
Closing in favour of #896. Thanks for putting the efforts on this though 👍 |
What's this PR do?

This PR solves #561 . It allows the User to set a time limit for DND mode, which would be switched off, once the time exceeds the limit.
Any background context you want to provide?
This checks every min the current time, and compares with the set of conditions, whether to turn off the DND mode or not.
Screenshots?
GIF ( 9 slides )
You have tested this PR on:
Though, I have tested this feature in Fedora 28, but it is platform independent.