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

Allow usage of external Listfile to rebuild MPQ. #36

Merged
merged 10 commits into from Mar 22, 2021
Merged

Allow usage of external Listfile to rebuild MPQ. #36

merged 10 commits into from Mar 22, 2021

Conversation

zach-cloud
Copy link
Contributor

@zach-cloud zach-cloud commented May 2, 2020

  • By calling the addExternalListfile method, allows JMPQ3 to write archives that were missing (listfile)
  • Refactored code that was going to be common between the addExternalListfile method and the internal readListfile

@coveralls
Copy link

coveralls commented May 2, 2020

Coverage Status

Coverage decreased (-0.4%) to 85.503% when pulling 5b6e5a8 on zach-cloud:master into 7e87dc9 on inwc3:master.

@@ -120,6 +120,8 @@

/** If write operations are supported on the archive. */
private boolean canWrite;
/** If the archive was originally read-only */
private boolean openedAsReadOnly;
Copy link
Member

@Frotty Frotty May 3, 2020

Choose a reason for hiding this comment

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

why add this superfluous variable?
It also wouldn't sound right to open the mpq as read-only and for it then to become writable.
The listfile function should probably only be available in edit mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the "edit mode" is only available in the Constructor. If the (listfile) doesn't exist, it sets canWrite to "false" even if it was originally not opened as read-only. That's why the setExternalListfile method needs to set canWrite to "true". However I don't want to set canWrite to "true" if the archive was orignally opened as readOnly. Does this make sense? Maybe there's a better way of accomplishing this.

Copy link
Member

Choose a reason for hiding this comment

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

could pass listfile in constructor i suppose to prevent setting canWrite to false.

* Removes files from the listfile if they aren't
* actually in the map.
*/
private void removeMissingFiles() {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@zach-cloud zach-cloud May 3, 2020

Choose a reason for hiding this comment

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

Yes, it is. Good point, that's a much better way of doing this.
Huh.. interesting that this was already in the code.. I swear it didn't work properly without adding in this removeMissingFIles method. I'll test again.

Copy link
Member

Choose a reason for hiding this comment

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

Is it resolved?

@Frotty
Copy link
Member

Frotty commented May 3, 2020

Thanks for your contribution 😃
I added a couple comments, otherwise looks good.
The hiddenFile count was never working reliably iirc, should probably check up on that.
e: Ok I fixed travis integration. The Test fails currently.

@zach-cloud
Copy link
Contributor Author

zach-cloud commented May 3, 2020

Ah, found why the test failed :) Fixed that.

@Frotty
Copy link
Member

Frotty commented May 16, 2020

Status?

@zach-cloud
Copy link
Contributor Author

zach-cloud commented May 16, 2020

Status?

Sorry for the delays. Been very busy lately.
I've re-tested and confirmed it does work without the removeMissingFiles method, so I've deleted this.
Should be good to go anyways

@Frotty
Copy link
Member

Frotty commented Mar 22, 2021

Sry for not merging, thanks for the contribution.

@Frotty Frotty merged commit d583df8 into inwc3:master Mar 22, 2021
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.

None yet

3 participants