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

Add option to disable abort notification #3610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

j4k0xb
Copy link

@j4k0xb j4k0xb commented May 14, 2024

Closes #3287

It's my first PR here so not too familiar with the codebase, hope the way of excluding the logger target is how it's intended to be used (still logs to files/stderr).
I ran clang-format and tested it on Linux.

I think it should be Enabled by default, at least for the next few versions, and then we can ask the community how they feel about it. This way the backlash would be minimal.
(#3287 (comment))

image

@AlexFolland
Copy link

I'm not a maintainer, but I am a programmer and this looks great to me. I read the code changes and they make sense. The logging is preserved and only the notification is toggled by this option.

Is there any reason not to merge this at this point? It makes the software better and from my review of the code, I don't see how it could introduce any regressions.

@mmahmoudian
Copy link
Member

@j4k0xb thanks for implementation, and @AlexFolland thank for reviewing the code. I'm a maintainer but I mostly do triage. We should wait for other devs to review and merge this. I typically only merge documentation or minor code changes, and this PR contains 33 lines of code across 6 files.

@j4k0xb and @AlexFolland it would be a great help if you can give us your opinion on other PRs. The more eyes we have on the codebase, the safer the users will be and the better code quality we would have. I appreciate every bit of help.

Thank you again for the PR and for the review.

@AlexFolland
Copy link

I'm not deeply familiar with the code base, but even I can tell that this particular PR is just a bunch of settings-related boilerplate and documentation, plus the one change that actually interprets and uses the setting's value. It's a very simple change, so that's why I could easily claim to have reviewed it.

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.

Option to disable the "screenshot aborted" notification
3 participants