### Denali UI Code Structure Review, Part 2 #### Peter Lucia plucia@diality.com Apr 6, 2020 #### Model View Controller While it may require more components, it will be more easily maintainable, so I think it's a good choice. - https://en.wikibooks.org/wiki/C%2B%2B_Programming/Code/Design_Patterns#Model-View-Controller_(MVC) "Newcomers will probably see this "MVC" model as wasteful, mainly because you are working with many extra objects at runtime, when it seems like one giant object will do. ***But the secret to the MVC pattern is not writing the code, but in maintaining it, and allowing people to modify the code without changing much else."*** From Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides - Design patterns: Elements of Reusable Object-Oriented Software - "MVC consists of three kinds of objects. The Model is the application object, the View is its screen presentation, and the Controller defines the way the user interface reacts to user input." - "The view should ensure that its appearance reflects the state of the model. Whenever the model's data changes, the model notifies views that depend on it. In response, each view gets an opportunity to update itself. This approach lets you attach multiple views to a model to provide different presentations. You can also create new views for a model without rewriting it. " From the UI Application SW Data Adjustment diagram, the GUI is pictured as sending a request to VGui to make a change when the user requests it. This request is then handled by cApp (Application Controller), then passed to the message dispatcher to send down to the firmware. There is a back and forth between the UI and Firmware, and then CMessage dispatcher updates the Model, which then automatically updates the View, which the diagram shows is binded to the QML screens. This process seems to align with the MVC design pattern. Questions: 1. What is the View block between the Model and GUI? 2. How decoupled are our views, controllers, and models are from one another? Could we easily attach new views to a model to provide new presentations? ![Architecture](UI-Diagram-Behrouz.png) It seems that in the context of MVC, our "view" really is both QML + C++ associated View classes that hand off data directly to the QML. For example, looking at SettingsHome.qml: The following model data ``` Repeater { model: [ vTreatmentDialysateFlow.dialysateFlow_FlowSetPoint .toFixed(2) , vTreatmentDialysateFlow.dialysateFlow_MeasuredFlow .toFixed(2) , vTreatmentDialysateFlow.dialysateFlow_RotorSpeed .toFixed(2) , vTreatmentDialysateFlow.dialysateFlow_MotorSpeed .toFixed(2) , vTreatmentDialysateFlow.dialysateFlow_MotorCtlSpeed .toFixed(2) , vTreatmentDialysateFlow.dialysateFlow_MotorCtlCurrent.toFixed(2) , "%" + vTreatmentDialysateFlow.dialysateFlow_PWMDutyCycle .toFixed(2) ] ``` is exposed by vtreatmentdialysateflow.h/.cpp: ``` class VTreatmentDialysateFlow : public QObject { Q_OBJECT PROPERTY( qint32 , dialysateFlow_FlowSetPoint , 0, false) PROPERTY( float , dialysateFlow_MeasuredFlow , 0, false) PROPERTY( float , dialysateFlow_RotorSpeed , 0, false) PROPERTY( float , dialysateFlow_MotorSpeed , 0, false) PROPERTY( float , dialysateFlow_MotorCtlSpeed , 0, false) PROPERTY( float , dialysateFlow_MotorCtlCurrent, 0, false) PROPERTY( float , dialysateFlow_PWMDutyCycle , 0, false) VIEW_DEC(VTreatmentDialysateFlow, DialysateFlowData) }; ``` and the properties are updated by the MDialysateFlow model's Data struct, found in mtreatmentflows.h: ``` class MDialysateFlow : public MFlow { public: struct Data { qint32 mFlowSetPoint = 0; /*!< vFlowSetPoint - Flow Set Point value of type signed int extracted out */ float mMeasuredFlow = 0; /*!< vMeasuredFlow - Measured Flow value of type float extracted out */ float mRotorSpeed = 0; /*!< vRotorSpeed - Rotor Speed value of type float extracted out */ float mMotorSpeed = 0; /*!< vMotorSpeed - Motor Speed value of type float extracted out */ float mMotorCtlSpeed = 0; /*!< vMotorCtlSpeed - Motor Controller Speed value of type float extracted out */ float mMotorCtlCurrent = 0; /*!< vMotorCtlCurrent - Motor Controller Current value of type float extracted out */ float mPWMDutyCycle = 0; /*!< vPWMDtCycle - PWM Duty Cycle in % value of type float extracted out */ }; ... typedef Model::MDialysateFlow::Data DialysateFlowData; ``` Are two cpp classes necessary to expose the model's data to QML? Could it be done with just one model class? I took a look at the macros which handle the signal slot mapping. Now, I can see how it's useful to dynamically add signals and slots with macros. Maybe it could save lines of code? Take this call in applicationcontroller.cpp for example: ``` ACTION_RECEIVE_MODEL_BRIDGE_CONNECTIONS(_MessageDispatcher) ``` Which calls the following macro in mmodel.h: ```batch #define ACTION_RECEIVE_MODEL_BRIDGE_CONNECTIONS(vSOURCE) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, BloodFlowData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, DialysateFlowData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, OutletFlowData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, TreatmentTimeData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, PressureOcclusionData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, AlarmStatusData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, PowerOffData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, TreatmentRangesData ) \ ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, AdjustBloodDialysateResponseData ) ``` Where ACTION_RECEIVE_BRIDGE_CONNECTION is defined in main.h. That just points to the macro ACTION_METHOD_BRIDGE_CONNECTION, hardcoding didActionReceive and onActionReceive as the signal and slot names. ``` #define ACTION_METHOD_BRIDGE_CONNECTION(vMETHOD, vSOURCE, vTYPE) \ connect(&vSOURCE, SIGNAL(did##vMETHOD(const vTYPE &)), \ this , SLOT( on##vMETHOD(const vTYPE &))); //--------------------------------------------------------------------------------// #define ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, vTYPE) \ ACTION_METHOD_BRIDGE_CONNECTION(ActionReceive, vSOURCE, vTYPE) ``` That's three macros to accomplish the following. ```batch connect(&_MessageDispatcher, SIGNAL(didActionReceive(const BloodFlowData&)), \ this , SLOT( onActionReceive(const BloodFlowData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const DialysateFlowData&)), \ this , SLOT( onActionReceive(const DialysateFlowData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const OutletFlowData&)), \ this , SLOT( onActionReceive(const OutletFlowData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const TreatmentTimeData&)), \ this , SLOT( onActionReceive(const TreatmentTimeData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const PressureOcclusionData&)), \ this , SLOT( onActionReceive(const PressureOcclusionData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const AlarmsStatusData&)), \ this , SLOT( onActionReceive(const AlarmsStatusData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const PowerOffData&)), \ this , SLOT( onActionReceive(const PowerOffData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const TreatmentRangesData&)), \ this , SLOT( onActionReceive(const TreatmentRangesData&))); connect(&_MessageDispatcher, SIGNAL(didActionReceive(const AdjustBloodDialysateResponseData&)), \ this , SLOT( onActionReceive(const AdjustBloodDialysateResponseData&))); ``` In my opinion the above macros hide the true intention of the code. The signals/slots makes it simpler and clearer what the intention is. 16 lines with macros vs. 18 lines with signals / slots. So, you're not really saving lines of code, but instead are just placing the mappings somewhere else instead of inside the class they affect. So, I think this could be simplified. The following macro, however, I think saves you lots of lines of code, and is particularly useful way of automating setup of Q_PROPERTIES. Nice! ```batch #define PROPERTY(vTYPE , vVARIABLE , vDEFVALUE, vALWAYSEMIT ) \ Q_PROPERTY(vTYPE vVARIABLE \ READ vVARIABLE \ WRITE vVARIABLE \ NOTIFY vVARIABLE##Changed) \ Q_SIGNALS: \ void vVARIABLE##Changed \ ( const vTYPE & v##vVARIABLE ); \ private: \ vTYPE _##vVARIABLE = vDEFVALUE; \ vTYPE vVARIABLE () const { \ return _##vVARIABLE ; \ } \ void vVARIABLE ( const vTYPE & v##vVARIABLE ) { \ static bool init = false; \ if ( vALWAYSEMIT \ || ! init \ || _##vVARIABLE != v##vVARIABLE ) { \ DEBUG_PROPERTY_CHANGED(vVARIABLE) \ init = true; \ _##vVARIABLE = v##vVARIABLE; \ emit vVARIABLE##Changed( _##vVARIABLE ); \ } \ } ``` Other topics: It seems that there was an intention to create a message model class that we haven't yet had time for. from messageglobals.h: ``` struct Message { // TODO : Should be converted to MessageModel class // no time left for now !!! Can_Id can_id; Sequence sequence = 0; // seq 0 is invalid Gui::GuiActionType actionId = Gui::GuiActionType::Unknown; ... ``` We might consider adding a controllers folder. Right now, the applicationcontroller is in the parent directory, while the gui controller is in the gui directory. Otherwise, the organization looks good: ```batch abstract/ applicationcontroller.cpp* applicationcontroller.h* applicationpost.cpp* applicationpost.h* canbus/ configuration/ gui/ main.h maintimer.cpp* maintimer.h* model/ storage/ threads.cpp threads.h utility/ view/ ``` QML: The QML code is well structured. The grouping makes sense. Lots of reusable components, which is great. I think using multiple groups of stacks - one for Manager, Treatment, and Settings should be okay, too. ``` components/ dialogs/ globals/ main.qml* pages/ ```