This is a list of all comments for UI-DEN-4964-1. Review Summary: No summary General Comment by Behrouz NematiPour on 29 October 2020, 14:08 https://devapps.diality.us/cru/UI-DEN-4964-1#c5766 When this code review started the Doxygenization wasn't done. So assumed that this code could be included in master and covered as part of the Doxygenization. Now that the Doxygenization is done, all the classes shall have the documentation with messaging information. Please add to all your classes. For example on how to do please refer to the Model/View classes. And when doc generated it will also be used for SDD. it should contain: - \brief - \details - Message information - Message Payload information as each field in a line (see example below) - \sa(see also) which has related classes to this class - Logging info which at least has 3 lines (see example below and follow the HTML syntax for formating in doxygen please.) An example provided. please look at more examples for your specific case in the code on the master branch. {code} /*! * \brief The MAdjustSalineResponse class * \details The Saline Bolus adjustment response model * * | MSG | CAN ID | M.Box | Type | Ack | Src | Dest | Description | * |:---:|:------:|:-----:|:----:|:---:|:---:|:----:|:---------------------:| * | 20 | 0x020 | 6 | Rsp | Y | HD | UI | Saline Bolus Response | * * | Payload || * | || * | #1:(U32) | \ref Data::mAccepted | * | #2:(U32) | \ref Data::mReason | * | #3:(U32) | \ref Data::mTarget | * * \sa Data * \sa MAdjustSalineReq : Saline Bolus Request * \sa MTreatmentSaline : Saline Bolus Data * *

Logging info

* | || * | || * | typeText | Event | * | unitText | HD | * | infoText | AdjustSaline | * */ {code} Reply by pmontazemi on 02 November 2020, 15:10 > Updated DG/HD accelerometer data class headers, and will > apply this header concept for DG/HD version data moving > forward. Reply by Behrouz NematiPour on 16 December 2020, 22:52 > Please do the same for the Versions. Reply by pmontazemi on 29 December 2020, 15:16 > Done. Reply by Behrouz NematiPour on 03 January 2021, 22:40 > RESOLVED ---------------------------------------- File: sources/model/hd/data/MHDAccelerometerData.cpp Revision Comment by Sean Nash on 13 October 2020, 10:12 https://devapps.diality.us/cru/UI-DEN-4964-1#c5446 Need to add function header comments. Reply by pmontazemi on 14 October 2020, 09:23 > BehrouzN has addressed headers for all Data on Master Branch > as part of the doxygenization effort. Reply by Sean Nash on 20 October 2020, 14:12 > RESOLVED. ---------------------------------------- File: sources/model/hd/data/MHDAccelerometerData.h Revision Comment by plucia on 13 October 2020, 10:07 https://devapps.diality.us/cru/UI-DEN-4964-1#c5443 The m prefix is not informative / needed Reply by pmontazemi on 14 October 2020, 09:26 > Definitions: > m is used for model data (structure) variables > > Moving forward, my expectation is for all of us to follow the > same naming convention. I have also requested BehrouzN to > update the C++ Coding Standards. Reply by plucia on 15 October 2020, 12:56 > RESOLVED. Revision Comment by Behrouz NematiPour on 29 October 2020, 14:55 https://devapps.diality.us/cru/UI-DEN-4964-1#c5774 Pleae align semicolons. Reply by pmontazemi on 02 November 2020, 11:57 > Addressed. Reply by Behrouz NematiPour on 16 December 2020, 21:44 > RESOLVED Revision Comment by Behrouz NematiPour on 16 December 2020, 22:04 https://devapps.diality.us/cru/UI-DEN-4964-1#c6836 This is a broadcast message and should be Datum as you have mentioned in the function typeText() method correctly. Reply by pmontazemi on 29 December 2020, 15:43 > Done. Reply by pmontazemi on 03 January 2021, 22:01 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:14 > RESOLVED Revision Comment by Behrouz NematiPour on 03 January 2021, 22:14 https://devapps.diality.us/cru/UI-DEN-4964-1#c7130 Please change it to "HDAccelData" as you have stated in the infoText function. this should be exactly the text user may search for in the log file to find the logged message. Reply by pmontazemi on 03 January 2021, 22:23 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:30 > RESOLVED ---------------------------------------- File: sources/view/hd/data/VHDAccelerometerData.cpp Revision Comment by plucia on 13 October 2020, 10:19 https://devapps.diality.us/cru/UI-DEN-4964-1#c5450 This obfuscates what's going on under the hood. Code clarity is king Reply by pmontazemi on 14 October 2020, 09:12 > Based on my review of the QObject classes underneath, the > whole intent of the VIEW_DEF and VIEW_DEC macros is to > provide a much more simplistic interfaces to the user when > wanting to create views and their corresponding data. We also > made the decision to not touch these two macros. Hence, my > expectation is for all features implemented moving forward to > follow this design pattern. > > Note: If someone wants to look at the definition of these > macros, they can always go back to their corresponding *.h. Reply by plucia on 14 October 2020, 09:28 > Does VIEW_DEF support bidirectional messaging, e.g. UI to > HD as well as HD to UI? Reply by plucia on 20 October 2020, 11:46 > RESOLVED > > After looking into this, VIEW_DEF does not support > bidirectional messaging but it should be okay in this > case since you are just reading data from FW. Revision Comment by plucia on 13 October 2020, 10:08 https://devapps.diality.us/cru/UI-DEN-4964-1#c5445 The v prefix is not informative / needed From the C++ coding standard: "4. Functions and variables should have descriptive and meaningful names or well-commented declarations; variables with a small scope can have short names." Reply by pmontazemi on 14 October 2020, 09:40 > Definitions: > _data is used for private data member > vData is used for method argument > Data is used for data type > > Moving forward, my expectation is for all of us to follow the > same naming convention. I have also requested BehrouzN to > update the C++ Coding Standards. Reply by plucia on 14 October 2020, 09:49 > "1. Any violation to the guide is allowed if it enhances > readability. The justification must be provided in the > source code, where the violation occurs. In addition, > approval for the violation must be granted by Software > Engineering Management, Software Engineering, and Quality." > > Changing parameter names to enhance readability is > encouraged by our coding standard, so much that any > violation to enhance readability (with justification) is > allowed. > > vData and "data" is not descriptive or meaningful, so its > usage violates "4. Functions and variables should have > descriptive and meaningful names..." Reply by plucia on 20 October 2020, 11:48 > RESOLVED ---------------------------------------- File: sources/gui/qml/pages/ManagerHome.qml Revision Comment by Sean Nash on 13 October 2020, 10:07 https://devapps.diality.us/cru/UI-DEN-4964-1#c5444 Since RefUFVol and MeasUFVol are in liters, can we show these with 3 decimal places so we can infer mL? Reply by pmontazemi on 14 October 2020, 09:25 > Addressed. Reply by Sean Nash on 15 October 2020, 13:25 > RESOLVED. Revision Comment by plucia on 13 October 2020, 10:22 https://devapps.diality.us/cru/UI-DEN-4964-1#c5452 When you run the code, the component could be positioned better. It should be moved left or up so there's no gap and so it's better aligned with either Tmprtr or Prsr Oc. Reply by pmontazemi on 14 October 2020, 09:04 > Left is reserved for another HD item already and top is > reserved for DG data. Regardless, once all the visual > elements of acceleration and versioning are added to the UI, > we will be going through a real estate optimization exercise > to ensure best positioning given the limited amount of space > in one screen. Reply by plucia on 14 October 2020, 09:24 > RESOLVED Revision Comment by Sean Nash on 13 October 2020, 10:05 https://devapps.diality.us/cru/UI-DEN-4964-1#c5441 If .toFixed(2) indicates that we want to display 2 decimal places, these first 6 values are in gravities - likely between -1 and +1 - and so I'd like to see these with 3 decimal places. Reply by pmontazemi on 14 October 2020, 09:28 > Addressed. Reply by Sean Nash on 16 October 2020, 09:53 > RESOLVED. ---------------------------------------- File: sources/view/hd/data/VHDAccelerometerData.h Revision Comment by Sean Nash on 13 October 2020, 10:16 https://devapps.diality.us/cru/UI-DEN-4964-1#c5447 I'm seeing these '{" on new line like this sometimes and at end of line above in some cases. Does our C++ coding standard call for one or the other or does it allow either? Reply by pmontazemi on 13 October 2020, 11:22 > Our C++ coding standard calls for the first "{" to be opened > in the same line as the class/function definition. I will > change. > > Addressed. I have also asked BehrouzN to run Qt Beautifier to > make all of these consistent throughout the UI Application > code base. Reply by Sean Nash on 16 October 2020, 09:53 > RESOLVED. Revision Comment by plucia on 13 October 2020, 10:19 https://devapps.diality.us/cru/UI-DEN-4964-1#c5449 This obfuscates what's going on under the hood. Code clarity is king Reply by pmontazemi on 14 October 2020, 09:22 > Based on my review of the QObject classes underneath, the > whole intent of the VIEW_DEF and VIEW_DEC macros is to > provide a much more simplistic interfaces to the user when > wanting to create views and their corresponding data. We also > made the decision to not touch these two macros. Hence, my > expectation is for all features implemented moving forward to > follow this design pattern. > > Note: If someone wants to look at the definition of these > macros, they can always go back to their corresponding *.h. Reply by plucia on 14 October 2020, 09:33 > No need for a duplicate conversation about this. RESOLVED. ---------------------------------------- File: denali.pro Revision Comment by Behrouz NematiPour on 12 January 2021, 18:08 https://devapps.diality.us/cru/UI-DEN-4964-1#c7274 Please remove if not used. Reply by pmontazemi on 02 February 2021, 21:45 > Reverted change. Reply by Behrouz NematiPour on 02 February 2021, 22:09 > RESOLVED Revision Comment by plucia on 13 October 2020, 10:06 https://devapps.diality.us/cru/UI-DEN-4964-1#c5442 This should go under Models - HD - Data Reply by pmontazemi on 14 October 2020, 09:31 > Will do. > > Let's all agree to do it this way moving forward. I like the > arrangement of files in their corresponding buckets, > denali.pro looks much cleaner this way. Reply by plucia on 30 October 2020, 09:51 > RESOLVED Revision Comment by plucia on 13 October 2020, 10:04 https://devapps.diality.us/cru/UI-DEN-4964-1#c5439 This should go under Views - HD - Data Reply by pmontazemi on 14 October 2020, 09:32 > Will do. > > Let's all agree to do it this way moving forward. I like the > arrangement of files in their corresponding buckets, > denali.pro looks much cleaner this way. Reply by plucia on 30 October 2020, 09:51 > RESOLVED Revision Comment by plucia on 13 October 2020, 10:03 https://devapps.diality.us/cru/UI-DEN-4964-1#c5438 This should go under Models - HD - Data Reply by pmontazemi on 14 October 2020, 09:34 > Will do. > > Let's all agree to do it this way moving forward. I like the > arrangement of files in their corresponding buckets, > denali.pro looks much cleaner this way. Reply by plucia on 30 October 2020, 09:52 > RESOLVED Revision Comment by plucia on 13 October 2020, 10:05 https://devapps.diality.us/cru/UI-DEN-4964-1#c5440 This should go under Views - HD - Data Reply by pmontazemi on 14 October 2020, 09:33 > Will do. > > Let's all agree to do it this way moving forward. I like the > arrangement of files in their corresponding buckets, > denali.pro looks much cleaner this way. Reply by plucia on 30 October 2020, 09:52 > RESOLVED ---------------------------------------- File: sources/canbus/messageglobals.h Revision Comment by Behrouz NematiPour on 16 December 2020, 22:07 https://devapps.diality.us/cru/UI-DEN-4964-1#c6837 Since added one more parameter ( one F32, 4 bytes) in the payload of each messages the length should reflect the exact amount of the bytes and it needs to be changed to 8 * 4 for both of the : Gui::GuiActionType::ID_BloodFlow , Gui::GuiActionType::ID_DialysateInletFlow otherwise ui will read only 7*4 bytes and you will get zero for the last parameter (signal strength). Reply by pmontazemi on 29 December 2020, 15:45 > Done. Reply by Behrouz NematiPour on 03 January 2021, 19:46 > RESOLVED Revision Comment by Behrouz NematiPour on 16 December 2020, 22:13 https://devapps.diality.us/cru/UI-DEN-4964-1#c6838 this message has zero length by definition. Reply by pmontazemi on 29 December 2020, 15:46 > Done. Reply by Behrouz NematiPour on 03 January 2021, 19:47 > RESOLVED ---------------------------------------- File: sources/canbus/messageinterpreter.cpp Revision Comment by Behrouz NematiPour on 16 December 2020, 22:18 https://devapps.diality.us/cru/UI-DEN-4964-1#c6839 These two lines are not correct/required since this message doesn't have a payload at all. Just keep the LOG_EVENT Reply by pmontazemi on 29 December 2020, 15:48 > Done. Reply by Behrouz NematiPour on 03 January 2021, 19:58 > please remove the if statement (L249) as well which checks > the length of the vData, as well. Reply by pmontazemi on 03 January 2021, 22:00 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:16 > RESOLVED ---------------------------------------- File: sources/model/dg/data/MDGAccelerometerData.h Revision Comment by Behrouz NematiPour on 16 December 2020, 21:56 https://devapps.diality.us/cru/UI-DEN-4964-1#c6833 This is a broadcast message and should be Datum as you have correctly mentioned in the function typeText() method. Reply by pmontazemi on 29 December 2020, 15:28 > Changed Event to Datum for both DG and HD *.h files. Reply by pmontazemi on 03 January 2021, 21:58 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:31 > RESOLVED Revision Comment by Behrouz NematiPour on 03 January 2021, 20:05 https://devapps.diality.us/cru/UI-DEN-4964-1#c7121 Please change it to "DGAccelData" as you have stated in the infoText function. this should be exactly the text user may search for in the log file to find the logged message. Reply by pmontazemi on 03 January 2021, 21:56 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:47 > incorrect comment line has been changed. > please revert back "Datum" and change the infoText section. Reply by pmontazemi on 05 January 2021, 10:57 > Addressed. Reply by Behrouz NematiPour on 05 January 2021, 11:21 > RESOLVED ---------------------------------------- File: sources/view/dg/data/VDGAccelerometerData.h Revision Comment by Behrouz NematiPour on 29 October 2020, 14:21 https://devapps.diality.us/cru/UI-DEN-4964-1#c5772 please document the class with the format like the example below: \sa (see also) should refer to the mode data which is used in this class in your case Model::DGAccelerometerData {code} /*! * \brief The VDGDrainPump class * \details View for Model's Data representation. * * \sa Model::MDGDrainPump * */ {code} Please refer to more examples in the master branch and do the same for other Model/View classes. As a tip just above your class name type "/*!" and hit enter to let QtCreator complete the scaffold for you. Reply by pmontazemi on 02 November 2020, 13:22 > Addressed in both VDGAccelerometerData.h and > VHDAccelerometerData.h. Reply by pmontazemi on 29 December 2020, 15:25 > Also addressed in VTreatmentAdjustDG/HDVersions.h. Reply by Behrouz NematiPour on 03 January 2021, 22:37 > RESOLVED ---------------------------------------- File: sources/model/hd/adjustment/MTreatmentAdjustHDVersionsResponse.h Revision Comment by Behrouz NematiPour on 16 December 2020, 22:03 https://devapps.diality.us/cru/UI-DEN-4964-1#c6835 Please add the message definition this method implements on top of the class like what you did for MDGAccelerometerData.h Reply by pmontazemi on 29 December 2020, 15:42 > Done. Reply by pmontazemi on 03 January 2021, 22:03 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:13 > RESOLVED ---------------------------------------- File: sources/model/hd/adjustment/MTreatmentAdjustRequests.h Revision Comment by Behrouz NematiPour on 16 December 2020, 22:00 https://devapps.diality.us/cru/UI-DEN-4964-1#c6834 Please add the same messaging documentation for your new message container models should be like below (only for your new message, the others are already added): {code} /*! * \brief The MAdjustDurationReq class * \details The treatment duration change request model * * | MSG | CAN ID | Box | Type | Ack | Src | Dst | Description | * |:----:|:------:|:---:|:------:|:---:|:---:|:---:|:-----------: | * |0x1600| 0x100 | 9 | Req | Y | UI | HD | Treatment Duration Change Request | * * | Payload || * | || * | #1:(U32) | \ref duration | * */ {code} Reply by pmontazemi on 29 December 2020, 15:42 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:34 > RESOLVED ---------------------------------------- File: sources/model/dg/adjustment/MTreatmentAdjustDGVersionsResponse.h Revision Comment by Behrouz NematiPour on 16 December 2020, 21:55 https://devapps.diality.us/cru/UI-DEN-4964-1#c6832 Please add the message definition this method implements on top of the class like what you did for MDGAccelerometerData.h Reply by pmontazemi on 29 December 2020, 15:26 > Done. Reply by Behrouz NematiPour on 03 January 2021, 20:02 > RESOLVED Revision Comment by Behrouz NematiPour on 03 January 2021, 20:03 https://devapps.diality.us/cru/UI-DEN-4964-1#c7120 Please change it to DGVersions as you have stated in the infoText function. this should be exactly the text user may search for in the log file to find the logged message. Reply by pmontazemi on 03 January 2021, 21:59 > Addressed. Reply by Behrouz NematiPour on 03 January 2021, 22:25 > RESOLVED ---------------------------------------- File: sources/view/dg/adjustment/VTreatmentAdjustmentDGVersions.h Revision Comment by Behrouz NematiPour on 16 December 2020, 22:55 https://devapps.diality.us/cru/UI-DEN-4964-1#c6841 Since we have only one version request per messages definition we should not have the same request in both the VTreatmentAdjustmentDGVersions and VTreatmentAdjustmentHDVersions I will give my suggestion but feel free to ask me if it's not clear and we need to discuss. My suggestion is to merge these two views in one view by: 1 - rename all the version response messages properties to have HD,DG in them. 2 - merge them all in the VTreatmentAdjustmentHDVersions 3 - make the VTreatmentAdjustmentHDVersions to listen to the DG version response message as well by adding the line below in the VTreatmentAdjustmentHDVersions initConnection method. {code} ACTION_VIEW_CONNECTION(AdjustDGVersionsResponseData); {code} Note : please reorder the lines in initConnection to have first the HD and then DG and also have the request first and then responses, now that we have more that two category of messages. 4 - remove the HD from the name of the VTreatmentAdjustmentHDVersions to be VTreatmentAdjustmentVersions . 5 - remove the two VTreatmentAdjustmentDGVersions (.h , .cpp) files and all it's references in the View.h and Globals. Reply by pmontazemi on 05 January 2021, 18:54 > Addressed. Reply by Behrouz NematiPour on 05 January 2021, 18:55 > RESOLVED --- ID: UI-DEN-4964-1 https://devapps.diality.us/cru/UI-DEN-4964-1 Title: UI-DEN-4964_UI Messaging Version Accelometer Statement of Objectives: State: Closed Summary: Author: pmontazemi Reviewers: (0 active, 3 completed*) Sean Nash (*) plucia (*) Behrouz NematiPour (*)