Skip to content
This repository has been archived by the owner on Apr 25, 2022. It is now read-only.

Add common interface for archive and converter #14

Closed
rr- opened this issue Mar 31, 2015 · 4 comments
Closed

Add common interface for archive and converter #14

rr- opened this issue Mar 31, 2015 · 4 comments
Assignees
Labels

Comments

@rr-
Copy link
Member

rr- commented Mar 31, 2015

For #13 to work nicely, Archive and Converter should derive from a new class, Transformer, which has pure virtual methods such as add_cli_help and parse_cli_options.

@rr- rr- self-assigned this Mar 31, 2015
@rr-
Copy link
Member Author

rr- commented Apr 1, 2015

BTW, the only reason why Converter is not an Archive that does file_saver.save(one_file) is to ensure that following

./arc_unpacker ~/file.dat ~/file2.dat

produces

~/file~.png
~/file2~.png

and not

~/file~/file.png
~/file2~/file2.png

Because of this, the code needs to be duplicated all over the place:

  • ConverterFactoryArchiveFactory
  • Converter::add_cli_helpArchive::add_cli_help
  • Converter::parse_cli_optionsArchive::parse_cli_options
  • Converter::try_convert(File &)Archive::try_unpack(File &, FileSaver &)
  • Converter::convert(File &)Archive::unpack(File &, FileSaver &)
  • Converter::convert_internal(File &)Archive::unpack_internal(File &, FileSaver &)
  • bin/file_decoderbin/arc_unpacker

If Archive and Converter were to be merged into one Transformer, the original behaviour could be emulated with a use_folder_prefix public field inside each Transformer. This isn't exactly best solution ever, but it's probably better than having to deal with such a dual design thorough whole project.

@rr-
Copy link
Member Author

rr- commented Apr 1, 2015

I tried implementing this by making Converter derive from Archive ( == Transformer from my ponderings above) but it didn't end too well, since it didn't understood that nested archives should have name in form of parent file/inner file. It just kept naming everything in inner file form.

If I am to merge these classes indeed, use_folder_prefix or injecting file naming strategy on Transformer level is an absolute must. Additionally, the ideal solution would unify file naming logic between main executable and nested archives handling. Right now they're completely separate 👎


The other option to detect how to name the file is to do it after the fact. If the transformer produced more than two files, it should create a folder for them, if not, place it aside the original file. Detecting this is difficult, because it would require file pooling to be able to tell how to proceed before saving, or renaming the files after the fact. Both variants can get a little mind-numbing when considering nested archives.

Needless to say, I'd prefer automatic way over manual, failure-prone micromanagement. I'm going to try file pools this weekend. Until then, I'll focus on making handling nested archives less pain in the ass (see 6a270ee for what I mean).

@rr-
Copy link
Member Author

rr- commented Apr 2, 2015

Needless to say, I'd prefer automatic way over manual, failure-prone micromanagement. I'm going to try file pools this weekend. Until then, I'll focus on making handling nested archives less pain in the ass (see 6a270ee for what I mean).

Actually I just stumbled across a use case that makes manual file naming convention injecting seem much more reasonable. Imagine archive containing files, that, once extracted, make up for the final file system:

  • th.dat
    • file1.anm
      • data/character/patchouli/sprite1.png
      • data/character/patchouli/sprite2.png
      • data/character/patchouli/sprite3.png
    • file2.anm
      • data/character/alice/sprite1.png
      • data/character/alice/sprite2.png
      • data/character/alice/sprite3.png

So forget about file pools for now, gonna go with use_folder_prefix (or similar, more enterprisey approach). The way I see it is that both Archive and Converter should derive from a Transformer and implement use_folder_prefix. By default Converter disables prefixes and Archive enables them, but at the same time, it's left public so it can be manually tweaked depending on current needs. After that, FileSaver proxy within Transformer will either add the folder prefix to the saved file or not, depending on configuration of active sub-transformer.

rr- pushed a commit that referenced this issue Apr 4, 2015
- Removed need to implement internal archive unpacking manually thanks
  to new add_transformer(Archive*).
- Replaced existing implementations with add_transformer().
- Changed archive/converter ownership management: transformers now need
  to be bound to Archive::Internals. This change makes possible to
  support recursive archives (e.g. .zip inside .zip).
- Changed add_transformer visibility to protected - because they're
  now given raw pointers and are intended to be used only from within
  Archives, it makes sense.
- Changed naming of ABMP7/ABMP10 files. It's much nicer now.
- Broken naming of ANM. This is something that needs to be addressed
  through #14.
rr- pushed a commit that referenced this issue Apr 4, 2015
- Completely revamped file naming management
- Introduced file naming strategies
- Added Transformer as a common class for Archive and Converter
- Merged Converter and Archive factories (those #includes...)
- Added a few badly needed tests
@rr- rr- closed this as completed Apr 4, 2015
@rr-
Copy link
Member Author

rr- commented Apr 4, 2015

Fuck yeah 💯

The naming management is now hard-to-follow, but it works better than before, made project structure much cleaner and unit tests should convey why it needs to be like this. I'm quite satisfied with the results.

@rr- rr- added the general label Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

1 participant