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.

Sign up for GitHub

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

[processing] use background color from the project settings in the rasterize algorithm (fix #19866) #8965

Merged
merged 3 commits into from Jan 24, 2019
Merged

[processing] use background color from the project settings in the rasterize algorithm (fix #19866) #8965

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c3ed75
[processing] use background color from the project settings in the
alexbruy Jan 22, 2019
f3eb3f1
use project from the algorithm context
alexbruy Jan 24, 2019
6df9ef1
move call to iface to the prepareAlgorithm method
alexbruy Jan 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions python/plugins/processing/algs/qgis/Rasterize.py
View file
Open in desktop
Expand Up @@ -260,10 +260,14 @@ def __init__(self, map_theme, layer, extent, tile_size, mupp, output,
self.settings.setFlag(QgsMapSettings.RenderMapTile, True)
self.settings.setFlag(QgsMapSettings.UseAdvancedEffects, True)

r = QgsProject.instance().readNumEntry('Gui', '/CanvasColorRedPart', 255)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm - this algorithm is rather badly behaved. It shouldn't be using QgsProject.instance(), rather the project passed in from the processing context.

It also access the map canvas in processAlgorithm! That's a big no - because it's access a gui widget from a background thread... :(

Copy link
Member

Choose a reason for hiding this comment

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

Could it be an alg parameter and the color selector has an easily accessible option to pull in the canvas bg color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be using QgsProject.instance(), rather the project passed in from the processing context.

Fixed.

It also access the map canvas in processAlgorithm! That's a big no - because it's access a gui widget from a background thread... :(

I don't know why it was accepted as is more than a year ago, see 525aeab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it be an alg parameter and the color selector has an easily accessible option to pull in the canvas bg color?

Personally I agree with issue described in the linked ticket. If I have custom canvas background I expect to get rasterized map with the same look-n-feel. Also, IMHO this does not break current behavior, as if canvas background it white it will work as previously.

Introducing new parameter should be possible, it is necessary to create a widget wrapper for string(?) parameter to show color selector

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I wish I'd picked this up earlier. A quick fix here would be to move that code to the prepareAlgorithm step, so at least it's run in the main thread. It's still not ideal, because we shouldn't allow algorithms to use iface at all (as it breaks when run in standalone scripts etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can access to iface in prepareAlgorithm be conditional to the presence of that interface? If not available, just use white as background color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nyalldawson moved to the prepareAlgorithm()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nirvn this is not related to the background color, iface used to access mapSettings object and get crs, layers etc.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this algorithm down't make much sense right in standalong code unless there are more config options for the rendering added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That may be the case - but I want to take a hard-line approach in future and block all use of iface from algorithms, just to be safe and to ensure that they work outside of QGIS. I've got worked planned in future to allow execution of algorithms from a standalone executable (e.g. something like "qgis_run --alg qgis:buffer --DISTANCE=5 --INPUT=/home/nyall/blah.shp etc"), and processing algorithms are also used as a WPS server (via plugin).

For this alg, I think the correct solution would be to add the required settings as advanced options to the algorithm, and make a custom dialog/widget which auto populates them based on the canvas settings. OR take the default values of these from the canvas when it's available, and if not, just use some hardcoded value.

(For reference, I do the second approach for a number of plugin-specific algorithms. )

g = QgsProject.instance().readNumEntry('Gui', '/CanvasColorGreenPart', 255)[0]
b = QgsProject.instance().readNumEntry('Gui', '/CanvasColorBluePart', 255)[0]
if make_trans:
self.settings.setBackgroundColor(QColor(255, 255, 255, 0))
self.bgColor = QColor(r, g, b, 0)
else:
self.settings.setBackgroundColor(QColor(255, 255, 255))
self.bgColor = QColor(r, g, b)
self.settings.setBackgroundColor(self.bgColor)

if QgsProject.instance().mapThemeCollection().hasMapTheme(map_theme):
self.settings.setLayers(
Expand Down Expand Up @@ -300,11 +304,9 @@ def renderTile(self, x, y, feedback, make_trans):
"""

if make_trans:
background_color = QColor(255, 255, 255, 0)
self.image.fill(background_color.rgba())
self.image.fill(self.bgColor.rgba())
else:
background_color = QColor(255, 255, 255)
self.image.fill(background_color.rgb())
self.image.fill(self.bgColor.rgb())

painter = QPainter(self.image)

Expand Down