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

Launcher and wizard ampersand escape support #1502

Merged
3 commits merged into from Oct 16, 2017

Conversation

Projects
None yet
2 participants
@AnyOldName3
Copy link
Member

AnyOldName3 commented Oct 15, 2017

Before this pull request:

  • The launcher wouldn't display .es[pm] files in folders whose path contained an ampersand (and may or may not have deactivated them), even though they would load fine in OpenMW itself.
  • The installation wizard would produce an invalid openmw.cfg file when selecting an install in a path containing an ampersand character, as discovered by this user: https://forum.openmw.org/viewtopic.php?f=8&t=4676

This PR handles the escaping of quotation marks and ampersands in data=... lines more in line with how boost::program_options does, meaning the launcher and wizard produce config files which the engine and editor can read correctly and can read these config files correctly themselves.

With this pull request:

  • The launcher displays all content files which the engine and editor can actually see, and definitely won't erroneously deactivate any (although I didn't check if it was doing that in the first place anyway).
  • The installation wizard produces a valid openmw.cfg file when processing an installation with an ampersand in the path.

I'm still salty about the decision to use two different systems to handle interaction with the configuration files, but at least they should have achieved parity now.

I submit this to the mercy of Travis and AppVeyor and hope my offering pleases them more than my last one.

@AnyOldName3

This comment has been minimized.

Copy link
Member Author

AnyOldName3 commented Oct 15, 2017

Also, this should theoretically make it possible for users to load content from paths that they've somehow managed to get a quotation mark into. On Windows, this isn't an option, and I've not got a quick way of testing such a niche use-case as I've not got my Ubuntu laptop handy.

@ghost ghost merged commit 7f8d996 into OpenMW:master Oct 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Aesylwinn

This comment has been minimized.

Copy link
Contributor

Aesylwinn commented Oct 17, 2017

Just a heads up, this seems to have broken the recognition of tokens like "?userdata?" for both the game and the CS, on linux at least.

@Aesylwinn

This comment has been minimized.

Copy link
Contributor

Aesylwinn commented Oct 17, 2017

The tokens are supposed to be filled in with the relevant path, but they are not.

@AnyOldName3

This comment has been minimized.

Copy link
Member Author

AnyOldName3 commented Oct 17, 2017

The function in charge of that is ConfigurationManager::processPaths(Files::PathContainer& dataDirs, bool create) in https://github.com/OpenMW/openmw/blob/master/components/files/configurationmanager.cpp and I'm pretty sure that still does what it did before except now it doesn't remove quotes from paths as it shouldn't have to. Nothing I did should have broken it. I'll try and get a dev environment set up in an Ubuntu VM so I can test this.

Just in case something else has somehow broken this, have you done a before and after test with this PR?

@Aesylwinn

This comment has been minimized.

Copy link
Contributor

Aesylwinn commented Oct 18, 2017

Just in case something else has somehow broken this, have you done a before and after test with this PR?

Yes, it first occurs here. Perhaps when escaping, the token is getting changed to \"datadir\" or something?

@AnyOldName3

This comment has been minimized.

Copy link
Member Author

AnyOldName3 commented Oct 18, 2017

It looks like it only broke the ?userdata? token specifically, and not because of any of the reasons I initially thought. I'm submitting another PR to fix it.

@AnyOldName3

This comment has been minimized.

Copy link
Member Author

AnyOldName3 commented Oct 18, 2017

PR is here: #1507

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.