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

Allow changing the map extent properly when showing the map #62

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

Joonalai
Copy link
Contributor

@Joonalai Joonalai commented May 10, 2024

This PR fixes a bug where adding map layer causes the map to zoom to its whole extent even though the extent is changed after adding the layer.

This should be merged after #60 (CI changes) and added to the upcoming release.

@Joonalai
Copy link
Contributor Author

@LKajan could you please review this? This should be added to the upcoming release as well.

Copy link
Contributor

@LKajan LKajan left a comment

Choose a reason for hiding this comment

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

Do I understand the issue correctly? The generated map doesn't respect the extent set explicitly by qgis_canvas.setExtent in the test but uses the extent from layers added?

src/pytest_qgis/pytest_qgis.py Outdated Show resolved Hide resolved
src/pytest_qgis/pytest_qgis.py Outdated Show resolved Hide resolved
tests/visual/test_show_map.py Outdated Show resolved Hide resolved
tests/visual/test_show_map.py Outdated Show resolved Hide resolved
Copy link
Contributor

@LKajan LKajan left a comment

Choose a reason for hiding this comment

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

Thinking about this further, I realized that this also changes the behavior of the program tested. Which can lead to false test results when it runs differently during tests and real execution.

Do you think it work to create for example a method like:

def set_extent_after_events_processed(extent):
    _APP.processEvents()
    canvas.setExtent(extent)

and call it in test methods if the extent is to be changed?

@Joonalai
Copy link
Contributor Author

Thinking about this further, I realized that this also changes the behavior of the program tested. Which can lead to false test results when it runs differently during tests and real execution.

The thing is that it really doesn't change the behavior. In QGIS the events are processed all the time but in test this is not the case. I think it definitely is more intuitive that the plugin behaves more like QGIS than not.

Do you think it work to create for example a method like:

def set_extent_after_events_processed(extent):
    _APP.processEvents()
    canvas.setExtent(extent)

and call it in test methods if the extent is to be changed?

I don't really think this is a good idea since the main idea of the plugin is to act in the background and let the user use PyQGIS API to run QGIS code in tests. What if the extent is changed in code rather than in test? Should user mock the canvas.setExtent to call this function instead?

@Joonalai
Copy link
Contributor Author

Joonalai commented Jun 4, 2024

@LKajan, @ismogis what do you think, is there room for improvement or should this be merged?

Copy link
Contributor

@LKajan LKajan left a comment

Choose a reason for hiding this comment

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

I'm still a little bit unsure of all side effects this causes.
This includes clear comment though so in my opinion this can be merged pending a better way to resolve the matter.

@LKajan LKajan merged commit eaa39d5 into GispoCoding:main Jun 14, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants