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

Remove hardcoded limit (fixes #17689) #7399

Merged
merged 1 commit into from
Jul 20, 2018

Conversation

pblottiere
Copy link
Member

@pblottiere pblottiere commented Jul 11, 2018

Description

This PR removes a hardcoded value limiting the maximum number of pages in the atlas.

The same hardcoded limit is in 2.18 branch, so this PR should be backported once terminated.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • 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

@pblottiere
Copy link
Member Author

@nyalldawson Do you think it's safe or do we want to add a setting to manage a upper bound to avoid some performance issues in case of too many pages?

@nyalldawson
Copy link
Collaborator

I think we need a limit here. I've read about atlas exports exceeding 10k pages (although never tried this myself).

@nyalldawson nyalldawson added this to the 3.4 milestone Jul 11, 2018
@nyalldawson nyalldawson self-assigned this Jul 11, 2018
@pblottiere
Copy link
Member Author

I think we need a limit here.

@nyalldawson Do you think it should be an advanced option only or a parameter configurable through the layout option panel?

@nyalldawson
Copy link
Collaborator

I think it should be hardcoded - there's no reason a user should have to set/change this.

I'd suggest running some tests with large layers used as atlas, with expression based page name, and see if you can determine a reasonable maximum size for the combo box before performance is degraded.

@haubourg
Copy link
Member

@nyalldawson Hi Nyall, I just had a try with a 120 K feature layers here. It loads fast (2s) and works smoothly when loaded.

image

I tracked memory and CPU, and I see only a reasonable surge during the loading.

What is the rational for keeping a hardcoded value? What caveats do you foresee that we could test more precisely?

@nyalldawson
Copy link
Collaborator

Well, what if someone accidentally selects a coverage layer with 10 million features? What about 100million? I think there should be an upper limit here - even if it's 150k.

@nyalldawson
Copy link
Collaborator

Just to add to this - the combo box isn't the only way to navigate to a particular atlas feature. There's also the " set as atlas feature" action, which was designed to allow jumping directly to a particular feature in a huge layer (because it works with map based selections and filters inside the attribute table).

@giswgjs
Copy link

giswgjs commented Jul 19, 2018

I think this closed then? https://issues.qgis.org/issues/17689

@jef-n jef-n changed the title Remove hardcoded limit Remove hardcoded limit (fixes #17689) Jul 19, 2018
@haubourg
Copy link
Member

@nyalldawson ok, we can keep a higher limit value, however 500 was really a low one. I think users can understand we don't go upper than some thousands. What about 100K ?
We needed also to backport this to 2.18 where the set atlas feature is not native.
@pblottiere thoughts?

@nyalldawson
Copy link
Collaborator

100k is fine for me!

But fyi, set atlas feature action has been around since... Maybe 2.4?

@pblottiere
Copy link
Member Author

I increased the limit to 100k.

@nyalldawson do you agree to merge?

@nyalldawson
Copy link
Collaborator

You may surely proceed with the knowledge of my full blessing kind sir

@pblottiere pblottiere merged commit dbd5a86 into qgis:master Jul 20, 2018
@pblottiere
Copy link
Member Author

I think this closed then? https://issues.qgis.org/issues/17689

@giswgjs The issue is closed now. I'm going to backport the bugfix on 2.18 branch.

Thanks for pointing that out!

@pblottiere pblottiere mentioned this pull request Jul 20, 2018
8 tasks
@haubourg
Copy link
Member

set atlas feature action

@nyalldawson Wow ok, I knew the python action, I thought something new was added in 3.0 - and as I am in vacations, I didn't check. that said, Adding a python action needs some work and advanced knowledge, so this is a satisfying solution for application-like qgis projects, but less for average user.

@haubourg
Copy link
Member

Anyway, thanks @pblottiere and @nyalldawson for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants