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

Partial fix for plot title not updating when plot is open #36839

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

jhaigh0
Copy link
Contributor

@jhaigh0 jhaigh0 commented Feb 12, 2024

Description of work

I think this was introduced with some combination of #36393 and mpl updating. If you never set up the call back on the title then it works, but I also had to alter the function to update the axes title (called after changing the plot title in the plot list) which isn't affected by the call back at all.

This PR introduces a couple of draw_idle() calls to try and keep the title rendering up to date. It also adds an override of set_title in the MantidAxes class so that it copies the current font properties to the new text.

The fix is partial because while the text updates, the font properties are not updated until the title is set again (you can even just double-click the title and click okay).

Summary of work

Some (potential useful) other things I found / tried

  • Removing the call to get_window_title() and just hard-coding the title as "ws-1" solve the problem, strangely. I tried putting the qt function calls in get_window_title and set_window_title on their own thread using QAppThreadCall but that didn't produce any different results. This makes me think there is some underlying thread issue, possibly also responsible for plotSpectrum() hang #36727.
  • Looking at the mpl code for Axes.set_title, two calls to update() are made, first setting the text and default font properties, second setting any specified font properties (i.e fontsize). update() is the function which calls ends up calling the callbacks. I thought if I could notice font property changes in my call back as well as text changes, then I could force a draw_idle for those too. I could never quite get this to work; it began more and more to feel like something I shouldn't be having to do. The two separate calls would point quite nicely to why this fix is only a partial one, with the text updating but not the font properties.

Fixes #36771

To test:

  • Run the script from the issue. The title will update text-wise, but not the font size.
  • Setting the title through double-clicking the title, changing in the plot settings, changing the title in the plot selector widget (lower left), should then update the title with the new name and correct font size.
  • Try running `plt.title("hello123") in the ipython shell, this should update the title too.
  • Tests from the PR Add tracking between axis title, window title, and plot name #36393 should all still work

This does not require release notes because it was introduced during 6.9 development.


Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

@MohamedAlmaki MohamedAlmaki self-assigned this Feb 13, 2024
@sf1919 sf1919 added this to the Release 6.9 milestone Feb 14, 2024
MohamedAlmaki
MohamedAlmaki previously approved these changes Feb 14, 2024
Copy link
Contributor

@MohamedAlmaki MohamedAlmaki left a comment

Choose a reason for hiding this comment

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

Looks good, works as expected. I guess it is better to remove fixes as this PR doesn't fully resolve the issue

@SilkeSchomann
Copy link
Contributor

I think this should go into release-next.

@jhaigh0 jhaigh0 changed the base branch from main to release-next February 14, 2024 10:02
@jhaigh0 jhaigh0 dismissed MohamedAlmaki’s stale review February 14, 2024 10:02

The base branch was changed.

@SilkeSchomann SilkeSchomann merged commit d605fc1 into release-next Feb 14, 2024
9 checks passed
@SilkeSchomann SilkeSchomann deleted the 36771_plot_title_not_updating_when_open branch February 14, 2024 10:02
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.

Setting the title of an open plot doesn't update the plot
4 participants