Store downloaded maps unzipped #8622
Conversation
Updates code to extract from zip existing and newly downloaded maps. Various code paths to load maps from zip can then be removed.
...a/games/strategy/engine/framework/map/file/system/loader/AvailableGamesFileSystemReader.java
Outdated
Show resolved
Hide resolved
| @@ -47,27 +41,6 @@ | |||
| new File(userMapsFolder, normalizedMapName)); | |||
There was a problem hiding this comment.
Not directly related to this PR, but I start wondering how much code we could end up saving by migrating away from java.io.File to java.nio.Path.
This simple method could get simplified to something like:
static List<Path> getCandidatePaths(final String mapName, final Path userMapsFolder) {
Preconditions.checkNotNull(mapName);
final String normalizedMapName = normalizeMapName(mapName) + "-master";
return List.of(
userMapsFolder.resolve(Path.of(mapName, "map")),
userMapsFolder.resolve(mapName),
userMapsFolder.resolve(Path.of(normalizedMapName, "map")),
userMapsFolder.resolve(normalizedMapName));
}There was a problem hiding this comment.
Not having to deal with file separator is a win. We likely should be preferring path where we can. Working in existing code is a bit odd as it's mixed now, but I'm certainly on board to convert and use Path in new code.
game-core/src/main/java/games/strategy/triplea/ResourceLoader.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #8622 +/- ##
============================================
- Coverage 24.92% 24.88% -0.04%
+ Complexity 7375 7365 -10
============================================
Files 1291 1291
Lines 80312 80359 +47
Branches 11006 11011 +5
============================================
- Hits 20017 19997 -20
- Misses 58221 58289 +68
+ Partials 2074 2073 -1
Continue to review full report at Codecov.
|
| private static String getMapPrefix(final File mapZip) { | ||
| try (ZipFile zip = new ZipFile(mapZip)) { | ||
| final Optional<? extends ZipEntry> baseTilesEntry = | ||
| zip.stream() | ||
| .filter(entry -> entry.getName().endsWith(REQUIRED_ASSET_EXAMPLE_FOLDER)) | ||
| .findAny(); | ||
| if (baseTilesEntry.isPresent()) { | ||
| final String path = baseTilesEntry.get().getName(); | ||
| return path.substring(0, path.length() - REQUIRED_ASSET_EXAMPLE_FOLDER.length()); |
There was a problem hiding this comment.
@jdimeo , this update now has everything being a folder. Does the submodule structure you have still work as directories rather than as expanded zip files?
There was a problem hiding this comment.
@DanVanAtta I'm not sure! I think this discovery process may still have to be re implemented for the folders, but I also know that the unzipped maps had less hardcoded assumptions about where things were located.
There was a problem hiding this comment.
Thanks. It's certainly a bit of a mess that TripleA assumes that a map name is equal to the map folder. This is something I'm trying to remove. Once the 'map.yml' file lands, it might be a lot of what you would want in order to have more customized locations. Regardless, that would probably be the right time FWIW to 'fix' the path discovery.
| return Optional.empty(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ah one thing I forgot to mention:
The Zipped Maps Extractor is just a migration tool right?
If so, would this be a candidate for a class that should be removed in the next major release or some other point in time?
|
We'll continue downloading maps as a zip and will want to support manually
installed map zip files. So it'll be around long term.
…On Wed, Jan 13, 2021, 2:29 AM RoiEX ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
game-core/src/main/java/games/strategy/engine/framework/map/file/system/loader/ZippedMapsExtractor.java
<#8622 (comment)>:
> + try {
+ final Path newLocation = badZipFolder.resolve(mapZip.getName());
+ Files.move(mapZip.toPath(), newLocation);
+
+ return Optional.of(newLocation);
+ } catch (final IOException e) {
+ log.error(
+ "Failed to move file: "
+ + mapZip.getAbsolutePath()
+ + ", to: "
+ + badZipFolder.toFile().getAbsolutePath(),
+ e);
+ return Optional.empty();
+ }
+ }
+}
Ah one thing I forgot to mention:
The Zipped Maps Extractor is just a migration tool right?
If so, would this be a candidate for a class that should be removed in the
next major release or some other point in time?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#8622 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC6SZOMJ6SNI3HDDTSVJXOLSZVYX3ANCNFSM4V6S2VNA>
.
|
That is good. |
| return findResource(path).or(() -> findResource(inputPath)).orElse(null); | ||
| return findResource(inputPath) // | ||
| .or(() -> findResource(inputPath)) | ||
| .orElse(null); |
There was a problem hiding this comment.
Looks like I missed this in this PR:
findResource() is called twice with the same parameter, will create a small refactoring PR including this change
Updates code to extract from zip existing and newly downloaded
maps. Various code paths to load maps from zip can then be
removed.
Testing
Screens Shots
Additional Notes to Reviewer
Release Note
UPDATE|Downloaded maps are now extracted and stored as flat files rather than as zipped files.