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

-txtout command line action #2288

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

bbbradsmith
Copy link
Contributor

@bbbradsmith bbbradsmith commented Dec 11, 2024

Edited Summary

  • Add -txtout command line option to export text file.
  • Improve empty error message from command line output fopen failures (using strerror(errno)).
  • Use ps_fopen instead of fopen for command line output, permitting unicode filenames on Windows.
  • Consolidate boilerplate command line output text checks into a single boolean.
  • Remove unimplemented -zsmout command from documentation.

Original explanation

Add -txtout to allow command line export to text file. The desire for this feature is so that I can use it for export into a game engine's needed format, because the text export is easy to parse. I had written the text export feature for Famitracker years ago, and found it extremely useful in this way.

Included one other commit noting that -cmdout and -vgmout were giving an empty error if fopen fails, assuming it was intended to output the filename instead.

An aside note, I see -zsmout listed in the documentation, but it does not appear to exist in the code. Is this a mistake?

Copy link
Owner

@tildearrow tildearrow left a comment

Choose a reason for hiding this comment

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

Very good - just one small change needed.

src/main.cpp Outdated Show resolved Hide resolved
@bbbradsmith
Copy link
Contributor Author

Okay, that change has been made.

Copy link
Owner

@tildearrow tildearrow left a comment

Choose a reason for hiding this comment

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

Whoops

src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@bbbradsmith
Copy link
Contributor Author

bbbradsmith commented Dec 12, 2024

Are you sure about this one? I made that change because I tested the error case there and the onscreen message looks like could not open file! (). An error in fopen does not seem to do anything to e so e.getLastError() returns an empty string.

Could you test that error and verify that this is really what you want? You can trigger it by using a path to a nonexistent folder, or write protecting a pre-existing file with that name.

Though if you are looking for an error string and not just the filename, we might consider using fopen's reported error via strerror(errno) for that %s?

Though I have one other question about whether these should be using ps_fopen instead of fopen, as is done in gui.cpp's export functions. It seems that our command line is already parsed into UTF8, so wouldn't this be the correct way to support unicode paths from the command line?

@bbbradsmith
Copy link
Contributor Author

bbbradsmith commented Dec 12, 2024

By using strerror(errno) as suggested above, the onscreen error now looks like could not open file! (No such file or directory) or could not open file! (Permission Denied) (read-only). I'm hoping this is the revision you are really asking for, because e.getLastError() only returns "" in these cases.

I have also switch fopen to ps_fopen and verified that this is required to use unicode files from the command line on Windows.

I also reduced the boilerplate test of all four output strings with a single boolean for convenience.

I also removed -zsmout from the documentation because this command does not exist.

Outside the scope of this PR, I would imagine that what we really want instead of -zsmout is a -romout command, though I see that the ROM exports have additional options which creates an additional want to customize those. Maybe that could be obviated if the export options were saved in the .fur file, allowing the user to specify them there instead? Anyway, I won't try to implement it in this PR because ROM export seems like a more complicated operation.

@tildearrow
Copy link
Owner

Are you sure about this one? I made that change because I tested the error case there and the onscreen message looks like could not open file! (). An error in fopen does not seem to do anything to e so e.getLastError() returns an empty string.

Could you test that error and verify that this is really what you want? You can trigger it by using a path to a nonexistent folder, or write protecting a pre-existing file with that name.

Never mind then.

Though if you are looking for an error string and not just the filename, we might consider using fopen's reported error via strerror(errno) for that %s?

Yeah. Sounds like a good idea.

Though I have one other question about whether these should be using ps_fopen instead of fopen, as is done in gui.cpp's export functions. It seems that our command line is already parsed into UTF8, so wouldn't this be the correct way to support unicode paths from the command line?

Indeed. You should use ps_fopen. fopen() on Windows expects an "ANSI" string, so it won't work if there are non-ASCII chars. ps_fopen() converts the string to UTF-16 ("Unicode") and then calls _wfopen.

@tildearrow
Copy link
Owner

By using strerror(errno) as suggested above, the onscreen error now looks like could not open file! (No such file or directory) or could not open file! (Permission Denied) (read-only). I'm hoping this is the revision you are really asking for, because e.getLastError() only returns "" in these cases.

Yes, it is. Thank you!

I have also switch fopen to ps_fopen and verified that this is required to use unicode files from the command line on Windows.

Exactly.

I also reduced the boilerplate test of all four output strings with a single boolean for convenience.

I also removed -zsmout from the documentation because this command does not exist.

Outside the scope of this PR, I would imagine that what we really want instead of -zsmout is a -romout command, though I see that the ROM exports have additional options which creates an additional want to customize those. Maybe that could be obviated if the export options were saved in the .fur file, allowing the user to specify them there instead? Anyway, I won't try to implement it in this PR because ROM export seems like a more complicated operation.

Yeah. I have adapted ZSM export to the ROM export framework, so -zsmout is a leftover.
We'll deal with command line ROM export at a later time.

@tildearrow tildearrow merged commit bcff1f0 into tildearrow:master Dec 13, 2024
5 checks passed
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.

2 participants