Fix the crash in schedules view #614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmarrec
reviewed
Aug 16, 2023
src/openstudio_lib/SchedulesView.cpp
Outdated
| @@ -453,27 +453,29 @@ void SchedulesView::showDefaultScheduleDay(const model::ScheduleRuleset& schedul | |||
| void SchedulesView::showSummerScheduleDay(const model::ScheduleRuleset& schedule) { | |||
| setUpdatesEnabled(false); | |||
|
|
|||
| model::ScheduleRuleset scheduleRuleset = schedule; | |||
There was a problem hiding this comment.
I don't doubt that this works. But this looks fishy, taking something by const ref then making a copy.
Worst case we should take by value.
In any case we need a comment explaining why we do this.
More interestingly, why is that happening? Did you track it down?
There was a problem hiding this comment.
Tested that the fix works.
There was a problem hiding this comment.
It is fishy, you're right a comment will help. Here is what was happening:
- NewProfileView is created storing m_scheduleRuleset
- NewProfileView::onAddClicked emits a signal (e.g. addSummerProfileClicked) with a reference to m_scheduleRuleset
- SchedulesTabController slot (e.g. addSummerProfile) receives the m_scheduleRuleset reference and makes the new scheduleDay object
- SchedulesTabController slot tells the current SchedulesView to draw the new scheduleDay (e.g. showSummerScheduleDay)
- The SchedulesView deletes all the current widgets including NewProfileView which deletes its m_scheduleRuleset object, this invalidates the schedule reference passed to the function (e.g. showSummerScheduleDay)
The fix just creates a shallow copy of the schedule reference before deleting widgets. I updated it to make a const copy just to be a bit more correct. Another solution would be to call deleteLater on the widgets instead.
jmarrec
approved these changes
Aug 21, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #613 and addresses part of #617