Calculate size of downloaded maps when selected #13516
Conversation
- method newMapUrlAndSizeLabelText: Extracted logic to separate methods - new method appendSizeAndPath: Includes sum up in case multiple downloaded maps are selected - new method appendSizeAndPathSingle FileUtils.java - new method getFolderSize - new method getByteSizeFromPath
...game-core/src/main/java/games/strategy/engine/framework/map/download/DownloadMapsWindow.java
Outdated
Show resolved
Hide resolved
|
What is the performance like of this update for the very large maps? I believe TWW is the largest map at like 200MB or something. |
|
I did not recognize any even when selecting multiple maps with >400MB |
- method getTextForSizeAndPath: Restructure processing with static methods
| final long[] size = {0}; | ||
|
|
||
| Files.walkFileTree( | ||
| folderPath, |
There was a problem hiding this comment.
Does walkFileTree work regardless of whether we have a directory or a file? If so, we don't need the check for is a file and this method can be combined into getByteSizeFromPath
There was a problem hiding this comment.
The check is already done in method getByteSizeFromPath
There was a problem hiding this comment.
Yeah, what I'm saying is that check is potentially not needed. You can maybe simplify and just walk the file tree whether the parameter is a file or a directory.
The javadocs for the method make it sound like the parameter can be a file:
Where the file is a directory....
There was a problem hiding this comment.
@DanVanAtta I am not clear one what that would help: If the path is not a file what is the expectation when calling method walkFileTree? From my perspective the way it is written now it is more clear what is supposed to happen.
There was a problem hiding this comment.
If the path is not a file what is the expectation when calling method walkFileTree?
Returns just the file
From my perspective the way it is written now it is more clear what is supposed to happen.
I think questions are raised for why walkFileTree not being used. It's overly complicated, the getByteSizeFromPath seems entirely unnecessary.The questions are specifically things like "what did the author know that I don't that makes this not usable?" It's exactly the same scenario where we were wondering about ppt.getTerritory() and why that value was not being used directly - it's a mystery why it's coded that way since a simpler solution seems obvious.
There was a problem hiding this comment.
If I misunderstand how walkFileTree works, then the two methods are reasonable. Let's experiment a bit.
The javadocs sound like we should be getting back the same file:
This method walks a file tree rooted at a given starting file. The file tree traversal is depth-first with the given FileVisitor invoked for each file encountered.
Fixes according to review comments BattleCalculatorPanel.java - use new constant for N/A DownloadMapsWindow.java - constant for "N/A" - method getTextForSizeAndPath: rename map -> selectedMap; use new method mapsWindowModel.getByteSizeTextForMap - move getTextForSizeAndPathByMapPath to DownloadMapsWindowModel - method getTextForSizeAndPathFromMaps: rename map -> mapItem DownloadMapsWindowModel.java - new method getByteSizeTextForMap FileUtils.java - method getFolderSize: moved after usage as method is private
|
I hope I have not missed any of the already clarified comments in my new commit 2ca1ac5. |
| return installedMapsListing.findMapFolderByName(mapDownloadItem.getMapName()); | ||
| } | ||
|
|
||
| public Optional<@NonNls String> getByteSizeTextForMap(final MapDownloadItem mapDownloadItem) { |
There was a problem hiding this comment.
Should return a number here. Returning a string is mixing the UI and model logic together.
There was a problem hiding this comment.
What with exception case?
Maybe you provide a commit for this to show me what you think is best.
There was a problem hiding this comment.
Change:
byteSizeText =
org.apache.commons.io.FileUtils.byteCountToDisplaySize(
org.triplea.io.FileUtils.getByteSizeFromPath(mapPath));
To:
return org.apache.commons.io.FileUtils.byteCountToDisplaySize(
org.triplea.io.FileUtils.getByteSizeFromPath(mapPath));
There was a problem hiding this comment.
I think this is resolved now, right?
There was a problem hiding this comment.
I don't see any updates since my original comments nor any discussion based on the suggestion. I don't see any evidence for how this was resolved - I don't think it was.
| defenderLeftWhenDefenderWon.setText( | ||
| Double.isNaN(avgDefIfDefWon) | ||
| ? "N/A" | ||
| ? DownloadMapsWindow.TEXT_ABBREVIATION_NOT_APPLICABLE |
There was a problem hiding this comment.
IMO, leave the string inlined here. Why should DownloadMapsWindow own the N/A string?
I don't think the constant is really helping very much, it's okay to let things be inlined and duplicated sometimes.
There was a problem hiding this comment.
It is needed to make it translateble only once. The usage is the same, but I did not want to create an own class for it. At leadt not now when there are no others yet.
There was a problem hiding this comment.
Sure, but we're not making it translatable just yet. Let's do this work when we do the other thing. Otherwise this is just half done. Let's leave it be unless we do the whole effort.
There was a problem hiding this comment.
We are outsourcing the text and combine 3 places into one. I do not see what benefit it has to undo it in order to redo it again in the future.
There was a problem hiding this comment.
I do not see what benefit it has to undo it in order to redo it again in the future.
I'm suggesting that is premature optimization. Who is to say this class will even exist in its current form when we do redo it. The rule of thumb, have the simplest possible code for now, do the other stuff when it is needed. Would you prefer to add a feature to the simplest possible code base, or would you prefer to add a feature that tried to predict all the features that were going to be added?
To another extent, this PR is to "calculate the size of downloaded maps when selected." What does that have to do with internationalizing the string N/A?
FileUtils.java - removed method getFolderSize - method getByteSizeFromPath: Take over code from getFolderSize
2 participants
FileUtils.java
Notes to Reviewer
I tried to also get the size from the maps selected to be downloaded, but the GitHub-URLs did never return the size and a download just to know the size was pointless.