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
Fix regression server print selection pdf release 3.4 #9692
Fix regression server print selection pdf release 3.4 #9692
Conversation
Hi @elpaso , @pblottiere now that #9654 has been merged and waiting for backport #9705 can you review this one ? thanks |
@@ -490,6 +490,8 @@ QgsLayoutExporter::ExportResult QgsLayoutExporter::exportToPdf( const QString &f | |||
( void )contextRestorer; | |||
mLayout->renderContext().setDpi( settings.dpi ); | |||
|
|||
mLayout->renderContext().setFlags( settings.flags ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, but this change may only have been ok in 3.6/master because of corresponding changes in the layout designer dialog. Can you look through the git blame on master for when I introduced this and please confirm that there's no related changes made in the designer to accompany this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyalldawson you're right an other commit is associated in the PR [needs-docs][layouts] Add checkbox to disable raster tiling for PDF/SVG exports #9016.
Do you think the other commit has to be backported ?
Does the Fix inconsistent use of layout render context flags 2752f83 introduced some regressions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue fixed by your PR is https://issues.qgis.org/issues/19500 Layout export - raster divided into tiles, edges evident in pdf/svg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyalldawson can I cherry-pick 60b8d05#diff-b7c7a3b8898bc4177771ab86148a8348 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not linked to layout designer dialog. I have introduced flag in layout to print selection in server context.
I have done the code for release 3.4.0 with the PR #8320 [Server] Reactivate the capability to print selection with Server 3.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nyalldawson,
The mLayout->renderContext().setFlags( settings.flags );
has been introduced before the Refactor layout context 5bc543a
https://github.com/qgis/QGIS/blame/master/src/core/layout/qgslayoutexporter.cpp#L356 in QGIS 3.0.0 in method exportToImage
because this commit only rename the context access in layout.
The mLayout->context().setFlags( settings.flags );
has been introduced before the **Work on PDF export ** 91179f1 in QGIS 3.0.0 in which you introduce exportToPdf
and the setFlags
missed.
In this commit you wrote
// If we are not printing as raster, temporarily disable advanced effects
// as QPrinter does not support composition modes and can result
// in items missing from the output
mLayout->context().setFlag( QgsLayoutContext::FlagUseAdvancedEffects, settings.rasteriseWholeImage );
But flags are not set before.
The mLayout->context().setFlags( settings.flags );
in exportToImage
has been introduced by Expose antialiasing option in image export dialog 2f0969e#diff-7393cc3d739cdcd8a6531909637b63b5
Hi @nyalldawson, I would like to know what can I do to fix tise server regression ? Thanks for your review. |
Hi @nyalldawson I have cherry-pick 60b8d05 so this PR is also a backport of PR [needs-docs][layouts] Add checkbox to disable raster tiling for PDF/SVG exports #9016. |
Thanks @rldhont. I don't have time to review this until late next week, so PLEASE thoroughly test that theres no regressions in the designer GUI. It makes me quite nervous backporting this to the stable LTR branch. |
@nyalldawson thanks for your reply, I'll test the UI. Do you think someone else can review this ? |
c54d86e
to
30d147a
Compare
@nyalldawson tested without issue |
Can I merge ? |
@nyalldawson Now that QGIS 3.4.7 has been released, do you think, I can merge this ? Other devs can test it ? |
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
|
Hi @nyalldawson, can I have a decision about this bugfix ? |
Hi @nyalldawson, Do you think it is acceptable to only backport the commit 0954deaf97638519e9f4b3e28855e03a8a34f5c1 Fix inconsistent use of layout render context flags |
…ormat In QGIS 3.4, Selection can be printed in Image output and not in PDF or SVG output. A fix has been done 2752f83 to fix inconsistent use of layout render context flags, and draw selection is activated with a flag.
30d147a
to
ea5bcdc
Compare
Hi @nyalldawson, The |
Hi @rldhont, Considering the fact that the impact on the code is pretty minimal, I'd be in favor of merging this PR. Moreover, unit tests have been added too. Initially, some worries had been expressed about border effects on the designer GUI, but I think that you have tested it, isn't it? If so, I propose to merge this PR in the coming days, unless an objection is raised in the meantime. |
Hi @pblottiere, I have tested the GUI and I didn't see any border effects. Thanks, |
Thanks @pblottiere |
Description
Manually backporting fix inconsistent use of layout render context flags 2752f83 to fix draw selection in Server GetPrint Request.
And backporting unit-tests to avoid regression in QGIS Server context.
Checklist
scripts/prepare-commit.sh
script before each commit