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

[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

Conversation

alexbruy
Copy link
Contributor

@alexbruy alexbruy commented Jan 24, 2019

Description

Native rasterize algorithm ignores project background color and always outputs raster with white background. Proposed fix takes into account project background color making users who non-standard backgrounds a bit more happy.

Fixes https://issues.qgis.org/issues/19866.

@m-kuhn I will be grateful for your review and opinion.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • 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

@@ -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. )

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