Skip to content

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

Closed
wants to merge 16 commits into from

Conversation

Harkishen-Singh
Copy link

@Harkishen-Singh Harkishen-Singh commented Dec 1, 2018


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 )
webp net-gifmaker

You have tested this PR on:
Though, I have tested this feature in Fedora 28, but it is platform independent.

@abhigyank
Copy link
Collaborator

Hello @Harkishen-Singh , thank you for working on this. Before reviewing this could you please do the following -

  • Remove the package-lock file from the PR (it shouldn't be required in this PR).
  • functions showDNDTimeLeft and checkDNDstate are repeated in two files. I'd suggest to put it in one file, preferaby in dnd-util and use from there (I haven't read their workings but I feel it shouldn't be require in both the places).
  • Also could you give a brief workflow of how you have implemented the feature? Specially I am curious how you are handling if a user shuts the app down during a dnd time period.

@Harkishen-Singh
Copy link
Author

Harkishen-Singh commented Dec 1, 2018

@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.
The sleep() has been used so as to give a good UI feel, and also to prevent some async conflicts.

Edit 1
Removed the package-lock.json which was added in a mistake in previous commits.

Edit 2
Added Cancel button, Minor UI changes
cancel

Copy link
Collaborator

@abhigyank abhigyank left a 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?

@akashnimare akashnimare requested a review from priyank-p December 2, 2018 09:29
@Harkishen-Singh
Copy link
Author

Harkishen-Singh commented Dec 2, 2018

@abhigyank , I have made all the changes suggested. Please have a look, if it meets the requirements.
For the UI changes, please describe for further refinement.

@akashnimare
Copy link
Member

@Harkishen-Singh Thanks for working on this. I have not looked deep into the code but I would suggest following changes -

  • Document the code/functions etc so that others can see what's going on
  • Try to use the appropiate class/function's name + clean up the code
  • We mostly use ES6 (arrow functions etc) so you should follow the same
  • This needs heavy changes in the design

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.

@priyank-p
Copy link
Member

priyank-p commented Dec 2, 2018

@Harkishen-Singh Thanks for working on this! I have couple of suggestions:

  1. I think creating the whole html for this feature in javascript, specially per element and with the lengthy and complex logic you have in the makeView function, is wrong way to do it. You should instead either include the html in the html file or if you really think it needs to be created in js side you should look at how we do it at webview component which is clear way to do it and easier to review.

  2. Also, for the checking the time to turn off this feature, you currently have very complex logic that is hard to comprehend hence as mentioned very error-prone. I suggest you use something like node-schedule to schedule the turn off of dnd and save the end time in the JsonDB and remove the record once the dnd is turned off. And to handle computer shutdown and turn off cases you should check the saved record on app startup and if there are records you can handle the edge cases there.

  3. As a sidenote you have css classes that are in camel case, we use hypen case so you should rename you css classes accordingly like, for example, .dnd-input instead of .dndInput.

@Harkishen-Singh
Copy link
Author

Harkishen-Singh commented Dec 2, 2018

@priyank-p , thanks for the suggestions!
There are few things to note:
First, the View part of the Time List is in dynamic way so as to give a more emphasis on the DOM control, which could be done very effectively, if the entire is based in js. Further, it would also allow more space for addition of more features in the same view dynamically in a easy manner by the future contributors. Still, if that's useful, the html way, i will surely do that way.
Second, the Time checking Logic is working in a plain simple way, it extracts the time from the database (if existing) and converts that into the string format, and concatenates the hr with the min. This makes it very easy to compares the two time moments, by converting into the Integer format. The same is done with the present current time during the comparison. I had actually used node-schedule, but later felt not necessary, since that would be an addition of the size to the final electron-package after the builds as that could be solved by mere converting to the string type as described above.
Third, I would make those css class changes in the next commit, once the UI is confirmed.
Suggestions are most welcomed!

@zulipbot
Copy link
Member

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 upstream/master branch and resolve your pull request's merge conflicts accordingly.

@zulipbot
Copy link
Member

zulipbot commented Apr 2, 2019

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 upstream/master branch and resolve your pull request's merge conflicts accordingly.

@akashnimare
Copy link
Member

Closing in favour of #896. Thanks for putting the efforts on this though 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants