-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Adds Support for Command-Line on Windows #3699
base: master
Are you sure you want to change the base?
Conversation
- removes the preprocessor macro that prevented arg parsing on Windows - adds windows-cli.cpp as src for a wrapper exe - adds flameshot-cli target into cmake when building on Windows - updates README
@mmahmoudian hi, looks like you're the most active maintainer... would you mind giving this a review? |
Did you test if this plays nice in unicode-enabled environments such as Windows Terminal? I have no idea what character sets it passes around, but considering it allows the use of e.g. emoji on the CLI (2+ characters in UTF-8, 1+ on UTF-16) that might be worth checking. I'm asking because CLI invocations can do a lot of stuff (like naming files with disallowed characters) that are caught in graphical environments |
@niwamo sorry, this last comment of yours fell into the cracks and I missed it. My role in this project is mainly community management, triage, and documentation as I'm not as experienced as other devs in C++. I will review to the best of my abilities, but based on an old agreement, I refrain to merge large codes that I might not fully understand the caveats. Nonetheless, I will review it when I get behind a computer. 🙂 Regardless of the results of the review, I would like to thank you for investing your time and effort into this community project. |
I have no solid understanding on C++ development on Windows, so I cannot provide good criticism. From my very limited understanding on Windows C++, and based on you clear explanation and the sources you provided, I believe this wrapper is a good compromise. @jack9603301 @panpuchkov @veracioux what do you think about this approach and this PR. |
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.
@niwamo
It looks clean and simple that I understand the code easily. Thank you. Just one small comment in the README file for extra clarity for users who don't want to inspect the code. After this change, I believe this PR should be ready, and I think it is small and benign enough that I can merge it myself if other devs do not object in the next 30 days.
Thank you again for the PR.
EDIT: I think the @RivenSkaye question is valid. Can you test it with some unicode characters and (e.g emoji) for instance in the filename and see if it behaves nicely? e.g flameshot-cli.exe gui --path 'in-the-spirit-of-🎃.png'
Thanks for the reviews and comments! I had not considered the possibility of emoji characters in CLI args. Right now, they result in silent errors. I will make updates within the next week or so. |
+ support for unicode characters in cli args + upstream changes to README + additional clarification in README re: flameshot-cli + new workaround for spaces in _popen path; works with relative output paths
Not sure how I (or the linter) missed the line ending. It's fixed. |
Description
Adds support for command-line usage / command-line arguments on Windows.
Closes #2118
Approach
I assume this wasn't done previously because Windows apps can be console apps or windows apps, but not both. That leaves two options with a single flameshot executable:
There are some workarounds to mitigate the inconveniences of these approaches, but ultimately, the best solution is two executables.
Good discussions on this topic can be found on Stack Overflow here and here.
This commit adds the windows-cli.cpp source, which compiles into flameshot-cli.exe when built on Windows. It is a minimal wrapper around flameshot.exe but ensures that all stdout is captured and output to the console.
Code Changes
README Updates