Closed Huojuu closed 2 years ago
Changes look good, though there are couple of minor issues in the EfficiencyDialog
that could be fixed while we are at it:
EfficiencyDialog.delete
method is never called. There is no QDialog.delete
method that would get called automatically when the dialog is closed. This method is just a Potku thing. We should override the close event handler to ensure that the method is actually called when it is supposed to:
def closeEvent(self, event: QtGui.QCloseEvent) -> None:
self.delete() # remember to remove self.close() call from self.delete()!
super(EfficiencyDialog, self).closeEvent(event)
super().__init__()
instead of storing it as an instance variable. Passing it to the super class creates a proper parent - child relationship between the objects. When parent object is deleted, all of its children are deleted as well (check docs for more info on Object Trees). Storing the parent widget as an instance variable doesn't really do anything except maybe cause memory leaks.First change was changing EfficiencyDialog from QWidget to QDialog so that exec_ could be called
I do not remember what was the original reason to change Dialog
to Widget
in #213 .
Btw, temember to update the docstring too: """Efficiency widget dialog which is opened on top of detector settings."""
Second change was to fix the files being plotted. Potku was trying to plot files from the last folder the user used when they added new efficiency files but this might cause crashes and other problems. Now it tries to plot files from
/Default/Detector/Efficiency_files
I think there had been a logical / conceptual misunderstanding in the first place.
All in all, a good catch @Huojuu ! :) Did you get @jussiks's explanation? Do not hesitate to ask if you did not.
@Huojuu, is this ready to merge or are you looking to implement some/all of the suggestions?
@tpitkanen The suggestions that @jussiks brought up have not yet been implemented. Therefore I see two options:
I'm not sure how important the fixes are since I haven't yet looked into what they really mean so I don't know which option is better for the time being.
@tpitkanen The suggestions that @jussiks brought up have not yet been implemented. Therefore I see two options:
1. Merge this one as it is and implement the suggestions later. (Make an issue about them) 2. Do not merge and wait until the suggestions are implemented
I'm not sure how important the fixes are since I haven't yet looked into what they really mean so I don't know which option is better for the time being.
Those are minor performance things, so they can wait to be implemented later for all similar components.
As for the documentation, I went ahead and updated it as an example.
Edit: seems like PyCharm & GitHub CLI didn't push the commit to the right branch. I'll try to fix that.
@Huojuu
The issue was that I didn't have your repository in my git remotes. Now that added it as the origin, everything works as it should. Hope you don't mind me pushing commits to your repo.
I'd also recommend creating a new branch for each PR (on your own repo) instead of using Master directly. This way you can easily work on several PRs at the same time, if needed.
This PR fixes the issue #250 where efficiency plots weren't working by basically making two changes.
QWidget
toQDialog
so thatexec_
could be called<request>/Default/Detector/Efficiency_files