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

Add json support to WMS GetLegendGraphic #31747

Merged
merged 3 commits into from
Oct 26, 2019
Merged

Conversation

elemoine
Copy link
Contributor

@elemoine elemoine commented Sep 13, 2019

Description

This PR adds support for GetLegendGraphic responses encoded as JSON. It builds on previous work by @pblottiere, who added QgsLegendRenderer::exportLegendToJson for that exact purpose.

For example a GetLegendGraphic request with FORMAT=image/png producing the image

image

will produce the following with FORMAT=application/json

{
  "nodes": [
    {
      "icon": "iVBORw0KGgoAAAANSUhEUgAAABoAAABuCAYAAAA5+QMZAAAACXBIWXMAAA9hAAAPYQGoP6dpAAAHaUlEQVRoge2YW4xdVRmAv3XZt3OdmdMZplPaDp1eCBUIoVwCxoxGLopTE5MmPBnEhJiqCbyYiIm+4ANvRjAxaEwTH4TURGIUY0SYqlGwlIvYaTsUpu1QOtdzzsy57XP23mv5QGeYdjqll9MXc76nk73X+r+91vrXv89egstk9+7d2SjKiiSRSWtAJRXXTQ4991z0af3EpzV4+OFvbMz15m8JUunPSiG3Bb7XC2QAnVgbxyYpEZmploneWKiUXznVaIyP7tsXXrLoW48/fq/npfYWugqfx1HrpfaIhcPJWpPFWpPEWLSW5H2HwZyHjJskrVZSrVVfCyu1/Tau//yZZ55pril68sknr6sl4meFnsJXg3RGn6wZ3jp+isb0GUR5BpJk1UNZ18cZuIG+6we47fo86SSiWJp7sxHWv/vTp5/+5yrRd773g135TPqF7p7Clsm65fXD79F6fwxhLSbI0CoMEBfWk6TzWKkRURNVX8SdOoE79yFYi01lGdx1N/dsyLBYWSgt1hYf/clTT724LPrm3ie2DmzoP5DPdw28PRcyduBVMAn1rbdR23kPUWHgomup6hW8yWNk/zOKLs1QuPNz3D9UYKFUmisWS8MKYGRkJLVxy7ZfFAqFXacbljdefpnWdZuY3f1tGkO3YlLZi0oArOMRrRugfuMdYA3J2//A27iV9Tk/1YzqPRogW+jfns/l7gM4OD5JfccuynePgPjUpFwtVA6Ld36ZuGcDh98fY+ttg9hEfkED5HLBDULiGwQNHVC+68okK6lvvZXY1hFAGNZzEqA4X7mjUW8A4N10+1VLlli36QasMYSNRkMCKJWcmZ2bLkss29wm2KsXaWO5OZomakUkURJLAISqho2wXK1UuCmaYSguXZVEGcEXmxP0JFVOTUwsaEdHcumm63iDp05OfNCq1fhKeIybo1nEZY5MWMjaFiPhET4TT/PB+8eLQsi0tRi9sqHrBFtOfDAx3r+hf/CBHuPuUPO86fZzQndhAbuWAOhOQnZGc9wST9Gcn2bsw9NF1/OzCDSAPr+T47rbp05PlednZ518d7d4MPcRSdDFpOpiXvlU8EikQBpL2kZ02QYbTAW/XsI2G5w6M4OyBtfze1Y8iV0lOivrskC5WKZYLBJZRTqTYUgrXC2wQiKFIk5iytWQ02GINDG+EEitQMhVMS8oWpoOhEQhkVha1UVa9uz0WbBYpBAIKcgoCWrNUJgkWdQACYQXW3ghBEqoNe9fDGstxpiTEsDWq3+LTVS/okiXIEqS1osSYP/+/adbcfQHa9fKqysnbNT/dfTdd/Ytr1ps9BNhK5xoqyRszEQmfPTQoUOfbNjnf/XsR2Gj+VAjrJ3gKkdmjMUkEdjk2d/s23cU4Jw8fOHXvzySJNGPPFdikgRjzWVL4jgmm/bYOrgBz9PB0vVVOZnx/c2bNw4QRTHzxTKVep0ksh/vDWEBcTb1BRaLMQZrLJ6rSQc+vb39pIOAxBi01JvWFAkphxyt8VyXTDqFMYZ6o0mtUSduJURJzFLSuI4m8D1SQYDrOkj5yQQpKdFK9Q0PD+vR0dH4HNHtjz3mOFqvX9lBSkkmHZBJB1wufuB1eb29OaB4zhoNnKnk/WBFjbpKXM9bF0QqB+clQ6xMPvC9vnaJPMfpF57NrxKlfNHtue76dom0VkHaz1y/SpRN57Zr7TjtE2lSKX/bKpHnezscvXYVvlwcrdGOM7hKpITcovSVVekLoZRCYDedI9qzZ4+rHd2v5OqX1pUihMB13e6RkZHUctQq5IM2pvYSvud1C5HJLouUMV2B5113DUR9xmnllkVC64Lruf3tFjmu06el170s6s12bXfamQln0VKpfD41tCzSrrNdO+1L7SWUVviOt0Kk1Bat2j4gHO0gtdosAYYfecT3HLdPtjG1l1BKIoUakAC5YjPvB16h7ZazBJ6Tl/Bx1dZab7lWIi8IChKg2gy/FgR+7lqJHKXWyZ179rhK6++7um1FezVCZiUzCw80olau2WpdM8/U/JyQFjO42Aj5cGr6mkjCZpPJqdlYCggBjk2eplheaKskSRLePXacE9MzB6VQ5hXATpXKvP7uYcoLi22RhM0mb40d5dD4cSw8r2YmJkq9g0PrgV2lao25UonA0WTSKa5kA0dxzPTMLAcPH+G/JyYx1h4c6+3aqwA2Fbr/HLnBXUIwVG2ETJ6ZobSwQBLHOFqhlF5Taq2l1YqoNxpMzc3xztFxDh57j5nyIhaOCKIHZ196qbL89bXj3t1Z5TReAL60dE1JSVc6RW8+Ry6dwvc8AtfFCgHW0oxiqrUq9VbEdLHMYr1ObJb/r/+2lei9x//+p1lYfV4ndg7f/3Vr7Q8RnFMpBB+/mpd/IzBYzPlfHpZ/C8mPD7/6l9+f338Vw8PDeto6I1LwkLXch2DThdqtCD4mpP1jYuzvjh7462tc4KTgUk4sxI3DD27W0gwkhj4p6MMYa4SsIMW4NM3xw6Oj1UuI06FDhw4dOnTo0KFDhw4dOnTo0KFDhw4dOvzf8D+CIQQwLMX+dAAAAABJRU5ErkJggg==",
      "title": "dwa_reservoir",
      "type": "layer"
    }
  ],
  "title": ""
}

The icon image is encoded in base64, and directly displayable in a web page.

Still WIP for the moment. I want to do more testing, and add unit tests. But preliminary comments are welcome.

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

@elpaso
Copy link
Contributor

elpaso commented Sep 13, 2019

Did you consider using nlohmann::json instead of QJsonDocument ?
It's generally faster and has a nicer API.

@elemoine
Copy link
Contributor Author

Did you consider using nlohmann::json instead of QJsonDocument ?
It's generally faster and has a nicer API.

You mean in my code? Or in the existing code written by @pblottiere?

@elpaso
Copy link
Contributor

elpaso commented Sep 13, 2019

nlohmann::json was probably introduced after original work by Paul, I was suggesting that from now on we should use it unless there is a good reason not to.
All new API and WFS3 which are both json-based use the nlohmann, and QgsJsonUtils does too.

@elemoine
Copy link
Contributor Author

nlohmann::json was probably introduced after original work by Paul, I was suggesting that from now on we should use it unless there is a good reason not to.
All new API and WFS3 which are both json-based use the nlohmann, and QgsJsonUtils does too.

Ok agree. But I do think this should be done with a separate PR.

@elpaso
Copy link
Contributor

elpaso commented Sep 13, 2019

nlohmann::json was probably introduced after original work by Paul, I was suggesting that from now on we should use it unless there is a good reason not to.
All new API and WFS3 which are both json-based use the nlohmann, and QgsJsonUtils does too.

