Byte buffer

[philjord]

Dexesttp, I've done a tiny branch that just swaps out the RandomAccessFile for ByteBuffer.

This is so I can use the parser in my code, because I pull data from within nif files and also from zip streams out of the bsa and ba2 files.

The start point for the Reader and Writer still accept File and just swap to ByteBuffer quickly.

I used TestView on a few files and it seemed to read in and out fine, however the second test of reading the output xml in and creating an hkx file seems to have trouble reading the xml file (it only reads part) but this is totally untouched by my change so perhaps this is expected. That is to say the xml reading and DOM parsing is still File based.

If you want any changes or more rigorous testing I'm happy to update.

[Dexesttp]

Nice stuff :) I'll test it right now, but I have a few remarks about the actual code.

Commented functions

The close() functions you just commented out won't make it into the master branch. I am trying to keep the code clean, and I'd prefer if you deleted the function's content and simply added @deprecated to the function's JavaDoc.

e.g. go from your current

    /**
     * Close this {@link Data2Interface} connection with the {@link File}.
     * @throws IOException
     */
    /*public void close() throws IOException {
        file.close();
    }*/

to a cleaner

    /**
     * Close this {@link Data2Interface} connection with the {@link File}.
     * @throws IOException
     * @deprecated
     */
    public void close() throws IOException {
    }

The same thing is to say for commented code in the functions itself. Versionning should take care of keeping old code for you :)

Changed JavaDoc

In a few cases, you changed the behavior of a function without documenting the new behavior. I'm thinking about this part especially, but it is also about any part where you change parameters without changing the relevant JavaDoc (just updating the File to ByteBufferis often enough).

Non-final attributes

I've seen it only in one case, here, but some attributes may be final where they aren't. I try to keep all parameters intended to not change after the constructor final, in order to prevent future devs from changing them when they don't really want to.

Merging to DexesTTP:master

I'm not quite fond of this idea, as a merge to master is usually for actual release and should go through its own pull request, accompanied with a version change. However, if you think this is a good idea, I'd be happy to hear your arguments and I would allow it if it's justified.

In any case, if you really want to merge to master you'll need to :

• Update the version number to reflect a version change
• Provide a change overview in a similar format to the beta pull request/releases.

---

I'll come back after the actual test, to see if it broke anything. The issue about partial XML reading is weird though, so I'll take a look.

edit : added stuff about the branch you merge into

[Dexesttp]

Ok, so results of the tests.

The actual extraction doesn't seem to have been affected. I could reproduce your issue while reading XML files though, so I believe it may have been introduced somehow by your code (it doesn't happen on /dev). I'm checking what could have happened.

edit : it appears to be an issue with HKX writing back, not XML reading. I'm still searching for the issue root.

code comments

File and ByteBuffer

I don't really agree with the way you added ByteBuffer support alongside File support inside HKXReader.java. A better way to do it would have been to resolve the HKX file in the constructor.

    public HKXReader(File hkxFile, HKXDescriptorFactory descriptorFactory, HKXEnumResolver enumResolver) throws IOException {
        RandomAccessFile raf = new RandomAccessFile(hkxFile, "rw" );
        this.hkxBB = raf.getChannel().map(MapMode.READ_WRITE, 0, hkxFile.length());
        this.descriptorFactory = descriptorFactory;
        this.enumResolver = enumResolver;
        raf.close();
    }

For HKXWriter.java, this part

outputBB = ByteBuffer.allocateDirect(10000000);//10 meg max should be fine, it'll only use what it needs

may also require a way to manually set this value. Files bigger than 10 Mb are unlikely, but if it happens this would be hardcoded into the core, which is something I'm not fond of.

It would also be nice to allow to set directly the Byte Buffer as constructor of HKXWriter, for consistency with HKXReader's behavior.

[Dexesttp]

Ok, I found the source of the HKX write error.

The last section written to the buffer is the data section, in the header. Therefore, flip() stores its position as the file's limit. Then, when using the limit() command, it gives the position of the header end.

Therefore, the proper way to handle limit storage would be as follows :
HKXWriter.java, line 99

dataHandler.fillPointers(data, resolver);
outputBB.flip();
// Write the data section to the file.

HKXWriter.java, line 109

outputBB.position(0);
byte[] bytes = new byte[outputBB.remaining()];
outputBB.get(bytes);
out.write(bytes);
out.close();

There's also some places where there's extra imports. If you're using Eclipse, use ctrl+shift+O to solve them

[philjord]

Ok thanks for the excellent and detailed response, that was amazingly fast and made my rework extremely simple.

Obviously I agreed with all your suggestions, with these notes:

The non-final variables you mentioned here gives an error in eclipse saying that the other might not be initialized(as they are either or) so I deliberately made them non final.

I made a public void setByteBuffer(ByteBuffer outputBB) so the temp buffer for writing can be controlled if required, but I didn't add it to the constructor as it seemed like a edge case and not "normal" as it were here

Finally, I'm not familiar with repo collaboration and pull requests etc, so I really don't mind on which branch (or any other aspects) the code gets merged, I really offered it for selfish reasons, I'd like to get your updates easily without doing a merge each time :) So if you tell me where you want it merged (if you're happy to) and what you want me to do next, I'll do it...

[ousnius]

Any person with write access to the repository (so in this case Dexesttp only) can merge any existing pull request. In other words, you don't need to do anything. Dexesttp can merge your commits by himself whenever he wants to.

[philjord]

I was just referring to

In any case, if you really want to merge to master you'll need to : Update the version number to reflect a version change Provide a change overview in a similar format to the beta pull request/releases.

In case Dexes wants any more from me ...

[Dexesttp]

Thank YOU for actually fixing small stuff :)

About the branch thing, I actually can handle that on my side. I'll also try to change the "final" things, to help keep the code consistent. It was just that if you had a special reason to want to merge into /master, I didn't want to just tell you off.

I'm not sure if this PR will appear as merged before the next release. You can consider it merged though, as I'll create a new branch corresponding to this branch on this repo, and descend the result to /dev then /master.

Thanks again for your contribution !

[neomonkeus]

Regarding the depreciation of code, it is fine to keep an implementation until a version point where it is removed. I presume you don't want to remove the method currently to avoid breaking the contract of the interface for classes which use it.

If you really want to push people away from from using it, then it is better to throw a Runtime Exception rather than having no implementation as an unaware developer can still call a still publicly accessible, but won't be alerted.

public void close() throws IOException {
    throw new RuntimeException("Depreciated method called - ClassnamesInterface.close() ");
}