Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid parenting mess by calling the widget directly #2559

Merged

Conversation

rozyczko
Copy link
Member

@rozyczko rozyczko commented Jul 19, 2023

Description

When counting instances of plots, one needs to be careful with QtWidget parentage being updated.
The existing Manager->Plotter->PlotterWidget chain was working with Qt5 (and from what I remember was necessary to achieve correct MDI space location) but started failing with Qt6.
Getting rid of Plotter and instantiating PlotterWidget directly seems to be safer.

Fixes #2527

How Has This Been Tested?

local testing on win10

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 25, 2023
@butlerpd butlerpd requested review from tsole0 and nouhaag July 25, 2023 13:42
@butlerpd
Copy link
Member

can merge as soon as the two reviews are positive

Copy link
Contributor

@tsole0 tsole0 left a comment

Choose a reason for hiding this comment

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

Changes work on MacOS and Windows on installer and in developer environment. Code is consistent with methods for ensuring child windows are created with proper high-level parents, and is compatible with opening additional windows from Plotter2DWidget.

This isn't the only place Plotter or Plotter2D is called, however, so issues stemming from window hierarchy may still arise in other parts of SasView, such as in src/sas/qtgui/Utilities/ImageViewer.py (lines 142-146):

    def addPlotter(self):
        """
        Add a new plotter to the frame
        """
        self.plotter = Plotter2D(self, quickplot=True)

where Plotter2D is still called.
Because I believe Plotter2D was kept around from pre-PySide and its functionality has been supplanted by Plotter2DWidget, I wonder if we need to keep it around at all. Removing it now though would raise a lot of other problems in dependent code (as noted above), so I believe that is better left to a separate PR.

I think this PR is good to go, especially considering this PR is important for fixing a bug on a time-sensitive project.

@nouhaag
Copy link

nouhaag commented Jul 26, 2023

The code works on Ubuntu in developer environment. Indeed, the dataset disappears from Data Explorer > Data menu, with all the plots linked to it. However, after clicking on the Delete button, the following error is raised several times in the terminal.

11:13:15 - sas.qtgui.Perspectives.Fitting.FittingWidget - ERROR - Traceback (most recent call last):
  File "/home/agouzal/sasview/sasview/src/sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py", line 279, in model_changed
    self.slider.extrapolation_parameters = self.extrapolation_parmameters
  File "/home/agouzal/sasview/sasview/src/sas/qtgui/Perspectives/Corfunc/CorfuncSlider.py", line 221, in extrapolation_parameters
    if self.find_parameter_problems(params) is None:
  File "/home/agouzal/sasview/sasview/src/sas/qtgui/Perspectives/Corfunc/CorfuncSlider.py", line 84, in find_parameter_problems
    if params.data_q_min >= params.point_1:
AttributeError: 'NoneType' object has no attribute 'data_q_min'

@rozyczko
Copy link
Member Author

Yes, this is a separate issue in the Corfunc, and it should be ticketed

@tsole0 tsole0 merged commit 46e9481 into main Jul 26, 2023
@rozyczko rozyczko deleted the 2527-deletting-a-dataset-is-not-possible-after-closing-a-plot branch July 29, 2023 21:27
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 31, 2023
@butlerpd butlerpd mentioned this pull request Aug 12, 2023
7 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deletting a dataset is not possible after closing a plot
4 participants