flameshot-org
Add option to disable abort notification
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)
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.
@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.
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.
Please review and merge this soon. It looks ready and the abort notification is disruptive to regular screenshot workflows, especially when capturing particular moments while gaming. I would prefer to never see an abort notification in my next attempt at the right screenshot after starting a capture at the wrong moment.
I'm reviewing now. Can you help me understand why this notification needs to be treated differently than all other notifications?
I would have preferred if it didn't show this notification and there wasn't a configuration, but another maintainer wanted to keep it backwards compatible: https://github.com/flameshot-org/flameshot/issues/3287#issuecomment-1664139462
I myself also find it a little bit annoying but it disappears fast enough that I never thought of it as an issue. I personally completely agree with making it configurable.
At the moment user can completely turn off all notifications (even the very useful ones):
disabling it by default also means that the current default behavior of Flameshot would change. And we all know how certain people react when something visual changes. 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.
And making all other notifications configurable as well is a bit overkill imo, especially because this PR is more like a temporary workaround that should hopefully not be necessary anymore after the next few versions.
Ok thanks I'm on board. Would you mind rebasing on master to get the CI to run? I think when this was first submitted the CI was broken but its working now. If not I can merge it and fix issues later.