Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Read SLD TextSymbolizer: venderOptions to get advanced settings #33813

Merged

Conversation

rldhont
Copy link
Contributor

@rldhont rldhont commented Jan 15, 2020

Description

VendorOptions element is used by QGIS is used to provide advanced settings.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

@rldhont rldhont added Labeling Related to QGIS map labeling Bug Either a bug report, or a bug fix. Let's hope for the latter! backport release-3_10 labels Jan 15, 2020
@rldhont rldhont added this to the 3.12.0 milestone Jan 15, 2020
@rldhont
Copy link
Contributor Author

rldhont commented Jan 15, 2020

@aaime and @luipir this PR completes #33725 can you review it

@luipir
Copy link
Contributor

luipir commented Jan 15, 2020

I could be boring, but lack of tests that this vendorOption font settings are propagated to font and settings.

other this this aspect, LGTM

Another question... this are QGIS or Geoserver vendor options? (I suppose qgis)

@rldhont
Copy link
Contributor Author

rldhont commented Jan 15, 2020

These VendorOption are QGIS ones.

All the sld loader and reader tests have to be enhancent.

@aaime
Copy link
Contributor

aaime commented Jan 15, 2020

I cannot provide much of a review in terms of QGIS behavior, but the code looks legit, and I'm noticing that the vendor option names match the ones used in GeoServer.

@rldhont
Copy link
Contributor Author

rldhont commented Jan 15, 2020

The vendor option names are the one used by QGIS in the save as SLD https://github.com/qgis/QGIS/blob/master/src/core/labeling/qgsvectorlayerlabeling.cpp#L457

@rldhont
Copy link
Contributor Author

rldhont commented Jan 16, 2020

@luipir I test AroundPoint with maxDisplacement, can I merge ?

@luipir
Copy link
Contributor

luipir commented Jan 16, 2020

AroundPoint with maxDisplacement, can I merge ?

not clear to me how did you teste all settings set in the added code. IMHO could be interesting create a test SLD (or a set) as in

mFilePath = os.path.join(TEST_DATA_DIR, 'symbol_layer/external_sld/simple_line_with_text.sld')
where all parameters are tested.

Are there problems to create a test data set? If the problem is the feature freeze (tomorrow?), I agree to merge and then add new tests later. Is this the reason to merge without good test coverage?

@rldhont
Copy link
Contributor Author

rldhont commented Jan 17, 2020

Thanks @luipir I will make tests next week.

@rldhont rldhont merged commit 0fa5a8f into qgis:master Jan 17, 2020
@qgis-bot
Copy link
Collaborator

The backport to release-3_10 failed:

The process 'git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_10 release-3_10
# Navigate to the new working tree
cd .worktrees/backport-release-3_10
# Create a new branch
git switch --create backport-33813-to-release-3_10
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 0fa5a8f4d86978901678c277cfff55c730ec9837
# Push it to GitHub
git push --set-upstream origin backport-33813-to-release-3_10
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_10

Then, create a pull request where the base branch is release-3_10 and the compare/head branch is backport-33813-to-release-3_10.

@rldhont rldhont deleted the fix-read-sld-textsymbolizer-vendoroptions branch January 17, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Labeling Related to QGIS map labeling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants