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] Regression in Composer legend, layer's title was not the default legend's title #7260

Closed

Conversation

rldhont
Copy link
Contributor

@rldhont rldhont commented Jun 18, 2018

Description

In release-2_14, the layer's title was the default legend's title. This
capabilities has been lost with the use of the QgsLegendModelV2 in
QgsLegendRenderer.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@nyalldawson
Copy link
Collaborator

Why would we use the title here? From the dox for QgsMapLayer::title(): "Returns the title of the layer used by QGIS Server in GetCapabilities request."

That doesn't sound related to legend titles to me.

@rldhont
Copy link
Contributor Author

rldhont commented Jun 18, 2018

@nyalldawson I probably have to update the dox. But in release-2_14 and before, the layer's title was used as the layer legend's title.

@wonder-sk
Copy link
Member

But this change would also change behavior of non-server code, right? That could be regarded as a regression by QGIS app users...

@rldhont
Copy link
Contributor Author

rldhont commented Jun 19, 2018

@wonder-sk Yes, this PR is not only for server it is also to fix a QGIS Desktop regression.

@nyalldawson
Copy link
Collaborator

I'm not too strong on server use, but too me this would be unexpected behaviour. The server metadata title and the name used in a legend may have no relation to each other, and it's standard qgis behaviour to take the layer's name for legends. I think this is quite an edge case which doesn't warrant changing the default behaviour for.

@m-kuhn m-kuhn closed this Jun 19, 2018
@m-kuhn m-kuhn reopened this Jun 19, 2018
@rldhont
Copy link
Contributor Author

rldhont commented Jun 19, 2018

The title was firstly designed for server, but it's a metadata and has been used as a title in legend. The name is for Desktop use, it can be the title or a simplify name to easily identify layers in the interface.

This PR is a fix for a regression compare to the old LTR. This feature has been introduce in desktop by #824.

@rldhont
Copy link
Contributor Author

rldhont commented Jun 19, 2018

This feature is in QGIS since 2.0

@nyalldawson
Copy link
Collaborator

I guess ultimately I'm a 0 here because I'm not a server end user so don't have the required knowledge to comment.

@m-kuhn
Copy link
Member

m-kuhn commented Jun 19, 2018

For my understanding, what's wrong with using QgsMapLayer::name()?
QgsMapLayer::title() is explicitly tagged in the API as being "used by QGIS Server in GetCapabilities request", so a bit unexpected to influence desktop behavior.

I have no strong opinion (yet), just wondering

  • about the clean way to deal with those two potentially conflicting QgsMapLayer properties
  • about the risk of breaking previously working 2.18 projects (compatibility with these is more important than compatibility with 2.14 behavior)

@wonder-sk
Copy link
Member

In #824 the logic seems more complicated than what I see here... it only applied to single symbol renderer, and it tried to use a text in these priorities: 1. symbol's custom text, 2. layer's custom text, 3. layer's title(), 4. layer's name()

I guess I am also 0 on this...

@rldhont
Copy link
Contributor Author

rldhont commented Jun 19, 2018

@m-kuhn the documentation about QgsMapLayer::title() is not complete. Since the first release of QGIS 2.0, if a title is defined it is used as the default layer name in the Composer legend.

Actually QGIS 2.18 breaks project build with 2.14, because the legend rendering is not based on the same model. Some user does not update to 2.18 because of some regressions in Composer, that I am looking to fix like #7250

@rldhont
Copy link
Contributor Author

rldhont commented Jun 19, 2018

@wonder-sk the old legend model was more complex.

@nyalldawson
Copy link
Collaborator

the documentation about QgsMapLayer::title() is not complete

But it's not just the documentation - it's how this setting is exposed within the qgis UI too.

@m-kuhn
Copy link
Member

m-kuhn commented Jun 20, 2018

I'm still struggling to see the difference between title and name. What's the use-case where you need them to be different and - as a user - how do I know how this works?

@mdouchin
Copy link
Contributor

I agree with @rldhont : as a QGIS user, when I load layers in my project, from Shapefiles, Gpkg or PostgreSQL, I do not rename them in the layer panel (the "legend") because I like to see the real source name. For example I keep "main_roads" instead of renaming to "Main roads". In this context, I like to use the metadata "title" property as a replacement. Inside the composer, I would like to have the titles used, because my exported PDF is for my colleague and not me. And I do not want to rename all the item in my composer legend manually.
The behaviour has changed between 2.14 (ex LTR) and 2.18, and I think it is a regression

@nyalldawson
Copy link
Collaborator

@mdouchin

That sounds like an edge case to me - the original layer source is already visible in the layer tooltips and from the layer properties window.

My issue here is the misleading reuse of an unrelated setting - the title field is quite clearly marked as applying only to qgis server.

…ault legend's title

In release-2_14, the layer's title was the default legend's title. This
capabilities has been lost with the use of the QgsLegendModelV2 in
QgsLegendRenderer.
@rldhont rldhont force-pushed the composer-legend-layer-title-218 branch from c41f2e2 to f9bd907 Compare June 20, 2018 13:27
Since QGIS 2.0, the layer's title is used as the default layer legend's name.
@nyalldawson nyalldawson modified the milestones: 2.18.21, 2.18.22 Jun 22, 2018
@nyalldawson
Copy link
Collaborator

Ping @probot

@nyalldawson nyalldawson reopened this Jun 28, 2018
@stale
Copy link

stale bot commented Jul 12, 2018

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 Jul 12, 2018
@stale
Copy link

stale bot commented Jul 19, 2018

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@stale stale bot closed this Jul 19, 2018
@rldhont rldhont deleted the composer-legend-layer-title-218 branch January 2, 2020 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Forwardporting stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants