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

Support for reading/writing single Enigma files #19

Merged
merged 9 commits into from
Jun 15, 2023

Conversation

NebelNidas
Copy link
Collaborator

@NebelNidas NebelNidas commented Jun 8, 2022

Currently, mapping-io only supports Enigma directories. This PR adds support for single Enigma mapping files. This should bump the library's major version, since MappingFormat.ENIGMA now relates to the file instead of the directory structure, which is now called MappingFormat.ENIGMA_DIR - same goes for the respective reader and writer classes.

Closes #22.

@Devan-Kerman
Copy link

why not just MappingFormat.ENIGMA_FILE, then you don't have to change the major version

@NebelNidas
Copy link
Collaborator Author

NebelNidas commented Jun 9, 2022

why not just MappingFormat.ENIGMA_FILE, then you don't have to change the major version

This would break the convention that MappingFormats are single files by default, which IMO definitely should be left this way. If you want to do something "weird" like exporting mappings to a directory, that should be explicitly stated, not the other way around

@Devan-Kerman
Copy link

I don't think that convention is important, and I would say it's def worth less than maintaining compatibility

@NebelNidas
Copy link
Collaborator Author

NebelNidas commented Jun 9, 2022

But it's irritating for the library consumer when there's no coherent naming scheme, I don't want to commit such no-gos. We're in the 0.x development phase after all. And I want to commit another breaking change later on anyway, so this is a good occasion for doing it now all at once ;)

@NebelNidas
Copy link
Collaborator Author

8f2e51c This was the second breaking change I was talking about, and I think it is needed since the format package became really cluttered (and would have become so even more after more formats get added)

NebelNidas added a commit to NebelNidas/jadx that referenced this pull request Jun 9, 2022
Temporarily move to my mapping-io fork until this PR gets merged: FabricMC/mapping-io#19
@NebelNidas NebelNidas marked this pull request as ready for review June 9, 2022 03:27
@sfPlayer1
Copy link
Contributor

It's be best to have the suffix on both, makes it least surprising. Other formats don't have multiple sub-types in this form so can be left alone.

I think it is better to not split the format implementations by version (put both tiny1+tiny2 in tiny). Internal classes like EnigmaWriterBase or ColumnFileReader should be package private if possible or documented as impl only otherwise. We could probably introduce the jetbrains annotations (same as in fabric api) to make it clearer.

@NebelNidas
Copy link
Collaborator Author

NebelNidas commented Jun 9, 2022

Internal classes like EnigmaWriterBase or ColumnFileReader should be package private [...] We could probably introduce the jetbrains annotations

Done!

Other formats don't have multiple sub-types in this form so can be left alone

Hmm, I feel like if we're adding suffixes anyway, we should apply them consistently. Leads to more confusion otherwise.

I think it is better to not split the format implementations by version (put both tiny1+tiny2 in tiny)

Generally I agree, but the only thing tiny v1 and v2 have in common is their name (and tabs), basically everything else is different 😅

Edit: I've merged them into the same package anyway.

skylot pushed a commit to skylot/jadx that referenced this pull request Jun 11, 2022
…(PR #1505)

* Add option to export mappings as Tiny v2 file

* Comply with JADX's import order conventions

* Only use Java 8 features

* Only use Java 8 features (2)

* Export comments to mappings file

* Method args test (doesn't work)

* Make method arg mapping exports work now

* Use `getTopParentClass()` instead of `getParentClass()`

See #1505 (comment)

* Remove unneeded method load call

* Small code cleanup; initial (broken) support for method vars

* Fixes regarding inner classes

* Add option to export mappings as Enigma directory

* Add option to export mappings as Enigma file/directory

Temporarily move to my mapping-io fork until this PR gets merged: FabricMC/mapping-io#19

* Fix method vars' lv-indices

* Use correct offset value for method var mappings

* Also supply lvt-index for method var mappings

* Clarify why we're using local mapping-io fork; comment out Fabric Maven for now

* Remove unnecessary `public` modifier

* Make an `if` condition less complicated

* Move mapping export code into jadx-gui (for now)

* Make mapping export async; make export menu only clickable when everything is loaded

* Fix export mappings menu field declaration position
@NebelNidas NebelNidas mentioned this pull request May 6, 2023
@NebelNidas NebelNidas force-pushed the master branch 3 times, most recently from ca70c2c to 73b26ca Compare May 9, 2023 21:27
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Lets merge, but get all the planned breaking changes in before releasing

@sfPlayer1 sfPlayer1 merged commit 6a06f2a into FabricMC:master Jun 15, 2023
4 checks passed
@NebelNidas NebelNidas added this to the 0.5 milestone Oct 3, 2023
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.

Enigma format doesn't support individual files
4 participants