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

Improve batch file to convert GOG cp437 config files to UTF-8 #3042

Merged
merged 1 commit into from Oct 23, 2023

Conversation

Kappa971
Copy link
Contributor

@Kappa971 Kappa971 commented Oct 22, 2023

Description

Describe a summary of your changes clearly and concisely, including motivation and context.
Breaking changes need extra explanation on backward compatibility considerations.

Feel free to include additional details, but please respect the reviewer's time and keep it brief.

More "advanced" version of this: #3031
It probably could be written better but that's the best I can do, sorry.
If you want to test it, I share the complete package: conf437-utf8.zip

There is also a simpler alternative version: #3031 (comment)

If you don't like it, you can close the PR.

Manual testing

Without recursive (no /R):

  • convert single CP437 conf in given directory
  • convert multiple CP437 confs in given directory
  • convert UTF-8 conf in given directory (it should skip it)
  • convert multiple UTF-8 confs in given directory (it should skip them)
  • convert mixed CP437 confs with some UTF-8 confs in given directory (it should skip the UTF-8s but convert the CP437s)

With recursion (/R):

Same as above test, but with confs inside nested directories under the given path:

  • convert single CP437 conf inside nested directory
  • convert multiple CP437 confs spread in nested directories
  • convert UTF-8 conf inside nested directory (it should skip it)
  • convert multiple UTF-8 confs inside nested directories (it should skip them)
  • convert mixed CP437 confs with some UTF-8 confs inside nested directories (it should skip the UTF-8s but convert the CP437s)

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@Kappa971 Kappa971 added enhancement New feature or enhancement of existing features Windows Issues related to Windows labels Oct 22, 2023
@Kappa971 Kappa971 requested a review from kcgen October 22, 2023 23:51
@Kappa971 Kappa971 self-assigned this Oct 22, 2023
@kcgen
Copy link
Member

kcgen commented Oct 23, 2023

It probably could be written better but that's the best I can do, sorry.

As long as the script:

  1. Adds value for general users (and GOG users are a big group!),
  2. Is clearly described and tested (does what it says it does),
  3. Doesn't cause harm,

Then that's sufficient for inclusion in the community contribution area.

No need to apologize; this is open-source, so taking the initiative to move the bar forward in an area is all that's needed. If someone else has a better implemention, then they're free to add/improve it.

I've listed some test cases in the PR text body. Can you setup those scenarios and run them?

Then simply click the check-box to "check" them when you've confirmed.

@Kappa971
Copy link
Contributor Author

Kappa971 commented Oct 23, 2023

  • convert single CP437 conf in given directory

The batch file can convert a single dosbox*.conf file (both with and without /R) but only if there is one file in that folder.
Currently you can't specify a single file (and I don't think it's necessary), the batch file will convert those starting with dosbox with .conf extension and that are not UTF-8 (assuming that the source file is cp437, this is implied because the GOG conf files are probably cp437).

You mean this?

EDIT
Ok I think the sentence means "given a directory, if there is only one CP437 dosbox*.conf file, does the batch convert it?" Yes

@Kappa971
Copy link
Contributor Author

If @OpenRift412 could give me some feedback, it would be helpful.

@OpenRift412
Copy link

What does this version do differently than the previous one?

@Kappa971
Copy link
Contributor Author

What does this version do differently than the previous one?

Now it has two search methods to convert conf files, and you don't have to copy the config files to the batch program folder:

  1. Normal search. You give it a game directory (e.g. D:\DOS_GAMES\DOOM) and the batch will convert all files starting with "dosbox" with a ".conf" extension (but only those that are not UTF-8);
  2. Recursive search. It does the same thing, but in this case you can give it the root directory (e.g. D:\DOS_GAMES) and it will convert the conf files of all GOG games contained in that directory.

I've tested it and it works but if you could give me some feedback, it would be helpful (maybe I missed something).

@kcgen
Copy link
Member

kcgen commented Oct 23, 2023

With all those test cases confirmed, we're safe to merged.

Then @OpenRift412 will be able to test it in the latest dev build.

@kcgen kcgen merged commit 32dabd0 into main Oct 23, 2023
50 checks passed
@delete-merged-branch delete-merged-branch bot deleted the kappa971/gog-cp437-to-utf8-batch-2 branch October 23, 2023 21:59
@OpenRift412
Copy link

What does this version do differently than the previous one?

Now it has two search methods to convert conf files, and you don't have to copy the config files to the batch program folder:

  1. Normal search. You give it a game directory (e.g. D:\DOS_GAMES\DOOM) and the batch will convert all files starting with "dosbox" with a ".conf" extension (but only those that are not UTF-8);
  2. Recursive search. It does the same thing, but in this case you can give it the root directory (e.g. D:\DOS_GAMES) and it will convert the conf files of all GOG games contained in that directory.

I've tested it and it works but if you could give me some feedback, it would be helpful (maybe I missed something).

I tested it and it seems to work alright. Don't think you missed anything. I'll let you know if I run into any more problems.

@johnnovak johnnovak added the localisation Issues related to localisation and internationalisation label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement of existing features localisation Issues related to localisation and internationalisation Windows Issues related to Windows
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants