Simplify link localization code #8637
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8637 +/- ##
============================================
- Coverage 24.88% 24.86% -0.02%
+ Complexity 7366 7363 -3
============================================
Files 1291 1291
Lines 80365 80376 +11
Branches 11012 11011 -1
============================================
- Hits 19998 19989 -9
- Misses 58295 58312 +17
- Partials 2072 2075 +3
Continue to review full report at Codecov.
|
Main goal of this simplification is to remove 'map name' from resource loader. One place that is used is in Link Localization. Instead of passing a resource loader to the link localizer, we can instead pass a file path which in turn simplifies how link localization can be done. Additional: - removed link localization cache - changed link localizatoin behavior to only localize links that are not already absolute. Previously links in single quotes would be localized and links in double quotes would be left as-is. The localization of fully qualified links would parse the link path, get the file name and localize the file name path. This is removed as it is difficult to understand and is much easier if we localize any relative links but keep absolute links as they are (regardless of quoting).
DanVanAtta
force-pushed
the
resource-loader-with-file-input
branch
from
4da2283 to
39c4e8d
Compare
|
|
||
| if (replacementUrl == null) { | ||
| log.error(String.format("Could not find: %s/%s", loader.getMapName(), firstOption)); | ||
| final String secondFallback = ASSET_IMAGE_FOLDER + ASSET_IMAGE_NOT_FOUND; |
There was a problem hiding this comment.
Is the fallback image never used?
There was a problem hiding this comment.
Yes, it's never used, the file 'notFound.png' is not part of the game engine. A map could have it, but none do.
|
|
||
| @VisibleForTesting | ||
| static String localizeImgLinksInHtml(final String htmlText, final ResourceLoader loader) { | ||
| public static String localizeImgLinksInHtml(final String htmlText, final File mapContentFolder) { |
There was a problem hiding this comment.
So in case the resource does not exist we will no longer notice at this point, right?
And because the HTML is not really handled by us, this will result in a silent error, just like it would in a normal web browser
There was a problem hiding this comment.
The resource loader was only used to get the map folder location, it was not actually loading the image or validating that it was there. Hence, that data was being used to localize the path only, everything was and is done as String operations, so we were not noticing it either way.
Main goal of this simplification is to remove 'map name' from resource loader.
One place that is used is in Link Localization. Instead of passing a resource
loader to the link localizer, we can instead pass a file path which in turn
simplifies how link localization can be done.
Additional:
absolute. Previously links in single quotes would be localized and links in
double quotes would be left as-is. The localization of fully qualified links
would parse the link path, get the file name and localize the file name path.
This is removed as it is difficult to understand and is much easier if
we localize any relative links but keep absolute links as they are (regardless
of quoting).
Testing
Screens Shots
Additional Notes to Reviewer
Release Note
UPDATE|Map making - clarified how image links in HTML is made localized. If the link is absolute, it will now always remain absolute (eg: https://path-to-my-image/my-image.png). Otherwise if the image path is relative (eg: "img src=someImage.png"), the path will be made relative to the maps folder 'doc/images/'.