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

pygmt.set_display: Fix the bug that method=None doesn't reset to the default display method #3396

Merged
merged 20 commits into from
Sep 3, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Aug 13, 2024

Description of proposed changes

pygmt.set_display(method=None) should revert the display method to the default value, but it doens't work. Here is a minimal example to reproduce the issue:

In [1]: import pygmt
In [2]: fig = pygmt.Figure()
In [3]: fig.basemap(region=[0, 10, 0, 10], projection="X4c/4c", frame=True)
In [4]: fig.show()  # Display the image in an external viewer
In [5]: pygmt.set_display(method="none")  # Disable image preview 
In [6]: fig.show()  # Image is not displayed
In [7]: pygmt.set_display(method=None)   # Should revert to the default display method
In [8]: fig.show()  # Image is not displayed. WRONG!

In the last fig.show() call, the image should be displayed, but it doesn't. It's because method=None is ignored in pygmt.set_display.

This PR fixes the bug and does some refactorings.

  • 2409284: Add the _get_default_display_method function so that the same logic of codes can be called when method=None is used.
  • 6bd742e: Refactor set_display to fix the bug.

Ideally, we should add a test to check if SHOW_CONFIG["method"] matches the value in pygmt.set_display(method="XXX"). However, SHOW_CONFIG is a global variable, changing its value will affect other tests. So, no test is added.

@seisman seisman added bug Something isn't working needs review This PR has higher priority and needs review. labels Aug 13, 2024
@seisman seisman added this to the 0.13.0 milestone Aug 13, 2024
pygmt/figure.py Outdated Show resolved Hide resolved
@seisman seisman changed the title Refactor and add the _get_default_display_method function pygmt.set_display: Fix the bug that method=None doesn't reset to the default display method Aug 13, 2024
pygmt/figure.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Aug 28, 2024

Need to wait for #3418.

@seisman seisman removed the needs review This PR has higher priority and needs review. label Aug 28, 2024
@seisman seisman force-pushed the refactor/default_display branch from 5bc1392 to a570ce2 Compare August 29, 2024 12:17
@seisman seisman changed the base branch from main to bug/external-display August 29, 2024 14:20
@seisman seisman force-pushed the refactor/default_display branch from a570ce2 to 7ea264e Compare August 30, 2024 00:15
Base automatically changed from bug/external-display to main September 1, 2024 06:56
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Sep 2, 2024
@seisman
Copy link
Member Author

seisman commented Sep 2, 2024

Ping @GenericMappingTools/pygmt-maintainers for final review.

def test_figure_set_display_invalid():
class TestSetDisplay:
Copy link
Member

Choose a reason for hiding this comment

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

We don't often use classes for the pytest unit tests, is there a reason to use it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just want to group multiple tests so that it is easier to find all tests for testing a specific function.

Copy link
Member

@weiji14 weiji14 Sep 2, 2024

Choose a reason for hiding this comment

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

Ok, it looks like we're using TestGetDefaultDisplayMethod below also, so ok with this.

@seisman seisman merged commit f9cf17c into main Sep 3, 2024
22 checks passed
@seisman seisman deleted the refactor/default_display branch September 3, 2024 02:59
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants