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

COMMON: Clean up archive path handling, add getFileName and getPathInArchive to ArchiveMember #5108

Merged
merged 2 commits into from Jul 19, 2023

Conversation

elasota
Copy link
Contributor

@elasota elasota commented Jun 20, 2023

Reworked this to hopefully minimize regression risk. This partially refactors archive path handling to better handle full paths and alternate path separators.

The main thing motivating this is a few things:

  • The default implementation of Archive::listMatchingMembers calls getName to determine the path to match, which acts as if getName returns a full path, except it may not. In particular, FSNode archive members always return only the file name.
  • ArchiveMember lacks a way to get the path of a file relative to the archive root.

I mainly want these to make a VFS for mTropolis to make dealing with installers easier (basically, the VFS would be an archive containing hardlinks to other archives - in order do to that though, it needs to be possible to scan an archive subpath).

The previous version of this tried just making getName always return only the filename, but that would require checking a LOT of engines for regressions which I think makes this too big of a project. Instead, this adds getFileName and getPathInArchive and deprecates getName. This is absolutely NOT guaranteed to work correctly with all existing cases (some engines may be returning full paths from getFileName, if they actually support them), but it will provide a smoother path to making this work in a sensible way.

This also changes GenericArchiveMember to accept a path instead of a string. This should transparently support existing cases since getName will return the path converted using the same path separator, except for StuffIt (see below).

This also changes FSDirectory to return FSDirectoryFile as the archive member type. The reason for this is so that when calling getMember with a subpath of a directory, it will return a FSDirectoryFile for which getPathInArchive returns the relative path from the directory that getMember was called on instead of the parent of the file, keeping the principle that passing getPathInArchive back to getMember should always return the same thing.

The only case this will cause a change in behavior is directly constructing a path using an incompatible separator, which is currently only possible for StuffIt and VISE archives since they are the only ones that use alternate path separators, and the StuffIt loader flattens the directory tree by default.

However, this should improve support for MacOS filenames with slashes in them, since StuffItArchive::getMember will no longer interpret the slash as a separator.

@elasota elasota changed the title COMMON: Clean up archive path handling, add getPathInArchive COMMON: [WIP] Clean up archive path handling, add getPathInArchive Jun 22, 2023
…e that unambiguously return the filename or the full path
@elasota elasota changed the title COMMON: [WIP] Clean up archive path handling, add getPathInArchive COMMON: Clean up archive path handling, add getFileName and getPathInArchive to ArchiveMember Jul 6, 2023
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

clang has a new warning with this branch:

./common/fs.h:252:30: warning: 'createReadStream' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual SeekableReadStream *createReadStream() const;
                                    ^
./common/archive.h:60:30: note: overridden virtual function is here
        virtual SeekableReadStream *createReadStream() const = 0; /*!< Create a read stream. */
                                    ^
1 warning generated.

@elasota
Copy link
Contributor Author

elasota commented Jul 6, 2023

Fixed, though that looks like a pre-existing problem.

@sev-
Copy link
Member

sev- commented Jul 19, 2023

Thank you!

@sev- sev- merged commit f38b732 into scummvm:master Jul 19, 2023
8 checks passed
mistydemeo added a commit to mistydemeo/scummvm that referenced this pull request Jul 24, 2023
These broke in the archive refactor,
b8acbe6/scummvm#5108, because it removed
the ability to directly convert an `ArchiveMember` to an `FSNode`.
As a result, it was no longer possible to directly open a resource
fork as a stream.
mistydemeo added a commit to mistydemeo/scummvm that referenced this pull request Jul 25, 2023
These broke in the archive refactor,
b8acbe6/scummvm#5108, because it removed
the ability to directly convert an `ArchiveMember` to an `FSNode`.
As a result, it was no longer possible to directly open a resource
fork as a stream.
sev- pushed a commit that referenced this pull request Jul 27, 2023
These broke in the archive refactor,
b8acbe6/#5108, because it removed
the ability to directly convert an `ArchiveMember` to an `FSNode`.
As a result, it was no longer possible to directly open a resource
fork as a stream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants