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

NWN2: Library and tool to fix NWN2 XML files #24

Closed
wants to merge 29 commits into from
Closed

Conversation

@rjshae
Copy link
Contributor

@rjshae rjshae commented Sep 24, 2018

This pull provides the class XMLFix, which can accept a
SeekableReadStream containing the contents of a NWN2 XML file and
return a SeekableReadStream with the XML in a valid form. This can
be exercised with the included fix2xml.exe build, which reads in a
NWN2 XML file and outputs a corrected XML file. The latter tool was
used to bulk test the stock NWN2 XML files and the XML files from
some NWN2 mods. The result was processed with a call to
'xmllint --noout filename.xml' to check for valid XML. All of the
converted test files passed validation.

The mods used to test the class were:

Tchos' HD UI panels and dialogue compilation and expansion
SlimGUI
Black UI
Kaldor Silverwand Context Menu Additions
I manually checked a sample of the output files looking for
irregularities, and these have been corrected.

The code required the addition of an at() call to the UString class
and a modification of the UString::trim* functions to check for
whitespace characters rather than just ' '.

Finally, it includes a unit test for producing valid XML output.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Sep 25, 2018

From the outside, that looks far better, yes :)

I can look at it in more depth in the evening

Copy link
Member

@DrMcCoy DrMcCoy left a comment

Yup, this PR is now far more what I was looking for. Thanks! :)

There's just a few style things to fix, and the line-ending thing (i.e. you've commited everything with Windows line endings, while we use Unix line endings).

And another thing:

The only state the XMLFixer class keeps around is the button counter, right? Would it be possible to move that counter completely into the function scope, and just pass it around as a parameter to isBadUIButtonRange()?

That way, you could make fixXMLStream() completely static, and the fixer wouldn't need an XMLFixer instance at all. You'd just call XMLFixer::fixXMLStream() and be done.

src/common/ustring.h Outdated Show resolved Hide resolved
src/aurora/xmlfixer.cpp Outdated Show resolved Hide resolved
src/aurora/xmlfixer.cpp Outdated Show resolved Hide resolved
src/fix2xml.cpp Outdated Show resolved Hide resolved
src/aurora/xmlfixer.cpp Outdated Show resolved Hide resolved
src/aurora/xmlfixer.cpp Outdated Show resolved Hide resolved
src/aurora/xmlfixer.cpp Outdated Show resolved Hide resolved
src/aurora/xmlfixer.cpp Outdated Show resolved Hide resolved
src/aurora/xmlfixer.h Outdated Show resolved Hide resolved
src/aurora/xmlfixer.h Outdated Show resolved Hide resolved
Create a class capable of reading and writing a SeekableReadStream.
This will be used to build a class that can fix broken, non-standard
NWN2 XML files.
@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Sep 28, 2018

Thanks. I've tried to address your concerns and I added a man page.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Sep 30, 2018

Yeah, that looks better now :)

However, "Common:" in the commit message of 4dc5666 should be "COMMON:".

Also, why did you name the tool fix2xml? Shouldn't it be rather fixnwn2xml?

Also also, I'd rather have the default output of the tool, when no output name is given, be stdout instead of a file with _Fixed appended. We have a class, StdoutStream, for that.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Sep 30, 2018

Oh, and that reminds me: it would be nice if the tool could read from stdin as well. If no input file name is given, for example.

We do have a StdinStream class for that... However, you can't, of course, seek the stdin, so StdinStream inherits from ReadStream, not SeekableReadStream.

Skimming over the code, you don't necessarily need to seek, right? The only thing you call is Common::readStringLine(), which does require a SeekableReadStream. But looking at that code, it and readFakeChar() don't really need to seek either, so they could be dropped to use ReadStream.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Sep 30, 2018

Though, that might be something for another PR.

Ignore that last comment of mine for now, only focus on the one before :)

@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Sep 30, 2018

Everything's done except the write to stdout. I'm a little stumped there:

void convert(Common::UString &inFile, Common::UString &outFile) {
	// Read the input file into memory
	Common::ScopedPtr<Common::SeekableReadStream> in(Common::ReadFile::readIntoMemory(inFile));
	Common::ScopedPtr<Common::WriteStream> out(openFileOrStdOut(outFile));

	// Filter the input
	Common::SeekableReadStream *fixed = Aurora::XMLFixer::fixXMLStream(*in);

	// Write to output
	out.writeStream(fixed);
	out.flush();
	out.close();

	if (!outFile.empty())
		status("Converted \"%s\" to \"%s\"", inFile.c_str(), outFile.c_str());
}

Gives:

$ make
make  all-am
make[1]: Entering directory '/home/rjhall/xoreos-tools'
  CXX      src/fixnwn2xml.o
src/fixnwn2xml.cpp: In function 'void convert(Common::UString&, Common::UString&)':
src/fixnwn2xml.cpp:101:6: error: 'class Common::ScopedPtr<Common::WriteStream>' has no member named 'writeStream'
  out.writeStream(fixed);
      ^~~~~~~~~~~
src/fixnwn2xml.cpp:102:6: error: 'class Common::ScopedPtr<Common::WriteStream>' has no member named 'flush'
  out.flush();
      ^~~~~
src/fixnwn2xml.cpp:103:6: error: 'class Common::ScopedPtr<Common::WriteStream>' has no member named 'close'
  out.close();
      ^~~~~
make[1]: *** [Makefile:5042: src/fixnwn2xml.o] Error 1
make[1]: Leaving directory '/home/rjhall/xoreos-tools'
make: *** [Makefile:3047: all] Error 2

Even though I see it in the WriteStream call in the WriteStream header. Ah well, I'll take a look later. Other projects call.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Sep 30, 2018

The ScopedPtr has no writeStream. The containing WriteStream has. So you need to use out->writeStream()

@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Oct 21, 2018

Hello DcMcCoy,

I believe I've addressed your concerns. Is there some other hold up? Thanks.

@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Oct 21, 2018

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Oct 21, 2018

Yes, I'm sorry, I just didn't have enough time to properly look at your PR. The hold-up is me.

From a cursory look, it really seems to be great now (thanks again!), but I still want to go through all the commits one by one.

I see if I can do that tomorrow evening, and possibly merge this PR then too.

@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Oct 22, 2018

Ah, okay. No problem then; whenever you can get to it. Thank you.

@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Oct 22, 2018

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Oct 28, 2018

Yeah, this looks pretty good now, thanks :)

How do you want to be credited in the AUTHORS file? The rough format I've been using is Name (Nickname) <email adress>, though all fields are basically optional.

Since you do have your name (and email address as well, of course) in the commits, I'd use Bob Hall (rjshae) and said email address, unless that's not what you want.

@rjshae
Copy link
Contributor Author

@rjshae rjshae commented Oct 28, 2018

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Oct 28, 2018

Okay, merged as 41aaf31...d641a56, thanks! :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants