-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix the known issue of saving JPEG to clipboard on macOS #3724
base: master
Are you sure you want to change the base?
Conversation
Hi @mmahmoudian |
@Ariandr thanks for the PR. I'm not quite sold on the idea of using Apple Script to handle this, but perhaps that stems from my lack of familiarity with osascript. Is this a common practice inApple world? I would rather wait for a dev that knows Apple ecosystem more to decide on this. Regardless of this, the code looks clean to me. Thanks again for your valuable contribution and the time you have invested in this. I hope this get merged fast if there is mo objections shit the osascript. |
@mmahmoudian It is also built into the OS, so it does not require any special dependencies or installations on the user machine. Actually, it's a preferred way, because in case the user needs to provide the permission to write to the clipboard, macOS will ask it, because it's Apple's supported tool, while other libraries/ways might not do it. Apple's Automator Mac app is built on using |
Also, while looking for this solution, I've tried some other ways. They (e.g. Qt Clipboard Management) just cannot achieve the same result of saving the compressed JPEG to clipboard. The only alternative that potentially can work is adding Objective-C runtime and Objective-C files to use |
In any case, until a better approach is found (we can leave a comment to investigate alternatives later or investigation welcome), I think it's good enough. The main thing is that it's reliable and doesn't break anything. By default, all macOS users will have it disabled anyway, since the PNG is the default way of saving to clipboard. I'm using the app today extensively, the fix works like a charm. |
Another issue was that my memory usage of Flameshot used to grow fast. In a day of extensive use, it could get to 1-2 GB in memory for whatever reason, while I was taking screenshots and copied them to clipboard as PNGs. So I always killed the app and relaunched it. With this fix, the positive side effect is that the memory footprint stays the same under 100 MB (actually around 20 MB), apparently it's more efficient than saving PNG to clipboard via |
Hi @mmahmoudian So I think macOS users will definitely benefit from this PR. |
This PR works perfectly for me on MacOS. Would be great if it could be merged :) |
@Ariandr thanks for PR. I had a quick look, and it seems this needs the attention of other devs. I've asked them for a review. Let's wait for that. I personally don't have access to mac to test this, and I don't have the fundamental expertise to review this in detail. @isak102 thanks for testing this and providing feedback. It is very helpful. The more eyes we have on the project the better. Thank you. |
@mmahmoudian Status on this? |
@isak102 thanks for the ping, but as I said before:
|
@mmahmoudian maybe this project needs more flexibility in terms of contributions? Main developers seems not much active anymore, the last release was in 2022... A huge community is there willing to submit PRs and they are stuck in review for months. Flameshot is such a great tool, I use it every day - but it really needs some actions taken to avoid dying. |
Agree with @b0ch3nski So it's safer to have it than not. I've been using my built app for months without any issues whatsoever. |
I don't appreciate the tone of your comment, although I agree with some of your points. Here are my unhappy response:
When you start swinging, swing it all the way. In other words, give clear solutions rather than pure complaint. For example: in your opinion, which part is not flexible and how can we create that "flexibility"?
Correct, and the remaining developers didn't feel ready for the next release. I don't see it as an issue. Anyone who wants to live on a bleeding edge can compile it themselves, use nightly builds (as explained on our website), or if on Arch, use AUR. Whenever the devs collectively decide on releasing a new stable version, that would happen, like any other project out there. I personally rather have fewer releases but safer and more stable code than getting hoards of duplicated bug reports from users.
I have said it over and over again, but I say it here one more time as well: anyone can help us review the PRs, literally anyone with a Github account. The more eyes we have on the PRs the better. I actually encourage people to read the code and express their opinion. From catching a typo in a comment to actual code optimizations. That said, I don't see anyone including yourself to be bothered to review the code and approve it or comment on the code. So that "huge community" is either more imaginary than the square root of -1 or can be more kind caring about the code and safety of users and help us by reading the PRs and providing feedback. In some other PRs people have actually helped, but ironically, not in this one.
I 100% agree. I'm doing my share and keeping the project up as the triage and community management guy. But I also need help from the community to either join the project as devs, or help with PRs and reviews, or documentation. In Flameshot we don't accept donations, and all the financial burden is on devs. What the community can do in return is to avoid bad language, avoid sending death threats, and instead help in having a safer and better code base. Don't you agree?
I disagree. Any PR is potentially malicious, so we need the PRs to be approved for the sake of user safety. Yes, you have good intentions and your PR to me is good, but I'm not in the position to merge such PRs without input of other devs unless and until there is no other dev and I'm forced to. So for the time being, I would refrain from unilaterally making decisions on this. I will only merge simple code changes and documentations, as that is my defined role in this project. That said, I will tag a few more people hoping they have time to help reviewing this PR: @FelixJochems @jack9603301 would you please invest some time into this PR and review it. |
@mmahmoudian We are on the technical site and this is purely technical discussion. My comment was not meant to insult anyone but to bring up the attention on a problem which I've been seeing around for quite some time.
There are PRs around on which community had provided the feedback whether the code works/looks good and nevertheless they stay open for months waiting for the confirmation of other devs (which basically not happens). If there are no objections to the implementation, we should get the code in after let's say, 14 days. People like @Ariandr uses their spare time to fix something not just for themselves, but to submit it back to the upstream for everyone else to profit - the appreciation should be shown for them. Keeping PRs like this open for months must be demotivating for others to bring in their own contributions (why doing so if it never gets merged). |
I've checked the code and it doesn't seem malicious. Overall it's better than a non-functioning JPEG clipboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As specified in my comment, I'm not the biggest of using applescript to achieve this. But I don't think we have another option at the moment.
You do realize that what I do and have done for the past 5 years have been done on my spare time without any form of appreciation, right? Same goes for every other dev in this project. None of us got paid for this, and we consciously chose to do this contribution to the FLOSS world. But in return we recieved death threats, yes, literally death threats in our private emails. We got doxxed, we get nagged at. So... what was your point again exactly?? Now let's get back to my point: if you have found this project useful, and you want it to move forward, consider giving something back in form of your time. For example, try running this PR on your machine or VM, and see if it works as expected.
I agree. What can be the solution?
I politely but firmly disagree. The code should be reviewed before getting merged. Period. Unreviewed code = potentially problematic code. I would not bear the responsibility of such code merge, and it would be morally and ethically wrong.
Thank you for investing your time into this, and thanks for the code approval.
Me neither, but considering the comments and update that @Ariandr kindly provided, I would take the leap of faith and assume the code works. @Ariandr let's ping @veracioux and @panpuchkov one more time in case they have any further comments. If we didn't hear back, we will merge in by the end of next week, considering that we now have one dev approved of the PR. Thanks again for the PR, and sorry for the delays. |
@mmahmoudian |
I fixed the known issue while setting
useJpgForClipboard=true
on macOS.I wrote specific macOS function for this case and re-introduced this setting to GUI.
The code is tested and I run the app on my Mac.
The initial issue is here with more context: #3722
For the quick test, you can use the built app with the fix: #3722 (comment)
Results:
The same screenshot in a
.png
is 7.6 MB in size, in a.jpeg
it's a 850 KB with 90 compression quality.