search

flameshot-org / flameshot

Powerful yet simple to use screenshot software :desktop_computer: :camera_flash:
https://flameshot.org
GNU General Public License v3.0
23.92k stars 1.52k forks source link

Add option to disable abort notification #3610

Open j4k0xb opened 1 month ago

j4k0xb commented 1 month ago

Closes https://github.com/flameshot-org/flameshot/issues/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. (https://github.com/flameshot-org/flameshot/issues/3287#issuecomment-1664139462)

image

AlexFolland commented 6 days ago

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 commented 5 days ago

@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 commented 5 days ago

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.