Repository navigation
feat(map): QGIS import reads merged categories, dashes and QGIS 1/2 names, and notes what it drops - #876
Conversation
…ames, and notes what it drops An audit of the importer against 149 QGIS files (QGIS 1.8 to 4.2) found settings it dropped without a note, and some it read wrongly. This fixes those and adds the missing notes. - Merged categories: QGIS 4 writes a category of several values as <val>s, which were missed, so the category was taken as "all other values". - Dashes: custom dash patterns (in their unit) and QGIS's named pen styles (dash, dot, ...) become the stroke's dash pattern instead of a "drawn solid" note. - Files from QGIS 1 and 2: layer order and visibility from <legend>, the project's coordinate system from the map canvas or SpatialRefSys, the scale range's old names, opacity as layerTransparency or transparencyLevelInt, the old names of simple line and fill settings (whose lines weren't drawn at all), and a symbol's outputUnit. - New notes: print layouts, annotations, bookmarks, map themes and relations; a layer's time settings, diagrams, other styles, joins, virtual fields and vector tile style; draw effects, order-by, dashed polygon outlines; label halos, distances, offsets, repeats, priorities, wrapping, callouts, data-defined settings and QGIS 2's labels. - A layer of no provider (QGIS's annotation layer) is noted by its kind.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Project parsing and layer metadata crates/map/src/qgis/mod.rs, crates/engine/src/cmd/qgis.rs, crates/engine/src/tests.rs, crates/map/src/qgis/tests.rs |
Project reading retains layer types, supports legacy CRS, canvas, legend, opacity, and scale settings, and records notes for project and layer features. Providerless layers are identified by layer kind in import notes. Tests cover legacy project reading and annotation layers. |
Renderer and symbol parsing crates/map/src/qgis/renderer.rs, crates/map/src/qgis/symbol.rs, crates/map/src/qgis/tests.rs, docs/user-guide/workspace.md |
Renderer parsing accepts category values from <val> elements. Symbol parsing normalizes legacy properties and handles supported dash patterns. Tests and documentation cover these parsing and rendering details. |
Label settings and import notes crates/map/src/qgis/labels.rs, crates/map/src/qgis/tests.rs, docs/user-guide/workspace.md |
The label importer records approximations for selected label settings and skipped callouts and data-defined settings. Tests and documentation describe these import notes and other unsupported project and layer features. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Suggested reviewers: giswqs
Merge Risk: 
36c7b
-
The inspected QGIS import paths show no actionable issue blocking merge after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 6 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly summarizes the main QGIS importer changes, including merged categories, dash support, legacy names, and reporting unsupported settings. Description check The description provides a detailed summary, testing information, and a related issue reference. It does not use the template headings exactly, but it includes all required information and is complete… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description provides a detailed summary, testing information, and a related issue reference. It does not use the template headings exactly, but it includes all required information and is complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 6 files. (2 skipped: 1 unsupported, 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the map at dawn,
And marks the layers one by one.
Dashes hop across the page,
Old legends find a newer stage.
“Skipped,” says the hare, then writes the note,
And bounds away with map in tow.
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| let halo = child(text, "text-buffer").is_some_and(|b| b.attribute("bufferDraw") == Some("1")); | ||
| if halo { | ||
| report.approximated( |
There was a problem hiding this comment.
Nit (low confidence): this note fires for every label with a buffer, even though a buffer is the case RustGIS does carry over as a halo. Most real projects will show it on nearly every labelled layer. Consider noting it only when the buffer's colour or size differs notably from what RustGIS draws.
There was a problem hiding this comment.
Keeping it as is. The note is true for every halo: RustGIS draws its own halo (thin, white or dark by the text colour), so a QGIS buffer's colour and size are always dropped, not only when they differ a lot. Notes repeat per layer once, so a project with many labelled layers gets one line each, which is what the notes list is for. Guessing what counts as "close enough" would hide real differences.
Code reviewI read the full diff and the new tests. I could not build or run anything here, so the tests are unrun. Bugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared: the whole PR against main (merge base 2aaa0f0 to PR head 36c7bbf)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
|
@pick2510 This is awesome work—thank you for such a thorough audit and for improving compatibility across QGIS versions! Would you be able to share the QGIS projects you used for testing, along with any supporting data or download links? Having these available would help us reproduce the comparisons and catch regressions as the importer evolves. Even a representative subset would be really useful. Could you also take a look at QGIS parity and share your thoughts on the remaining gaps and which ones we should prioritize? |
|
@deniial00 Thanks! The corpus is up in #881 under
On parity: the audit lists each setting these 149 files contain and whether the importer reads it, notes it, or ignores it as irrelevant. After #876 and #877, 72 settings that vary are still dropped without a note, mostly project-structure details. The bigger gaps are in what RustGIS can draw. Here they are in the order I'd take them, by how often real projects (SMV25 and the Training Manual) hit them:
The remaining open importer PR is #877 (rasters). After it, I'd start with 1 or 2: they make the biggest visible difference on real maps, and SMV25 makes it easy to measure. Its pixel difference from QGIS is now 7.2% (overview 11.0%). |
|
@pick2510 Thanks for sharing the corpus in #881 and linking the supporting data and Training Manual files! Having the fixtures and generator available will really help with reproducing the comparisons and catching regressions. Your plan and prioritization sound good to me. For 1 (map coordinate systems), please also see #769 and the discussion about non-Web-Mercator tile sources and reprojection. It doesn’t cover the whole map-CRS problem, but it’s related and worth considering when defining the scope. Since these are substantial changes, please open separate proposal issues for the pieces you’d like to tackle before starting much implementation, or add proposals to existing issues where appropriate. A short outline of the scope, approach, affected crates and commands, and testing plan would help us agree on the direction and coordinate the work. |
|
@deniial00 Thanks, will do: a proposal issue per piece before implementing. A correction to point 1 in my last comment. I looked at the nine projects again. All of them use EPSG:4326, not a national grid: #769 (tile matrix sets other than Web Mercator) is a separate, larger piece: it doesn't block opening QGIS projects, only WMTS and tile sources in such projects. I'll mention it in the proposal where it applies rather than fold it into this. |
Follow-up to #825 and #844. I audited the QGIS importer against 149 QGIS files: projects and styles written by QGIS 4.2.3 that cover every symbol layer, renderer, label option, paint effect, provider, layout item and annotation item, the QGIS Training Manual's files (QGIS 1.8 to 3.10), and swisstopo's SMV25 project. The audit lists every setting in those files that the importer neither reads nor notes. This PR fixes what it read wrongly and notes what it dropped without a word.
Read wrongly before
<val value="c"/><val value="d"/>. The importer looked only for a list<Option>, so it took the category as "all other values" and drew those features with the wrong symbol.color,widthandpenstyle, which weren't read, so those lines weren't drawn at all.color_border,width_border,style_border) were lost.minimumScale/maximumScale, swapped against today's names), the opacity (layerTransparency,transparencyLevelInt) and the project's coordinate system (in<mapcanvas><destinationsrs>orSpatialRefSys) weren't read.<legend>but no layer tree. A symbol'soutputUnitis now the unit for sizes that give none of their own.Carried over now
Noted now (the notes count each thing once per layer)
Results on the 149 files: none crashes. Files that come in without a note went from 41 to 22, and those 22 carry over in full. Settings dropped without a note that vary in the files went from 257 to 72, mostly small label options and project structure. SMV25's 157 dashed strokes now draw as QGIS draws them. Compared pixel by pixel with QGIS's own rendering, the share that differs went from 7.5% to 7.3% in a 1 m/px view of Winterthur's old town and from 11.7% to 11.6% in a 4 m/px overview.
Tests
merged_categories_and_dashes_carry_overfiles_of_qgis_1_and_2_read_by_their_old_nameswhat_rustgis_has_no_place_for_is_noteda_qgis_project_opens_as_a_map_and_a_qgis_style_styles_a_layer, which also checks the annotation layer's noteThe QGIS section of
docs/user-guide/workspace.mdlists what's new.Summary by CodeRabbit