Refactor with architecture and developer documentation#865
Open
Refactor with architecture and developer documentation#865
Conversation
New library openstudio_qt_utils: move Application, OSProgressBar, QMetaTypes, Utilities out of model_editor; add OpenStudioQtUtilsAPI.hpp. shared_gui_components now has its own CMakeLists (openstudio_shared_gui): - Move IconLibrary, OSVectorController, UserSettings out of openstudio_lib/model_editor - Add OSItemId.hpp (extracted from OSItem.hpp) and OpenStudioSharedGuiAPI.hpp - Fix missing OSVectorController.hpp in qt6_wrap_cpp (caused LNK2001 on all Qt meta-object/signal symbols) - Register OSItemId metatypes in OSGridController Add BaseApp::mouseOverInspectorView() pure virtual; implement in OSAppBase; use it in OSLineEdit2::focusOutEvent instead of inline 3-level chain. All include paths updated; no circular shared_gui→openstudio_lib dependency.
…to add_documentation # Conflicts: # src/model_editor/ModelEditorAPI.hpp # src/openstudio_qt_utils/Application.hpp # src/openstudio_qt_utils/Utilities.hpp # src/shared_gui_components/IconLibrary.hpp # src/shared_gui_components/MeasureManager.hpp # src/utilities/CMakeLists.txt
macumber
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces a new openstudio_qt_utils library and a new openstudio_shared_gui library, refactors existing modules to depend on these shared utilities/components (moving many includes away from model_editor), and adds substantial developer/architecture documentation for the codebase and CI workflows.
Changes:
- Create new
openstudio_qt_utils+openstudio_shared_guiCMake targets and migrate shared Qt helpers and GUI components into them. - Refactor include paths and type ownership (eg
OSItemId,OSVectorController) acrossopenstudio_lib,openstudio_app,model_editor, andbimserver. - Add developer documentation describing libraries, classes, and CI workflows.
Reviewed changes
Copilot reviewed 203 out of 206 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utilities/OpenStudioApplicationUtilitiesAPI.hpp | Removed Windows export-macro header for utilities |
| src/utilities/OpenStudioApplicationPathHelpers.hpp | Removed export macro usage from utilities API declarations |
| src/utilities/CMakeLists.txt | Changed link interface to PUBLIC; removed NINJA define block |
| src/shared_gui_components/WorkflowController.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/UserSettings.hpp | Renamed include guards for shared module naming |
| src/shared_gui_components/UserSettings.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/ProcessEventsProgressBar.hpp | Updated OSProgressBar include to openstudio_qt_utils |
| src/shared_gui_components/ProcessEventsProgressBar.cpp | Updated Application include to openstudio_qt_utils |
| src/shared_gui_components/OSVectorController.hpp | Include guard rename; OSItemId include + forward decls |
| src/shared_gui_components/OSVectorController.cpp | New implementation of OSVectorController moved into shared module |
| src/shared_gui_components/OSUnsignedEdit.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/OSQuantityEdit.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/OSLoadNamePixmapLineEdit.cpp | Updated IconLibrary include path to shared component |
| src/shared_gui_components/OSLineEdit.cpp | Updated Utilities include path; simplified inspector mouse-over query |
| src/shared_gui_components/OSItemId.hpp | New shared value type extracted from OSItem.hpp |
| src/shared_gui_components/OSIntegerEdit.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/OSGridView.hpp | Switched include dependency to OSItemId |
| src/shared_gui_components/OSGridView.cpp | Updated Application include path to openstudio_qt_utils |
| src/shared_gui_components/OSGridController.hpp | Added OSItemId metatype declarations and new include ordering |
| src/shared_gui_components/OSGridController.cpp | Added OSItemId metatype registrations at startup |
| src/shared_gui_components/OSDoubleEdit.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/NetworkProxyDialog.cpp | Updated Application/Utilities include paths to openstudio_qt_utils |
| src/shared_gui_components/MeasureManager.hpp | Removed conditional OPENSTUDIO_API export decoration |
| src/shared_gui_components/MeasureManager.cpp | Updated includes (Application/Utilities/UserSettings) for new module layout |
| src/shared_gui_components/MeasureDragData.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/LocalLibraryController.cpp | Updated includes for relocated UserSettings + Utilities |
| src/shared_gui_components/IconLibrary.hpp | Renamed include guards; removed OPENSTUDIO_API export decoration |
| src/shared_gui_components/EditView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/EditController.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/Component.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/shared_gui_components/CMakeLists.txt | New CMake target openstudio_shared_gui with moc setup + deps |
| src/shared_gui_components/BuildingComponentDialogCentralWidget.cpp | Updated Application include path to openstudio_qt_utils |
| src/shared_gui_components/BaseApp.hpp | Updated QMetaTypes include; added mouseOverInspectorView() to interface |
| src/shared_gui_components/BCLMeasureDialog.cpp | Updated includes for relocated UserSettings + Utilities |
| src/openstudio_qt_utils/Utilities.hpp | Renamed include guards; removed MODELEDITOR_API; added Path/UUID includes |
| src/openstudio_qt_utils/Utilities.cpp | New implementation file for Qt<->std/UUID/path conversions |
| src/openstudio_qt_utils/QMetaTypes.hpp | Renamed include guards; removed OSItemId metatype declarations from here |
| src/openstudio_qt_utils/QMetaTypes.cpp | Removed OSItemId registrations from central metatype init |
| src/openstudio_qt_utils/OSProgressBar.hpp | Renamed include guards; removed dead/commented protected bits |
| src/openstudio_qt_utils/OSProgressBar.cpp | Removed dead/commented implementation fragments |
| src/openstudio_qt_utils/CMakeLists.txt | New CMake target openstudio_qt_utils and dependencies |
| src/openstudio_qt_utils/Application.hpp | Renamed include guards; removed MODELEDITOR_API include |
| src/openstudio_qt_utils/Application.cpp | Removed dead/commented Qt plugin-search-path block |
| src/openstudio_lib/test/SpacesSurfaces_Benchmark.cpp | Updated Application include path to openstudio_qt_utils |
| src/openstudio_lib/test/OpenStudioLibFixture.cpp | Updated Application include path to openstudio_qt_utils |
| src/openstudio_lib/test/IconLibrary_GTest.cpp | Updated IconLibrary include to shared component |
| src/openstudio_lib/ZoneChooserView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/YearSettingsWidget.hpp | Updated QMetaTypes include path to openstudio_qt_utils |
| src/openstudio_lib/YearSettingsWidget.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/VariablesTabView.hpp | Updated QMetaTypes include path to openstudio_qt_utils |
| src/openstudio_lib/VariablesTabView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/VRFController.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/UtilityBillFuelTypeListView.hpp | Updated OSVectorController include + QMetaTypes include path |
| src/openstudio_lib/UtilityBillFuelTypeListView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/SwimmingPoolIndoorFloorSurfaceDialog.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/StandardsInformationMaterialWidget.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/StandardsInformationConstructionWidget.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/StandardOpaqueMaterialInspectorView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/SpacesSubtabGridView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/SpaceTypesGridView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/SpaceLoadInstancesWidget.cpp | Updated includes for IconLibrary + OSVectorController to shared component |
| src/openstudio_lib/ServiceWaterScene.hpp | Updated QMetaTypes include path to openstudio_qt_utils |
| src/openstudio_lib/ServiceWaterGridItems.cpp | Updated IconLibrary include to shared component |
| src/openstudio_lib/ScriptItem.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/SchedulesView.hpp | Updated QMetaTypes include path to openstudio_qt_utils |
| src/openstudio_lib/SchedulesView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/ScheduleFileInspectorView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/ScheduleDialog.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/ScheduleDayView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/ScheduleCompactInspectorView.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/openstudio_lib/RunTabView.cpp | Updated Application/Utilities include paths; formatting tweak |
| src/openstudio_lib/ResultsTabView.hpp | Updated QMetaTypes include path to openstudio_qt_utils |
| src/openstudio_lib/ResultsTabView.cpp | Updated Utilities include path; comment wording tweak |
| src/openstudio_lib/RefrigerationController.hpp | Updated QMetaTypes include path to openstudio_qt_utils |
| src/openstudio_lib/RefrigerationController.cpp | Updated IconLibrary + Utilities includes |
| src/openstudio_lib/OpenStudioAPI.hpp | Removed Windows export-macro header for openstudio_lib |
| src/openstudio_lib/OSItemSelectorButtons.cpp | Updated OSVectorController include to shared component |
| src/openstudio_lib/OSItemList.cpp | Updated OSVectorController include to shared component |
| src/openstudio_lib/OSItem.hpp | Moved OSItemId type to shared header include |
| src/openstudio_lib/OSItem.cpp | Updated IconLibrary + Utilities includes |
| src/openstudio_lib/OSDropZone.cpp | Updated IconLibrary + OSVectorController includes |
| src/openstudio_lib/OSDocument.hpp | Removed OPENSTUDIO_API decoration; updated QMetaTypes include |
| src/openstudio_lib/OSDocument.cpp | Updated UserSettings + Utilities + Application includes for new modules |
| src/openstudio_lib/OSAppBase.hpp | Removed OPENSTUDIO_API; updated QMetaTypes include; added mouse-over method |
| src/openstudio_lib/OSAppBase.cpp | Added mouse-over implementation; updated includes + UserSettings path |
| src/openstudio_lib/ModelObjectVectorController.hpp | Updated OSVectorController include + QMetaTypes include |
| src/openstudio_lib/ModelObjectTreeWidget.hpp | Updated QMetaTypes include path |
| src/openstudio_lib/ModelObjectTreeItems.cpp | Updated IconLibrary + Utilities includes |
| src/openstudio_lib/ModelObjectListView.hpp | Updated OSVectorController include + QMetaTypes include |
| src/openstudio_lib/ModelObjectListView.cpp | Updated Utilities include path |
| src/openstudio_lib/ModelObjectItem.cpp | Updated Utilities include path |
| src/openstudio_lib/ModelObjectInspectorView.cpp | Updated Utilities include path |
| src/openstudio_lib/MainWindow.cpp | Updated Utilities include path |
| src/openstudio_lib/MainRightColumnController.hpp | Included OSItem.hpp; removed forward-decl mismatch |
| src/openstudio_lib/LoopScene.hpp | Updated QMetaTypes include path |
| src/openstudio_lib/LoopChooserView.cpp | Updated Utilities include path |
| src/openstudio_lib/LocationTabView.cpp | Updated Utilities include path |
| src/openstudio_lib/InspectorView.cpp | Updated Utilities include path |
| src/openstudio_lib/HVACSystemsController.hpp | Updated QMetaTypes include path |
| src/openstudio_lib/HVACSystemsController.cpp | Updated Utilities include path |
| src/openstudio_lib/GroundTemperatureView.cpp | Updated Utilities include path |
| src/openstudio_lib/GridViewSubTab.hpp | Included OSItem.hpp |
| src/openstudio_lib/GridItem.cpp | Updated IconLibrary + Utilities includes; minor formatting tweak |
| src/openstudio_lib/GeometryPreviewView.cpp | Updated Application include path |
| src/openstudio_lib/GeometryEditorView.cpp | Updated Utilities include path |
| src/openstudio_lib/BuildingInspectorView.cpp | Updated OSVectorController + Utilities includes |
| src/openstudio_lib/ApplyMeasureNowDialog.cpp | Updated Utilities include path |
| src/openstudio_lib/AnalyticsHelper.cpp | Updated Application + Utilities includes |
| src/openstudio_app/test/Resources_GTest.cpp | Updated Utilities include path |
| src/openstudio_app/test/OpenStudioAppFixture.cpp | Updated Application include path |
| src/openstudio_app/main.cpp | Removed COMPILING_FROM_OSAPP + OpenStudioAPI include; updated utils includes |
| src/openstudio_app/OpenStudioApp.cpp | Updated Utilities include path |
| src/openstudio_app/LibraryDialog.cpp | Updated Utilities include path |
| src/openstudio_app/ExternalToolsDialog.cpp | Updated Utilities include path; added Logger include |
| src/openstudio_app/CMakeLists.txt | Removed NINJA define; stopped compiling shared_gui sources directly; trimmed deps list |
| src/model_editor/test/Utilities_GTest.cpp | Updated Utilities include path to openstudio_qt_utils |
| src/model_editor/test/QMetaTypes_GTest.cpp | Updated QMetaTypes include path |
| src/model_editor/test/PathWatcher_GTest.cpp | Updated Application include path |
| src/model_editor/test/ModelEditorFixture.cpp | Updated Application include path |
| src/model_editor/test/GithubReleases_GTest.cpp | Updated Application include path |
| src/model_editor/TestButton.hpp | Removed MODELEDITOR_API decoration |
| src/model_editor/TableWidget.hpp | Removed MODELEDITOR_API decoration |
| src/model_editor/TableWidget.cpp | Added missing SDK includes |
| src/model_editor/PathWatcher.hpp | Removed MODELEDITOR_API decoration |
| src/model_editor/PathWatcher.cpp | Updated Application/Utilities include paths |
| src/model_editor/ModelEditorAPI.hpp | Removed Windows export-macro header for model_editor |
| src/model_editor/ModelEditor.i | Removed MODELEDITOR_API define; updated Utilities include path |
| src/model_editor/ModalDialogs.hpp | Removed MODELEDITOR_API decorations; updated declarations |
| src/model_editor/ModalDialogs.cpp | Updated Application/Utilities include paths |
| src/model_editor/InspectorGadget.hpp | Removed MODELEDITOR_API decorations |
| src/model_editor/InspectorGadget.cpp | Updated Utilities include path |
| src/model_editor/InspectorDialog.hpp | Removed MODELEDITOR_API decoration |
| src/model_editor/InspectorDialog.cpp | Updated Application/Utilities include paths |
| src/model_editor/IGSpinBoxes.hpp | Removed MODELEDITOR_API decorations |
| src/model_editor/IGLineEdit.hpp | Removed MODELEDITOR_API decoration |
| src/model_editor/GithubReleases.hpp | Removed MODELEDITOR_API decorations |
| src/model_editor/GithubReleases.cpp | Updated Application include path |
| src/model_editor/CMakeLists.txt | Removed moved sources; now depends on openstudio_qt_utils |
| src/model_editor/BridgeClasses.hpp | Removed MODELEDITOR_API decoration |
| src/model_editor/AccessPolicyStore.hpp | Renamed include guards; removed MODELEDITOR_API decorations |
| src/bimserver/ProjectImporter.hpp | Removed BIMSERVER_API decoration and include |
| src/bimserver/CMakeLists.txt | Removed BIMserverAPI header; switched deps to openstudio_qt_utils |
| src/bimserver/BIMserverConnection.hpp | Removed BIMSERVER_API decoration and include |
| src/bimserver/BIMserverConnection.cpp | Updated Application/Utilities include paths |
| src/bimserver/BIMserverAPI.hpp | Removed Windows export-macro header for bimserver |
| src/bimserver/BIMServer.i | Removed BIMSERVER_API define |
| developer/doc/libraries/utilities.md | Added documentation for utilities module (needs updates per review) |
| developer/doc/libraries/qtwinmigrate.md | Added documentation for qtwinmigrate (module not present in repo) |
| developer/doc/libraries/openstudio_app.md | Added documentation for openstudio_app module (dependency text needs updates) |
| developer/doc/libraries/model_editor.md | Added documentation for model_editor module |
| developer/doc/libraries/bimserver.md | Added documentation for bimserver module (dependency text needs updates) |
| developer/doc/classes/shared_gui_components/OSGridView.md | Added class documentation |
| developer/doc/classes/shared_gui_components/OSGridController.md | Added class documentation |
| developer/doc/classes/shared_gui_components/MeasureManager.md | Added class documentation |
| developer/doc/classes/shared_gui_components/LocalLibraryController.md | Added class documentation |
| developer/doc/classes/shared_gui_components/BuildingComponentDialog.md | Added class documentation |
| developer/doc/classes/shared_gui_components/BaseApp.md | Added class documentation |
| developer/doc/classes/shared_gui_components/BCLMeasureDialog.md | Added class documentation |
| developer/doc/classes/openstudio_lib/ThermalZonesController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/SpaceTypesController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/SchedulesController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/RunTabController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/ResultsTabController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/OSWebEnginePage.md | Added class documentation |
| developer/doc/classes/openstudio_lib/OSVectorController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/OSItem.md | Added class documentation |
| developer/doc/classes/openstudio_lib/OSDropZone.md | Added class documentation |
| developer/doc/classes/openstudio_lib/OSDocument.md | Added class documentation |
| developer/doc/classes/openstudio_lib/OSAppBase.md | Added class documentation |
| developer/doc/classes/openstudio_lib/ModelObjectListView.md | Added class documentation |
| developer/doc/classes/openstudio_lib/MaterialsController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/MainWindow.md | Added class documentation |
| developer/doc/classes/openstudio_lib/MainTabController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/MainRightColumnController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/InspectorController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/HVACSystemsView.md | Added class documentation |
| developer/doc/classes/openstudio_lib/HVACSystemsController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/GeometryEditorController.md | Added class documentation |
| developer/doc/classes/openstudio_lib/ConstructionsController.md | Added class documentation |
| developer/doc/classes/openstudio_app/StartupView.md | Added class documentation |
| developer/doc/classes/openstudio_app/OpenStudioApp.md | Added class documentation |
| developer/doc/classes/model_editor/InspectorGadget.md | Added class documentation |
| developer/doc/classes/model_editor/InspectorDialog.md | Added class documentation |
| developer/doc/classes/model_editor/AccessPolicyStore.md | Added class documentation |
| developer/doc/classes/bimserver/ProjectImporter.md | Added class documentation |
| developer/doc/classes/bimserver/BIMserverConnection.md | Added class documentation |
| developer/doc/ci/workflows/release_notes.md | Added CI workflow documentation |
| developer/doc/ci/workflows/manual_cli_test.md | Added CI workflow documentation |
| developer/doc/ci/workflows/export_standards_data.md | Added CI workflow documentation |
| developer/doc/ci/workflows/cppcheck.md | Added CI workflow documentation |
| developer/doc/ci/workflows/clangformat.md | Added CI workflow documentation |
| developer/doc/ci/workflows/cla.md | Added CI workflow documentation |
| developer/doc/ci/workflows/check_osm_versions.md | Added CI workflow documentation |
| developer/doc/ci/workflows/app_build.md | Added CI workflow documentation |
| developer/doc/ci/scripts.md | Added CI helper scripts documentation |
| developer/doc/ci/jenkins.md | Added Jenkins documentation |
| CMakeLists.txt | Added new subdirectories/targets (openstudio_qt_utils, openstudio_shared_gui) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 204 out of 207 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move class level documentation to comments in the classes
…cing openstudio_lib symbols Fix by extending the BaseApp interface (already in shared_gui_components) with the handful of methods that were being called through OSAppBase/OSDocument/MainWindow. OSAppBase overrides them as final, forwarding to the current document. Call sites in shared_gui_components now go through BaseApp::instance() (a dynamic_cast of qApp) instead of including openstudio_lib headers directly.
…d move OSItem, OSDropZone, and ModelObjectItem from openstudio_lib into that library, removing the upward dependency. Wire BaseApp::currentDocument() returning BaseDocument* through the hierarchy with a covariant OSDocument* override in OSAppBase/OpenStudioApp, replacing all shared_ptr<OSDocument> local variables and member fields with raw pointers.
Collaborator
Author
|
@jmarrec this is a crazy big PR but most of it is just due to moving files into a better dependency graph. I have looked through all the files, I'm not sure if you want to or not. If this is too big I could try to break it up somehow, possibly with https://github.github.com/gh-stack/. I signed up for a preview of that, will see if we get access. |
jmarrec
reviewed
Apr 30, 2026
Collaborator
jmarrec
left a comment
jmarrec
left a comment
There was a problem hiding this comment.
This is obviously hard to review given how big it is
before I delve deeper and waste more than a few hours, @macumber can you please explain what you're trying to solve, why this is better, and do a walkthrough review of your changes (don't need to annotate everything, just one of each major change types).
Comment on lines
+1
to
+18
| # Main build pipeline. Builds the OpenStudio Application on 5 platforms (ubuntu-22.04, | ||
| # ubuntu-24.04, windows-2022, macos-15-intel, macos-15 arm64), runs CTest and benchmarks, | ||
| # applies code signing on tag pushes, and uploads installers to GitHub Releases. | ||
| # | ||
| # Two jobs: | ||
| # build - matrix over all 5 platforms | ||
| # test_package_macos - downloads and verifies the signed macOS DMG (macos-15-intel + arm64) | ||
| # | ||
| # Caching: three independent caches keyed by OS + Conan profile + CACHE_KEY secret: | ||
| # 1. ccache (compiler output cache) | ||
| # 2. Conan package cache | ||
| # 3. Qt + OpenStudio SDK download cache | ||
| # Rotate the CACHE_KEY secret to bust all caches immediately. | ||
| # | ||
| # Required secrets: MACOS_DEVELOPER_ID_APPLICATION_CERTIFICATE_P12_BASE64, | ||
| # MACOS_DEVELOPER_ID_INSTALLER_CERTIFICATE_P12_PASSWORD, MACOS_KEYCHAIN_PASSWORD, | ||
| # NOTARIZATION_API_KEY, NOTARIZATION_API_TEAM_ID, NOTARIZATION_API_ISSUER_ID, | ||
| # SIGNPATH_CI_TOKEN, ANALYTICS_API_SECRET, ANALYTICS_MEASUREMENT_ID, CACHE_KEY |
Collaborator
There was a problem hiding this comment.
This is true right now, but will likely fall out of sync. Not sure what value it really provides. You can read the workflow to understand it.
Should the workflow fail for some reason, you'll definitely need to understand it anyways
Collaborator
Author
There was a problem hiding this comment.
I agree that manually maintaining this documentation would quickly become stale. However, my thought was by adding the instruction "CI script added or modified (ci/*.sh, ci/*.py, ci/*.qs) → header comment must accurately describe usage, arguments, and exit codes." in .github\instructions\pr-review.instructions.md that we could rely on the co-pilot reviews to ensure it stays up to date.
Part of the intent of this PR is to add documentation to guide llms in understanding the code and being able to make changes. So, my thought was we could add documentation for llms and then rely on llms to help keep it up to date. I'm open to removing it though.
Comment on lines
+31
to
+32
| add_library(${target_name} STATIC ${${target_name}_src} ${${target_name}_moc_src}) | ||
| target_link_libraries(${target_name} PUBLIC ${${target_name}_depends}) |
Collaborator
There was a problem hiding this comment.
I don't really get why. We've already building a static library
CMake does not define the BUILD_SHARED_LIBS variable by default, meaning without project or user intervention add_library() will produce STATIC libraries.
Collaborator
Author
There was a problem hiding this comment.
This change was just to make the static libs more explicit. As part of dead code cleanup, I got rid of the *API header files which were only there in case we wanted to build shared libs. I wanted to remove all of that unused code to make the project simpler and more maintainable.
Comment on lines
-37
to
-39
| if(BUILD_MICRO_PROFILING) | ||
| target_link_libraries(${target_name} ${MICRO_PROFILER_LIB} ) | ||
| endif() |
Collaborator
There was a problem hiding this comment.
I'm guessing now that you explicitly link stuff in a flat/serial manner you are only linking OpenStudioApp (or perhaps openstudio_lib) to the micro profiler?
Collaborator
Author
There was a problem hiding this comment.
Yeah, we only have it linked in at the lowest openstudioapp_utilities level, it is then available to everything else in the project
Comment on lines
+85
to
+86
| // TODO: promote to BaseDocument once MainWindow is extracted into an IMainWindow interface | ||
| // that lives in shared_gui_components or openstudio_qt_utils (no openstudio_lib dependency). |
Collaborator
There was a problem hiding this comment.
Does the TODO need to be addressed?
Collaborator
Author
There was a problem hiding this comment.
Updated to this clearer TODO, I figured this PR was too big that I should not add this in now:
// TODO: create a BaseMainWindow interface in shared_gui_components and change this
// to a virtual method BaseMainWindow* mainWindow() in BaseDocument.
// This will allow us to split up openstudio_lib into domain-specific libraries
// (e.g. constructions_gui, hvac_gui, etc.) which need to call mainWindow().
| bool modified() const override; | ||
| QString savePath() const override; | ||
| QString modelTempDir() const override; | ||
| model::Model componentLibrary() const override; |
Collaborator
There was a problem hiding this comment.
We lost the comments / docstrings here?
Comment on lines
-85
to
-86
| // Returns the component library associated with this document. | ||
| openstudio::model::Model componentLibrary() const; |
Comment on lines
+35
to
+41
| /** | ||
| * OSGridView is the QWidget that hosts and renders an OSGridController. It manages a QScrollArea | ||
| * containing a grid of OSCellWrapper widgets arranged in rows and columns. It handles row | ||
| * selection highlighting and forwards keyboard navigation events to the controller. Domain views | ||
| * (ThermalZonesGridView, SpaceTypesGridView, etc.) typically subclass OSGridView or compose it | ||
| * with filter/search bars. | ||
| */ |
Collaborator
There was a problem hiding this comment.
Ok, that's actually probably helpful
| @@ -213,94 +212,4 @@ void RenderingColorWidget2::renderColorButtonClicked() { | |||
| } | |||
| } | |||
|
|
|||
| RenderingColorWidget::RenderingColorWidget(QWidget* parent) : QWidget(parent) { | |||
Collaborator
There was a problem hiding this comment.
I guess the idea is that RenderingColorWidget2 is kept, and RenderingColorWidget was unused and deleted?
Collaborator
Author
There was a problem hiding this comment.
Yeah RenderingColorWidget was no longer used
Collaborator
Author
Thanks @jmarrec sorry this did get pretty big, I updated the PR description at the top to describe the intent of the changes |
Collaborator
Author
|
@jmarrec it looks like the conan.openstudio.net certificate expired on 4/29/26: Unable to connect to remote nrel-v2=https://conan.openstudio.net/artifactory/api/conan/conan-v2
|
|
@tijcolem said that he updated the cert. Let me know if it still has issues. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Intent
This PR started off with the goal to add architecture documentation for humans and llms to reference. Instructions were added to prompt llms to review documentation for accuracy and suggest updates in future changes. After reviewing the architecture, I felt compelled to clean it up. I probably should have waited to do that on subsequent PRs but I continued on here to:
Changes
Architecture / build system
Dead code removed
Developer documentation added
This is a large PR, most of the changes are file moves and small changes. The main goal was to clean up the dependency graph:
