flameshot icon indicating copy to clipboard operation
flameshot copied to clipboard

Adds Support for Command-Line on Windows

Open niwamo opened this issue 1 year ago • 1 comments

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:

  1. A GUI app that respects CLI arguments but never returns any text to console
  2. A console app that always pops up a console window, even when started with a double-click

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

  • The preprocessor macro that prevented arg parsing on Windows has been removed from main.cpp
  • windows-cli.cpp has been added as the source for flameshot-cli.exe
  • The flameshot-cli target has been added to cmake (only when building on Windows)
  • README updates

README Updates

Usage on Windows

On Windows, flameshot.exe will behave as expected for all supported command-line arguments, but it will not output any text to the console. This is problematic if, for example, you are running flameshot.exe -h.

If you require console output, run flameshot-cli.exe instead.

niwamo avatar Aug 26 '24 02:08 niwamo

@mmahmoudian hi, looks like you're the most active maintainer... would you mind giving this a review?

niwamo avatar Sep 05 '24 16:09 niwamo

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

RivenSkaye avatar Oct 23 '24 08:10 RivenSkaye

@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.

mmahmoudian avatar Oct 23 '24 20:10 mmahmoudian

Windows apps can be console apps or windows apps, but not both. [...] 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.

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.

mmahmoudian avatar Oct 24 '24 07:10 mmahmoudian

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.

niwamo avatar Oct 24 '24 22:10 niwamo

Just added a commit with:

  • support for Unicode characters in CLI args
  • the requested additional clarification in the README
  • a new workaround for space-containing paths in _popen (the new workaround ensures expected behavior when using relative file paths for output files)

image

niwamo avatar Oct 29 '24 02:10 niwamo

Not sure how I (or the linter) missed the line ending. It's fixed.

niwamo avatar Nov 22 '24 18:11 niwamo

@borgmanJeremy what do you think about this PR?

mmahmoudian avatar May 28 '25 07:05 mmahmoudian

@niwamo sorry for the delay I had been away from the project for some time. From a review standpoint this looks good to me as long as its okay / customary to have two different binaries on windows. Would you be able to rebase this PR on master to get the CI to run? I'd like to make sure both binaries get packaged properly.

borgmanJeremy avatar May 28 '25 11:05 borgmanJeremy

Thanks for following up on this. I've rebased on the latest master commits, adjusted the flameshot.exe path construction, and verified locally that the binaries still build and function properly on Windows.

(You'll see a force push over my own commits - at some point last year, I had somehow incorporated unrelated README changes from another PR)

niwamo avatar May 29 '25 22:05 niwamo