-
Notifications
You must be signed in to change notification settings - Fork 42
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
Delete intermediate theory plots by tab id not model id. #3160
Conversation
I think that this looks sensible. DataItem is connected to a fitting tab, with a unique ID, so all dependent plots should be checked against that unique ID. |
@@ -2077,8 +2077,8 @@ def deleteIntermediateTheoryPlotsByModelID(self, model_id): | |||
return | |||
match = GuiUtils.theory_plot_ID_pattern.match(data.id) | |||
if match: | |||
item_model_id = match.groups()[-1] | |||
if item_model_id == model_id: | |||
item_tab_id = match.groups()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you compare the first, not last match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an example I just ran, match.groups()
returns:
('1', None, None, 'sphere')
And the first item seems to be the tab ID.
While confirming this, I just spotted a mistake since the signal is still set to emit a string when its actually now given an integer. I'm going to fix this now but I will probably convert the tab ID to a string before passing it to the event instead of converting the first match into an integer because if, for whatever reason, the first match is not a parsable number then it could error.
I think this should be against release_6.0.1 rather than main. Interestingly, checking 5.0.6 hitting return at the appropriate juncture immediately throws the traceback error but the parameter panel never goes blank? |
Yes I think you're right. I had it on main as that was the default when Github creates a PR from an issue. I'll try to change it.
Hmm interesting. I was going to try this for myself on IDAaaS but there seems to be some downtime at the moment but I'll have a look when thats back. |
881b037
to
3e888fd
Compare
Looks like the Mac build failed but by the looks of it, it is just a CI issue. The error is:
|
This is actually the behaviour I get on Linux, and I just tested 5.0.6, and got the same result (albeit on Linux again). My guess is that it depends when the parameters are redrawn, and at one point the theory item is accessed. |
Description
I've tried to fix #3159 by changing the
deleteIntermediateTheoryPlotsByModelID
method so that it deletes by tab id, and not model id. This was causing the bug as we had 2 tabs with the same model, and it was trying to delete theory items from the other tab.While this fixes the problem, I'm not entirely sure how suitable this fix is as I'm not aware of why this method was needed in the first place, and thus it could potentially break some other functionality. It would probably be useful to discuss this fix at the call.
How Has This Been Tested?
Followed the reproduction instructions Paul provided in #3159. I am no longer able to reproduce the bug in this pull request.
Review Checklist:
Documentation (check at least one)
Installers
Licencing (untick if necessary)