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

Plugin for limiting the number of consecutive skips or postpones #526

Merged

Conversation

undefiened
Copy link
Contributor

@undefiened undefiened commented May 28, 2023

I added plugin which allows to limit the number of consecutive times one can postpone or skip break. This is a middle ground between the strict mode which completely disables the skipping/postponing feature, and being able to click skip/postpone indefinitely. There is only one setting which tells how many times in a row one can skip breaks.

When user has skipped X breaks consecutively (without taking any breaks) both skip and postpone buttons become disabled.
Taking break completely (without skipping or postponing) resets the counter.

This should somewhat address #320. At the very least it addresses @tuhlaajapoika suggestion #320 (comment) (I also happened to have ADHD, so what @tuhlaajapoika described was exactly my problem: I was automatically clicking skip without even thinking).

There was no existing mechanism which would have allowed me to remove both buttons without making any changes in UI or core, so I added two flags to "context" and added some logic to UI. The intention behind these two flags is to simply disable both buttons for the next break, after which both flags become reset back to False. Not sure if it is the best solution, so if you have some better mechanisms in mind feel free to let me know and I will do it.

Also, I am not sure about wording of settings and plugin hints, so I would appreciate any suggestions.

@undefiened undefiened changed the title Added plugin for limiting the number of consecutive skips or postpones Plugin for limiting the number of consecutive skips or postpones May 28, 2023
@undefiened
Copy link
Contributor Author

undefiened commented Jun 27, 2023

Dear @hartwork and @slgobinath, I looked at the previous PR's and noticed that you are usually the people who resolve them. I just wanted to mention your usernames to make sure that you are aware of this pull request. There is absolutely no time pressure from my side, I'm just making sure that you are aware of this PR in case you didn't receive any notifications.

@slgobinath
Copy link
Owner

Hi @undefiened ,
Thank you for your contribution. Sorry for the late response I got busy with my life and have little to no time for my FOSS projects.
I didn't get a chance to test it but the code looks good to me. Can you please add an icon suitable for this plugin as described here https://github.com/safeeyes/safeeyes-plugins#best-practices ?

FYI: The new URL to download font awesome icons is https://fa2png.app/

@undefiened
Copy link
Contributor Author

undefiened commented Jul 8, 2023

Added an icon. I didn't find anything suitable, and 24x24 is a rather limiting size, so I did my best. If there are any better ideas, I will be happy to incorporate them.

No worries at all about not having time, it was not my intention to hurry you, I just was not sure how GitHub notification system works. Please take as much time as needed to reply and review. Thank you very much for all your hard work!

And thank you for giving a link to the safeeyes plugins repo, I somehow missed it in the description. Should I move my plugin to the safeeyes-plugins repo? If yes, then I probably should make two separate PRs, one here to modify the core stuff and one in the third-party plugins repo. Or is it fine to have the plugin here?

Also, I removed the copyright with your username from my plugin.py (I have copypasted the header from your other plugins, so it included your copyright). But I can return it back if needed, I don't really understand the logic of licenses and copyrights, I am happily giving up all my copyrights (if I ever had them) or whatever.

@archisman-panigrahi
Copy link
Collaborator

Hi @undefiened, I am a new maintainer, and I will be happy to test and merge this pull request ASAP. Can you please resolve the conflict in the commit?

I am in favor of merging this into main SafeEyes because it would improve the core functionality of SafeEyes.
I feel that the plugins in https://github.com/safeeyes/safeeyes-plugins are not related to the core functionality (Zoom meeting detection, weather forecast, etc).

Also, I removed the copyright with your username from my plugin.py (I have copypasted the header from your other plugins, so it included your copyright). But I can return it back if needed, I don't really understand the logic of licenses and copyrights, I am happily giving up all my copyrights (if I ever had them) or whatever.

Not that I understand how copyrights work either, but I would suggest that you add your own name as the author of this file, and also keep slgobinath's name in case you reused any code written by him.

@undefiened
Copy link
Contributor Author

@archisman-panigrahi Thank you very much for taking over the repo! It is very good to hear that this incredible project will continue to be developed.

I did what you have requested, please let me know is something is not up to standards!

Copy link
Collaborator

@archisman-panigrahi archisman-panigrahi left a comment

Choose a reason for hiding this comment

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

Could not find any bug/regressions. Does what it is supposed to do. I propose merging.

@archisman-panigrahi
Copy link
Collaborator

Unless someone finds any regression, I will merge this on Sunday (and release a new version of Safe Eyes).

@archisman-panigrahi
Copy link
Collaborator

@undefiened I agree that this pull request will fix #320. Thank you very much for your contribution as well as for your patience.

@archisman-panigrahi archisman-panigrahi merged commit 3df23f1 into slgobinath:master Jul 14, 2024
1 check passed
@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 14, 2024

This is super helpful. Thanks again! The latest version includes this PR.

@archisman-panigrahi
Copy link
Collaborator

@undefiened I am in favor of enabling this plugin in the default installation (with 2 allowed skips). Do you know how to do that?

@undefiened
Copy link
Contributor Author

@undefiened I am in favor of enabling this plugin in the default installation (with 2 allowed skips). Do you know how to do that?

I made a PR that seems to do that - #603

Although, as I wrote there, I am not sure whether it is a good idea because this plugin introduces a real blocking behavior by default.

@undefiened
Copy link
Contributor Author

This is super helpful. Thanks again! The latest version includes this PR.

Thank you very much! Now I can start using upstream SafeEyes again :)

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.

3 participants