Read shapefiles in UTF-8 mode #137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current (bad):
With this patch (good):
This example rendered the following shapefile: https://data.geo.admin.ch/ch.bafu.schutzgebiete-aulav_jagdbanngebiete/ |
https://github.com/onthegomap/planetiler/actions/runs/1997273067 ℹ️ Base Logs 0a06479ℹ️ This Branch Logs f367bf3 |
msbarry
reviewed
Mar 16, 2022
| @@ -125,7 +126,9 @@ private ShapefileDataStore open(Path path) { | |||
| } else { | |||
| throw new IllegalArgumentException("Invalid shapefile input: " + path + " must be zip or shp"); | |||
| } | |||
| return new ShapefileDataStore(uri.toURL()); | |||
| var store = new ShapefileDataStore(uri.toURL()); | |||
| store.setCharset(Charset.forName("UTF8")); | |||
There was a problem hiding this comment.
Thanks for bringing this up! I noticed the same issue when trying to read the natural earth shapefile. One concern here is that standard encoding for shapefiles is technically ISO8859-1 so I'd hesitate to always use UTF-8. It looks like shapefiles that have a different encoding actually contain a ".cpg" file next to the .shp file with the text of the encoding though, so maybe we could use that if it exists?
There was a problem hiding this comment.
Interesting point with the .cpg. QGIS seems to get the encoding right without further configuration:
There was a problem hiding this comment.
Hmm you're right.. the shapefile reader should be doing this automatically. From the geotools release page it looks like starting with version 26 (we're on 26.3) they attempt to use cpg file to determine the encoding. I wonder why that's not working here?
There was a problem hiding this comment.
https://docs.geotools.org/stable/javadocs/org/geotools/data/shapefile/ShapefileDataStore.html#isTryCPGFile-- returns false, i.e., by default it does not access the .cpg file. Let's check if I can change the behavior with setTryCPGFile()...
|
Thanks for making the change! Would it be possible to add a minimal test case that verifies a UTF-8 string from a shapefiles is decoded properly? |
|
Works without hardcoding utf-8 now, by using the |
The unit test would be helpful! I'm hoping to switch to Apache SIS at some point so it would be good to make sure it has the same handling. There is a tiny Also, it might be useful to have an extra option on |
|
I edited
I updated the test file |
|
Are you suggesting to overload |
Awesome, looks great! Thank you!
Yeah I could go either way on this, since there are now 2 optional parameters (source encoding and projection) there could be 4 signatures based on which ones you want to set and 8 if we add a third...I should refactor this to allow come kind of parameter object to make it these kinds of settings easier to add. Maybe a compromise would be to just add a character encoding argument to the existing method that includes a projection, and if it's null then ignore? Also I'd be fine doing nothing if adding the cpg file is an OK workaround. |
|
I think the |
|
Sounds good to me! Thanks for making this change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if a shapefile has attributes with letters like ä, ö, ü, etc, the resulting tiles show some strange letters. This pull requests fixes this issue by hard-coding the shapefile mode to UTF-8.