Skip to content
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

Handle commas in filenames #1078

Merged
merged 8 commits into from
Dec 1, 2023
Merged

Conversation

snoyer
Copy link
Contributor

@snoyer snoyer commented Nov 25, 2023

This should work around cxxopts splitting filenames on commas by retrieving the filenames from the unmatched arguments rather than into a vector<string> option.

This comes at the cost of the --input named argument but I'm unclear if having the filenames as both positional and named args is useful, so hopefully it's fine?

The TestHelpPositional test fails as, contrary to what its name suggest, it checks for --input in the help text,
Should it have been looking for file1 file2 ... this whole time? or should it have been named something like TestHelpInputArg in the first place?

edit: after @mwestphal request for backward compatibility:

  • the --input argument is kept but deprecated as it would be a lot of work to make it work with commas
  • the positional filenames are handled through the unmatched arguments instead of parse_positional and commas are ok there

would fix #1070

Copy link

codecov bot commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8641276) 96.42% compared to head (ada3451) 96.43%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1078   +/-   ##
=======================================
  Coverage   96.42%   96.43%           
=======================================
  Files         124      124           
  Lines        7671     7679    +8     
=======================================
+ Hits         7397     7405    +8     
  Misses        274      274           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only small changes required

@mwestphal
Copy link
Contributor

@Meakk ptal

@mwestphal mwestphal requested a review from Meakk December 1, 2023 07:52
Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice workaround, it's a shame we have to do that but I can live with it.

@mwestphal mwestphal merged commit 169e935 into f3d-app:master Dec 1, 2023
45 checks passed
@snoyer snoyer deleted the commas-in-filename branch December 3, 2023 07:48
mwestphal pushed a commit that referenced this pull request Feb 10, 2024
- the `--input` argument is kept but deprecated as it would be a lot of work to make it work with commas
- the positional filenames are handled through the unmatched arguments instead of `parse_positional` and commas are ok there

would fix #1070
mwestphalnew pushed a commit to mwestphalnew/f3d that referenced this pull request Feb 10, 2024
- the `--input` argument is kept but deprecated as it would be a lot of work to make it work with commas
- the positional filenames are handled through the unmatched arguments instead of `parse_positional` and commas are ok there

would fix f3d-app#1070
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arent we suposed to have a filename visible on this baseline ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, no, I got baited by the name of the test, although displaying the filename in the test would kinda make sense.

@Alex-Kent
Copy link

For anyone using an older version on *nix, here's a partial work-around. This can be specified as the program to use to open a file when double-clicking on it from one's file manager. It takes a single argument (the file's path). Adjust f3d's command line options to taste within the script as they can't be passed on the command line. Place the script into e.g. /usr/local/bin/f3d-wrapper

#!/bin/bash

IFS=$(echo -en "\n\b")

d=$( dirname $1 )
f=$( basename $1 )

cd "$d"
f3d "$f" --bg-color=0.1,0.1,0.1 --roughness=0.5 --ssao --tone-mapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comma in filename prevent file to be opened by F3D
4 participants