Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new MVVM-based GTKmm sample app alongside the existing MVC example to demonstrate architectural differences and trade-offs.
Changes:
- Introduces an MVVM example (
mvvm_ap) with aSharedDatamodel,SharedDataVMViewModel, and two Views (EditorWidget,DisplayWidget). - Updates build configuration to compile the new MVVM executable.
- Updates documentation to include an MVC vs MVVM trade-offs table; also makes small MVC observer/include adjustments.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ap/mvvm/IObserver.h | Adds MVVM-local observer interface used by model/viewmodel/views. |
| src/ap/mvvm/model/SharedData.h | Defines MVVM model holding shared state and notifying observers. |
| src/ap/mvvm/model/SharedData.cpp | Implements MVVM model state updates + observer notification. |
| src/ap/mvvm/viewmodel/SharedDataVM.h | Defines ViewModel mediating model updates to views. |
| src/ap/mvvm/viewmodel/SharedDataVM.cpp | Implements ViewModel commands and forwarding notifications to views. |
| src/ap/mvvm/view/EditorWidget.h | Declares editor view that submits user input to the ViewModel. |
| src/ap/mvvm/view/EditorWidget.cpp | Implements editor UI and self-registration with the ViewModel. |
| src/ap/mvvm/view/DisplayWidget.h | Declares display view that renders ViewModel-provided state. |
| src/ap/mvvm/view/DisplayWidget.cpp | Implements display UI and updates on ViewModel notifications. |
| src/ap/mvvm/mvvm_ap.cpp | Wires up the MVVM app window, constructing model/viewmodel/views. |
| src/ap/mvvm/CMakeLists.txt | Builds and links the new mvvm_ap executable. |
| src/ap/CMakeLists.txt | Enables building the mvvm subdirectory. |
| src/ap/mvc/IObserver.h | Adds missing <string> include for std::string usage. |
| src/ap/mvc/model/SharedData.cpp | Adjusts observer iteration style in MVC model implementation. |
| src/ap/README.md | Replaces the old examples section with an MVC vs MVVM trade-offs table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dataModel_->addObserver(editorView_.get()); | ||
| dataModel_->addObserver(displayViewLeft_.get()); | ||
| dataModel_->addObserver(displayViewRight_.get()); | ||
|
|
There was a problem hiding this comment.
The MVVM container is still registering Views directly with the Model. Since SharedDataVM already subscribes to the Model and Views self-register with the ViewModel, these addObserver() calls will make each View receive duplicate notifications (from both Model and ViewModel) and undermines the MVVM separation described above. Remove these registrations so only the ViewModel observes the Model.
| dataModel_->addObserver(editorView_.get()); | |
| dataModel_->addObserver(displayViewLeft_.get()); | |
| dataModel_->addObserver(displayViewRight_.get()); |
| ContainerWindow::ContainerWindow() | ||
| : mainLayout_(Gtk::Orientation::VERTICAL), | ||
| topRowLayout_(Gtk::Orientation::HORIZONTAL) { | ||
| set_title("MVC Integrated Demo"); |
There was a problem hiding this comment.
This window title still says "MVC Integrated Demo" even though this is the MVVM example (and the app id is singlemvvm). Update the title to avoid confusing users/readers.
| set_title("MVC Integrated Demo"); | |
| set_title("MVVM Integrated Demo"); |
| * Key differences from MVC's ContainerWindow: | ||
| * 1. No Controller is created. | ||
| * 2. No manual addObserver() calls, each View self-registers with the ViewModel during construction. | ||
| * 3. Views receive a shared_ptr<SharedDataViewModel>, never a Model pointer. |
There was a problem hiding this comment.
The class comment says views receive a shared_ptr, but the actual type used throughout is SharedDataVM. Update the comment to match the real type name to prevent confusion in this example.
| * 3. Views receive a shared_ptr<SharedDataViewModel>, never a Model pointer. | |
| * 3. Views receive a shared_ptr<SharedDataVM>, never a Model pointer. |
| void DisplayWidget::updateLabel(const std::string& text) { | ||
| std::string markup = "<span foreground='" + color_ + | ||
| "' size='x-large' weight='bold'>" + text + "</span>"; | ||
| labelData_.set_markup(markup); |
There was a problem hiding this comment.
updateLabel() builds Pango markup by concatenating unescaped user-provided text. If the text contains markup characters (e.g. '<', '&'), the label will render incorrectly or fail to parse; it also allows markup injection. Escape the text (e.g., Glib::Markup::escape_text) before embedding it, or use set_text() and apply styling via CSS instead of markup.
No description provided.