Conversation
…ng pngs to qrc, don't remove all old measures after updating measures
|
@jmarrec I'll test this some more before merging, let me know if you have any objections, it is in code near some of your changes. |
|
|
||
| std::vector<BCLComponent> OSDocument::getLocalComponents() const { | ||
| std::vector<BCLComponent> result; | ||
| if (m_haveLocalBCL) { |
There was a problem hiding this comment.
(I'm always thrown off by this ("Why would local bcl not be available") until I remember why we added it... )
| return result; | ||
| } | ||
|
|
||
| std::vector<BCLComponent> OSDocument::getLocalComponents() const { |
There was a problem hiding this comment.
I don't see why we need this.
| // remove old components | ||
| for (auto& oldComponent : OSAppBase::instance()->currentDocument()->getLocalComponents()) { | ||
| if ((oldComponent.uid() == component->uid()) && (oldComponent.versionId() != component->versionId())) { | ||
| LocalBCL::instance().removeComponent(oldComponent); | ||
| } |
There was a problem hiding this comment.
Why is looping on all components as opposed to using getLocalComponent(uid) better/needed?!
There was a problem hiding this comment.
Oh, I think I see. getComponent(uid) returns the first it finds
There was a problem hiding this comment.
I want to move this logic inside OSDocument where we do check if local bcl is available.
There was a problem hiding this comment.
Yeah, we should just add new methods to OSDocument similar to what you added in NatLabRockies/OpenStudio#5129. We can update those to call the new methods when they become available.
| // remove old measures | ||
| for (auto& oldMeasure : OSAppBase::instance()->currentDocument()->getLocalMeasures()) { | ||
| if ((oldMeasure.uid() == measure->uid()) && (oldMeasure.versionId() != measure->versionId())) { | ||
| LocalBCL::instance().removeMeasure(oldMeasure); | ||
| } |
| for (auto& oldMeasure : oldMeasures) { | ||
| LocalBCL::instance().removeMeasure(oldMeasure); | ||
| } |
There was a problem hiding this comment.
So that's the actual fix? There's similar code a dozen lines below though.
OpenStudioApplication/src/shared_gui_components/MeasureManager.cpp
Lines 1011 to 1013 in 8fc2a82
There was a problem hiding this comment.
the oldMeasures vector is not used anymore too.
Also, it seems you added that bit yourself in https://github.com/openstudiocoalition/OpenStudioApplication/pull/588/files#diff-46934f991308b4d8c13e26e0ec0f9530fcfecd8b0c79c80fda48eb29e8905a79R974 so is it really unnecessary?
There was a problem hiding this comment.
If remoteBCL.updateMeasures(); works correctly then it will remove the outdated measures. However, that wasn't working reliably due to the issue you fixed in NatLabRockies/OpenStudio#5129. My guess is that I was seeing a case where the old measures weren't removed so I added this code in. However, this code fails when the new measures aren't fetched for some reason, the old ones will just be cleared. I assumed that is what was happening in the bug report.
# Conflicts: # src/openstudio_lib/OSDocument.cpp # src/openstudio_lib/OSDocument.hpp # src/shared_gui_components/BuildingComponentDialogCentralWidget.cpp # src/shared_gui_components/MeasureManager.cpp
2 participants
Properly remove components and measures of the old version, add missing pngs to qrc, don't remove all old measures after updating measures
Fixes #696
Behavior will be further improved with NatLabRockies/OpenStudio#5127