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

Jump to bottom

Layout raster image exports #5837

Merged
merged 56 commits into from Dec 17, 2017
Merged

Layout raster image exports #5837

merged 56 commits into from Dec 17, 2017

Conversation

nyalldawson
Copy link
Collaborator

Restores raster image exports for layouts.

Big improvement here is that all the useful functionality is now exposed through QgsLayoutExporter. In composer in 2.x it was a pain to export compositions because so much of the useful api was locked away in app. This led to a ton of duplicated code for every plugin dealing with compositions...

@NathanW2
Copy link
Member

NathanW2 commented Dec 11, 2017 via email

@nirvn
Copy link
Contributor

nirvn commented Dec 13, 2017

@nyalldawson , page size is wrong for PDF outputs:
screenshot from 2017-12-13 11-38-47

@nirvn
Copy link
Contributor

nirvn commented Dec 13, 2017

BTW, I think we need to add a "Add page(s)..." toolbar button somewhere (possibly in the toolbox's toolbar?). I keep looking for it.

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/app/layout/qgslayoutdesignerdialog.cpp Outdated
break;

case QgsLayoutExporter::MemoryError:
QMessageBox::warning( nullptr, tr( "Memory Allocation Error" ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it wanted that the parent of QMessageBox is this in case of FileError and nullptr in case of MemoryError?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch - it was a copy/paste from the old composer code. I'll add the parent to all these. Or @nirvn should I use the message bar here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/app/layout/qgslayoutdesignerdialog.cpp Outdated


case QgsLayoutExporter::MemoryError:
QMessageBox::warning( nullptr, tr( "Memory Allocation Error" ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about the parent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/app/layout/qgslayoutdesignerdialog.cpp Outdated
//make sure print as raster checkbox is updated
mLayoutPropertiesWidget->updateGui();

delete m;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that a unique_ptr could used in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - actually doesn't need to be on the heap here at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/app/layout/qgslayoutdesignerdialog.cpp Outdated
{
settings.setValue( QStringLiteral( "LayoutDesigner/hideForceVectorWarning" ), true, QgsSettings::App );
}
delete m;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about unique_ptr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/app/layout/qgslayoutimageexportoptionsdialog.h


/**
* A dialog for customising the properties of an exported image file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\since QGIS 3.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure here. It's in app, so is the version information really useful for anything? Happy to add it if you think there's value.

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp
return renderRegionToImage( paperRect, imageSize, dpi );
}

///@cond PRIVATE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain to me the meaning of this notation ///@cond PRIVATE? Thank you :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used to hide code from doxygen. We use it to mark parts of headers (and cpp files) which we don't want doxygen to know about, with the belief that hiding them from doxygen is sufficient enough to dissuade most users from relying on non-stable api.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that because we have doxygen also scanning cpp files (can't remember why, but there was a good reason for it sometime ago) you need to make sure the cond notation is used both in headers and cpp files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thank you for the explanation, I didn't know that!

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp Outdated
LayoutItemCacheSettingRestorer( QgsLayout *layout )
: mLayout( layout )
{
mLayout->context().mIsPreviewRender = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mLayout could be checked to ensure that it's not null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll just assert here... there's absolutely no valid use for this object with no layout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp

~LayoutItemCacheSettingRestorer()
{
for ( auto it = mPrevCacheMode.constBegin(); it != mPrevCacheMode.constEnd(); ++it )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a lot of auto in QGIS source code. Do we have some kind of rules about its usage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not officially (but we should open a discussion on dev list). From scattered conversation my summary is:

  • don't use auto for declaring most variables. Explicit type declaration is preferred, as it helps readability and also qt creator code completion.
  • let's use it only for annoying declarations, like QMap< blah, blah >::const_iterator

I'm happy with this approach... personally I hate having to declare iterators, so always use auto for them now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • don't use auto for declaring most variables. Explicit type declaration is preferred, as it helps readability and also qt creator code completion.
  • let's use it only for annoying declarations, like QMap< blah, blah >::const_iterator

Fully agree 👍

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp
LayoutContextSettingsRestorer( QgsLayout *layout )
: mLayout( layout )
, mPreviousDpi( layout->context().dpi() )
, mPreviousFlags( layout->context().flags() )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could check that layout is not null before using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, there's no use for this class if layout is null. If it is, an assert would be better... but we would have crashed long before this class is used anyway :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp
~LayoutContextSettingsRestorer()
{
mLayout->context().setDpi( mPreviousDpi );
mLayout->context().setFlags( mPreviousFlags );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that mLayout may be a nullptr in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp Outdated
{
ImageExportSettings settings = s;
if ( settings.dpi <= 0 )
settings.dpi = mLayout->context().dpi();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that mLayout may be null at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I think it may be the case in other methods too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid point for QgsLayoutExporter. I'll make the constructor require a reference instead of the pointer to avoid this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp Outdated
GDALSetProjection( outputDS.get(), map->crs().toWkt().toLocal8Bit().constData() );
}
CPLSetConfigOption( "GDAL_PDF_DPI", nullptr );
delete[] t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that a unique_ptr may be used for t?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutexporter.cpp
{
if ( details.page == 0 )
{
return details.directory + '/' + details.baseName + '.' + details.extension;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A QStringLiteral().arg() is not to be preferred in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure here - my understanding is that the QString single character based methods are very fast, so I suspect that the extra allocations here would be faster than parsing the string literal for the % tokens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably QStringBuilder would be preferable here actually

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably QStringBuilder would be preferable here actually

Good point

pblottiere
pblottiere reviewed Dec 13, 2017
View reviewed changes
src/core/layout/qgslayoutpagecollection.cpp
{
double maxArea = 0;
QSizeF maxSize;
for ( QgsLayoutItemPage *page : mPages )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not qgis::as_const( mPages ) in this case? Sorry if it's a dumb question...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already const here, because the method it's contained in is const.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed :)

@pblottiere
Copy link
Member

Hi @nyalldawson!

Sorry, I have mainly questions and not so much valuable comments...

In any case, good job 👍 :)

@nyalldawson
Copy link
Collaborator Author

@pblottiere

Thanks so much for the review - it's very much appreciated!!

@nyalldawson
Copy link
Collaborator Author

@nirvn

page size is wrong for PDF outputs:

Fixed

@nyalldawson
Copy link
Collaborator Author

Ok so the description of this PR is a travesty.

There's LOTS of cool introduced by this PR now, including a new messagebar in layouts for feedback after exports, complete with hyperlinks to immediately open the outputed file for testing, and a new option to control antialiasing or not for raster exports.

But the main killer feature here now is that PDF exports no longer force rasterization of the entire output if map layer settings such as blend modes or layer opacity are present!! Now, there's more intelligent logic in place so that only items which HAVE to be rasterized are... this means the map may be included as a raster image in the PDF, while all the rest of the accompanying items will be left as vectors or pure text objects. No more rasterized legends just to have layer opacity in your maps!

To accompany this there's a new layout setting to "force vector output". If set, the output will always be vectors, even when it changes the actual appearance of the map.

@nirvn
Copy link
Contributor

nirvn commented Dec 15, 2017

YAY!

@nyalldawson
Copy link
Collaborator Author

@nirvn / others. Keen for some UX advice.

Currently, after exporting a layout you get a message bar with a clickable link which directly opens the generated image/pdf in the default program.

I'm thinking of changing this behavior so that clicking the link opens instead the folder containing the exported images/pdfs. To me this would be more flexible - it's still only going to be an extra double click to open the generated files if you want to view them, but opening instead the folder would also allow much more options, like right clicking and opening in a non-default application, sending via email, renaming/copying/moving, etc...

Thoughts?

@nyalldawson nyalldawson force-pushed the layout_next branch 2 times, most recently from 2144711 to ea38a27 Compare December 16, 2017 01:25
@nirvn
Copy link
Contributor

nirvn commented Dec 16, 2017

+1 to opening the folder, with the exported file selected if at all possible.

@nyalldawson nyalldawson reopened this Dec 16, 2017
@nyalldawson
Make QgsLayoutExporter::generateFileName virtual, so exporter
8b1e057 
subclasses can be made which customise the generated file names
@nyalldawson
Update tests masks
0110b43
@nyalldawson
Move containsAdvancedEffects test to QgsLayoutItem
a59dce5 
And start a generic test library for all item types to ensure
correct behavior for QgsLayoutItem subclasses

Currently justs tests to ensure that overriden
containsAdvancedEffects methods also call the base class
test
@nyalldawson
Work on PDF export
91179f1
@nyalldawson
[layouts][FEATURE] Don't force the whole layout to be rasterized
b992e87 
when exporting to PDF

If an individual layout item needs rasterisation in order to
be exported correctly, it can now be individually rasterised
without forcing every other item to also be rasterised.

This allows exports to PDF keeping as much as possible as vectors,
e.g. a map with layer opacity won't force labels, scalebars, etc
to be rasterised too.

To accompany this, a new "Always export as vectors" checkbox
was added to layout properties. If checked, this will force
the export to keep items as vectors, even when it causes the
output to look different to layouts.

Fixes qgis#7885
@nyalldawson
Add a messagebar to layout designer
261492d
@nyalldawson
Tweak logic regarding when a map item forces the whole layout
d3aee95 
to be rasterised
@nyalldawson
Tweak logic regarding page item margins following forced re-render
2b0ed50 
of layout items when exporting (i.e. disabling of cached item render)

The old issue of semi-transparent pixels around the edge of the page
had reared again. This is caused by the antialiasing while rendering
the page symbol. In order to avoid this, we cater to the most common
use case of having pages with a solid, borderless fill and slightly
extend the fill symbol polygon outside the page by 2 pixels
(determined by trial-and-error). The less common use case of having
a page symbol containing a border suffers by this border being
clipped by a couple of pixels, but we must address the much more
common use case over this.
@nyalldawson
Update test masks
c7abf46
@nyalldawson
Fix crash in ui while working with map grids
ab7873c
@nyalldawson
Fix incorrect detection of grids with advanced effects
770ffdf
@nyalldawson
Fix drawing of map items (grids, overviews) when rendering map item a…
ca37a1e 
…s a raster item
@nyalldawson
Add new page icon to toolbar
5601fc9
@nyalldawson
Add some layout related actions to app toolbar
2631589
@nyalldawson
Add links to open exported images in message bar
3704c48
@nyalldawson
[layouts][needs-docs] Add control for whether pages should be
f649f1f 
exported, including data defined setting

This replaces the 2.x data-defined "number of pages" setting.
Instead of requiring users to develop an expression to return
the number of pages, instead we allow individual pages to have
a data defined control of whether that page should be included
in the export.

This is more flexible, and works correctly with the mixed page
size model for layouts.
@nyalldawson
Fix items moving after altering page size or inserting/deleting pages
447a949
@nyalldawson
Port orientation decoding code from composer
65f4c4a
@nyalldawson
Restore data defined page orientation
5d6a509
@nyalldawson
Restore data defined page orientation control
db17c2c
@nyalldawson
Fix some multiple updates from gui, squash some undo commands
8f0144b
@nyalldawson
Fix pdf export page sizes
eb25ab7
@nyalldawson
Add missing parents to message boxes
8d1e717
@nyalldawson
Better memory management
9ab813f
@nyalldawson
Guard QgsLayoutExporter against nullptrs
ffb9d0c
@nyalldawson
Use unique_ptr over raw array
662ec7a
@nyalldawson
Sipify
831732f
@nyalldawson
Update test mask images
492f9ea
@nyalldawson nyalldawson merged commit b59bd94 into qgis:master Dec 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pblottiere pblottiere pblottiere left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nyalldawson @NathanW2 @nirvn @pblottiere