Ok agree. But I do think this should be done with a separate PR.

No problems with that, as you prefer.

@rldhont rldhont added the Server Related to QGIS server label Sep 14, 2019
@rldhont rldhont added this to the 3.12 milestone Sep 14, 2019
@rldhont rldhont added Frozen Feature freeze - Do not merge! Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Sep 14, 2019
@elemoine elemoine changed the title [WIP] Add json support to WMS GetLegendGraphic Add json support to WMS GetLegendGraphic Sep 16, 2019
@elemoine
Copy link
Contributor Author

88093f438b703861efadee3556c15f83866857e3 adds unit tests.

I guess this is ready for more reviews. Thanks!

@rldhont rldhont added Feature and removed Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Sep 16, 2019
@pblottiere
Copy link
Member

Just some minor comments but it looks good to me, thanks @elemoine 👍!

This PR adds support for GetLegendGraphic responses encoded as JSON. It builds on previous work by @pblottiere, who added QgsLegendRenderer::exportLegendToJson for that exact purpose.

You made my day ;)

@elemoine
Copy link
Contributor Author

Just some minor comments but it looks good to me, thanks @elemoine +1!

I just took your comments into account. Thanks a lot for your review Paul!

@elemoine
Copy link
Contributor Author

No problems with that, as you prefer.

@elpaso happy to look at converting the code to using nlohmann::json when this is merged.

@elpaso
Copy link
Contributor

elpaso commented Sep 17, 2019

@elemoine perfect, thanks!

@elemoine elemoine force-pushed the ele_glg branch 2 times, most recently from e756b49 to 64ef4cb Compare September 23, 2019 13:50
@elemoine
Copy link
Contributor Author

@elpaso @pblottiere @rldhont any news with this? @rldhont added a Frozen label. What's the reason for this label? When will this be unfrozen? Thank you.

@nyalldawson
Copy link
Collaborator

What's the reason for this label? When will this be unfrozen? Thank you.

It's a feature which wasn't merged before freeze, so it needs to wait till 3.10 final release occurs and master branch is unfrozen

@elemoine
Copy link
Contributor Author

Thank you @nyalldawson

@rldhont
Copy link
Contributor

rldhont commented Sep 27, 2019

@elpaso, @pblottiere or others, do you think this PR could be exempt of freeze ?

@stale
Copy link

stale bot commented Oct 11, 2019

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 11, 2019
@rldhont
Copy link
Contributor

rldhont commented Oct 11, 2019

not stale, waiting for unfrozen master.

@stale stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 11, 2019
@rldhont
Copy link
Contributor

rldhont commented Oct 11, 2019

@elemoine there is a conflict with test file.

@elemoine
Copy link
Contributor Author

@rldhont thanks for letting me know. I just rebased my branch onto master, and pushed the result.

@rldhont rldhont added the Merge After Thaw For approved PRs which are ready to be merged as soon as master is thawed label Oct 18, 2019
@nyalldawson nyalldawson removed the Frozen Feature freeze - Do not merge! label Oct 25, 2019
@nyalldawson nyalldawson merged commit f3df32d into qgis:master Oct 26, 2019
@elemoine elemoine deleted the ele_glg branch October 29, 2019 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Merge After Thaw For approved PRs which are ready to be merged as soon as master is thawed Server Related to QGIS server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants