This is a list of all comments for UI-DEN-5830-2. Review Summary: No summary ---------------------------------------- File: unittests/tst_messaging.h Revision Comment by plucia on 12 January 2021, 16:20 https://devapps.diality.us/cru/UI-DEN-5830-2#c7256 Will there be gaps in code coverage with these removed? Reply by Behrouz NematiPour on 12 January 2021, 16:32 > there were useless tests where the QVerify and QCompare were > removed from them after using the template function notify. Reply by plucia on 13 January 2021, 11:20 > RESOLVED ---------------------------------------- File: sources/canbus/MessageInterpreter.cpp Revision Comment by pmontazemi on 12 January 2021, 15:23 https://devapps.diality.us/cru/UI-DEN-5830-2#c7233 Add TODO on commented out line or delete commented line. This way, we can always find it. Reply by Behrouz NematiPour on 12 January 2021, 15:50 > I have different types of comments one of them is "// DEBUG:" > which we don't have a todo item for that. > It only exists for later or in casede bugging that I don't > want to log with that format all the time but sometimes > useful for debugging. Reply by pmontazemi on 15 January 2021, 14:36 > RESOLVED. Revision Comment by pmontazemi on 12 January 2021, 15:25 https://devapps.diality.us/cru/UI-DEN-5830-2#c7234 Where was this moved to? Or was it deleted? Reply by Behrouz NematiPour on 12 January 2021, 15:47 > By using the template function notify in lines 349,350,351 > they are not required anymore. > This shows how good/correct implementation helps to have a > clean code. Reply by pmontazemi on 15 January 2021, 14:36 > RESOLVED. ---------------------------------------- File: sources/model/MModel.h Revision Comment by plucia on 12 January 2021, 16:15 https://devapps.diality.us/cru/UI-DEN-5830-2#c7252 AlarmClearedConditionData is commented out above but this one remains? Reply by Behrouz NematiPour on 12 January 2021, 16:35 > I only wanted to exclude them from coco, but for some, I had > to completely comment them, but still kept the ones that > don't require to be commented. Reply by plucia on 13 January 2021, 11:21 > RESOLVED ---------------------------------------- File: sources/view/hd/alarm/VAlarmStatus.cpp Revision Comment by plucia on 12 January 2021, 16:17 https://devapps.diality.us/cru/UI-DEN-5830-2#c7253 Can be deleted Reply by Behrouz NematiPour on 12 January 2021, 16:33 > Kept it as a place holder to keep all 16 bits visibly used. Reply by plucia on 13 January 2021, 11:19 > RESOLVED ---------------------------------------- File: sources/gui/qml/dialogs/NotificationDialog.qml Revision Comment by plucia on 12 January 2021, 16:08 https://devapps.diality.us/cru/UI-DEN-5830-2#c7247 Shouldn't this be declared last? Reply by Behrouz NematiPour on 12 January 2021, 16:12 > Actually, No this is in the correct place and those two other > components shall cover the minimize mouseArea. > IF it goes last then it covers those two and they will never > work. Reply by plucia on 12 January 2021, 17:44 > RESOLVED ---------------------------------------- File: sources/model/hd/alarm/MAlarmStatusData.cpp Revision Comment by plucia on 12 January 2021, 16:13 https://devapps.diality.us/cru/UI-DEN-5830-2#c7250 Can be deleted Reply by Behrouz NematiPour on 12 January 2021, 17:58 > removed. Reply by plucia on 13 January 2021, 11:19 > RESOLVED ---------------------------------------- File: sources/gui/qml/dialogs/AlarmListDialog.qml Revision Comment by plucia on 12 January 2021, 16:07 https://devapps.diality.us/cru/UI-DEN-5830-2#c7246 Shouldn't this be declared last? Reply by Behrouz NematiPour on 12 January 2021, 16:13 > Same here. Reply by plucia on 12 January 2021, 17:44 > RESOLVED ---------------------------------------- File: tst_AlarmStatusData/test.py Revision Comment by plucia on 12 January 2021, 17:36 https://devapps.diality.us/cru/UI-DEN-5830-2#c7267 Can delete Reply by Behrouz NematiPour on 14 January 2021, 15:14 > I kept it for debugging. > The usual behavior of the SquishQt is to continue even if an > error happens to stop it if an error happens this value needs > to be set. > I'm still trying to figure out how exactly this works and > later may move it in a better file. Reply by plucia on 18 January 2021, 11:22 > RESOLVED ---------------------------------------- File: tst_ConfirmPrimingBegin/test.py Revision Comment by plucia on 12 January 2021, 17:33 https://devapps.diality.us/cru/UI-DEN-5830-2#c7264 Why were the extra sleeps needed? I don't have them and it's working fine Reply by Behrouz NematiPour on 14 January 2021, 15:27 > You actually had it with the value of 0.1 ms and I increased > it to 0.5 because after running the test multiple times I > figured it is not enough time. > I will review the waits in the pTx-UF COCO. Reply by pmontazemi on 19 January 2021, 08:35 > Our process calls for only 1 UNRESOLVED and 1 RESOLVED > response (comment) per thread please. Reply by plucia on 19 January 2021, 08:42 > Yes I know. I accidently clicked it and there's no way to > unclick Reply by Behrouz NematiPour on 19 January 2021, 11:33 > I will work on that later in prTx-UF story and it has > only been fixed here since it wasn't passing the test > sometimes while testing the Alarm design. Reply by plucia on 19 January 2021, 14:39 > RESOLVED ---------------------------------------- File: tst_CreateTreatment/test.py Revision Comment by plucia on 12 January 2021, 17:33 https://devapps.diality.us/cru/UI-DEN-5830-2#c7265 Can delete Reply by Behrouz NematiPour on 14 January 2021, 15:32 > deleted. Reply by plucia on 15 January 2021, 11:37 > RESOLVED ---------------------------------------- File: shared/scripts/names.py Revision Comment by plucia on 12 January 2021, 16:22 https://devapps.diality.us/cru/UI-DEN-5830-2#c7259 Can delete Reply by Behrouz NematiPour on 12 January 2021, 17:56 > deleted Reply by plucia on 13 January 2021, 11:20 > RESOLVED ---------------------------------------- File: tst_AlarmStatusData/verificationPoints/VP1 Revision Comment by plucia on 12 January 2021, 17:36 https://devapps.diality.us/cru/UI-DEN-5830-2#c7266 Was this added by mistake? Reply by Behrouz NematiPour on 14 January 2021, 15:31 > Good catch, > Dleted. Reply by plucia on 15 January 2021, 11:37 > RESOLVED ---------------------------------------- File: cr_objectives.sh Revision Comment by plucia on 12 January 2021, 16:21 https://devapps.diality.us/cru/UI-DEN-5830-2#c7257 Application is spelled wrong What is this for? Reply by Behrouz NematiPour on 12 January 2021, 17:57 > Thanks for catching that. Fixed. > This script is helping to have a summary of the objectives in > the code reviews and a summary of what has been done in this > branch. Reply by plucia on 13 January 2021, 11:20 > RESOLVED --- ID: UI-DEN-5830-2 https://devapps.diality.us/cru/UI-DEN-5830-2 Title: UI-DEN-5830_Alarm Design Statement of Objectives: Summary: ----- Applicarion ----- - Updated the old Alarm model structure to used the notify in the Message Interpreter. - Updated the Alarm Trigger, Clear, Status to inherit from MAbstract to be able to the above. - Added a macro to make the request messages handling easier. - Added 3 Alarm buttons - created Mute Button - created Up and Down Button (minimize/maximize) - used FW alarm status flag enum (Alarm_State_Flag_Bit_Positions) instead of the UI version and removed UI one. - modified and cleaned up Notification Dialog and Bar. - modified Time Text component to be able to show sections with one zero only. - cleaned up main.qml - Added User Action message response model. - Implementation of message 64: 'Alarm User Action' - Cleaned up the VAlarmStatus class. - increased the Notification Bar height a little bit more. - Fixed the bug of silence logic which was reverse - Fixed the bug that the length of the silence message payload was incorrectly set to 4 instead of 1. - changed the name of the MAlarmSilenceReq member variable to silence instead of the state which now implies it is silent (1) or not (0). - moved version and SD-Card icon to top which were covering the alarm bar. - added list icon on the notification bar - implemented the basics of the alarm list dialog. - tweaked the Notification default coloring - Added the message 63: 'alarm condition cleared, req/rsp' - Fixed the bug that alarm ID zero which is clear the alarm indication wasn't updating the properties, so caused to alarm_ID changed the notification signal not to raise and GUI wasn't getting updated. - Added a MACRO for convenience of empty payload messages handling. - Cleaned up the unit tests - Added a UI "AlarmGenerator" class to handle the HD_COMM_TIMEOUT and later the BLE Cuff alarm (PRS 395). - updated the alarm message table - commented as manually tested the AlarmClearedCondition. - added a unit test for the AlarmGenerator private method setBits. ----- TestSuit ----- - Completing the Alarm Status - Removing the Alarm Color which has been covered in the AlarmStatus in a bigger area. - Code Cleanup * Simulator: -- minor .ui changes -- Create a single object of the HD_Simulator and setting it in the DynamicLoader parent class of the plugins. -- The last piece of making the Simulator plugin base. -- Improvement: separator. -- Changed the RealTimeWidget parent name to DynamicLoader. -- Made DynamicLoader abstract class. -- Added Create Treatment plugin. -- Added the Alarm plugin -- modified the plugins to use a unique Loader class name and then define the unique name in the __init__.py file. -- using the os lib to get the current plugin path and load the unique interface.ui in the RealTmeWidget parent class. -- Added check CANBus in the simulator. ===== detailed by commits ===== ----- Application ----- commit: [f1e100|http://192.168.10.132:7990/projects/UI/repos/application/commits/f1e100d1368bfd132d88e09680f833dc53b4d0b0] - DEN-5833: Implementation - Updated the old Alarm model structure to used the notify in the Message Interpreter. - Updated the Alarm Trigger, Clear, Status to inherit from MAbstract to be able to the above. - Added a macro to make the request messages handling easier. commit: [45ce6e|http://192.168.10.132:7990/projects/UI/repos/application/commits/45ce6e781782be5de1480a1e7acecd1d272bcc84] - DEN-5833: Implementation - Added 3 Alarm buttons - created Mute Button - created Up and Down Button (minimize/maximize) - used FW alarm status flag enum (Alarm_State_Flag_Bit_Positions) instead of the UI version and removed UI one. - modified and cleaned up Notification Dialog and Bar. - modified Time Text component to be able to show sections with one zero only. - cleaned up main.qml commit: [e1eed3|http://192.168.10.132:7990/projects/UI/repos/application/commits/e1eed343ea59863ce6f04253b85f3a636786dab2] - DEN-5833: Implementation - Fixed a bouncing bug of the message when sent for the first time with a silenced flag on to show the alarm not maximized. commit: [eb23f8|http://192.168.10.132:7990/projects/UI/repos/application/commits/eb23f842f5b153622da276cdd167d98ea4d7ef04] - DEN-5833: Implementation - Added User Action message response model. commit: [d619b1|http://192.168.10.132:7990/projects/UI/repos/application/commits/d619b1355f03ab8e2e53956908bda66fea542224] - DEN-5833: Implementation - Implementation of the message 64: 'Alarm User Action' - Cleaned up the VAlarmStatus class. commit: [43cdee|http://192.168.10.132:7990/projects/UI/repos/application/commits/43cdee7bb03f7fa0bd5c2bba7f9d011bb06806b1] - DEN-5833: Implementation - made the mute button click optional and currently enabled. - made minimize on silent on the Notification Dialog optional and is disabled currently. - made separate slots for each user's actions instead of one for all with 4 parameters because it's easier and more convenient to use this way. - increased the Notification Bar height a little bit more. commit: [71784f|http://192.168.10.132:7990/projects/UI/repos/application/commits/71784f783e4e2c0eadebef28eefb0fc53534ba66] - DEN-5833: Implementation - Fixed the bug of silence logic which was reverse - Fixed the bug that the length of the silence message payload was incorrectly set to 4 instead of 1. - changed the name of the MAlarmSilenceReq member variable to silence instead of the state which now implies it is silent (1) or not (0). commit: [f325fa|http://192.168.10.132:7990/projects/UI/repos/application/commits/f325fa7bac94c7c0dcc9ed4185273e94669e17b7] - DEN-5833: Implementation - fix a naming bug. commit: [1d43ff|http://192.168.10.132:7990/projects/UI/repos/application/commits/1d43ff790e6a3fb03d371bb5bde9c2d432d299b6] - DEN-5833: Implementation - moved version and SD-Card icon to top which were covering the alarm bar. - added list icon on the notification bar - implemented the basics of the alarm list dialog. - tweaked the Notification default coloring commit: [3e5cdc|http://192.168.10.132:7990/projects/UI/repos/application/commits/3e5cdca5de3ac46619325bf1e37c572d729d5fa9] - DEN-5833: Implementation - Added the message 63: 'alarm condition cleared, req/rsp' commit: [0e001d|http://192.168.10.132:7990/projects/UI/repos/application/commits/0e001d4680d0f449e2d1d1a42870841293c6fbd5] - DEN-5833: Implementation - The message 63 has been tested commit: [5b17a1|http://192.168.10.132:7990/projects/UI/repos/application/commits/5b17a1d8d36a1fdae83391453aff79d41c1ee0c4] - DEN-5833: Implementation - Moved the version and sd-card icon to not cover others. commit: [9aedf|http://192.168.10.132:7990/projects/UI/repos/application/commits/9aedfea5361fddae22a8546f33a857b934a36303] - DEN-5833: Implementation - Fixed the bug that alarm ID zero which is clear the alarm indication wasn't updating the properties, so caused to alarm_ID changed the notification signal not to raise and GUI wasn't getting updated. commit: [51accb|http://192.168.10.132:7990/projects/UI/repos/application/commits/51accb0c8de0c3fd15b65c672dc348e99059597b] - DEN-5833: Implementation - Added a MACRO for convenience of empty payload messages handling. commit: [cd7694|http://192.168.10.132:7990/projects/UI/repos/application/commits/cd769413344091cea88a30861b49188c8c147cba] - DEN-5833: Implementation - added a touch expanding area for the TouchArea so regardless of its visual size it can have an expanded touchable area. - the touch expanding for the mute button set to 25 so it can have a bigger touch area than its size which is little. commit: [0d3114|http://192.168.10.132:7990/projects/UI/repos/application/commits/0d3114203575d6725576f8bdc8299ad772d55fd5] - DEN-5833: Implementation - Cleaned up the unit tests commit: [190921|http://192.168.10.132:7990/projects/UI/repos/application/commits/19092150c9343b729ad87be76805300417bfd68c] - DEN-5833: Implementation - Added a UI "AlarmGenerator" class to handle the HD_COMM_TIMEOUT and later the BLE Cuff alarm (PRS 395). commit: [b42479|http://192.168.10.132:7990/projects/UI/repos/application/commits/b42479c75add1af6281a6f7d8e6090117eaa55a9] - DEN-5836: Code Coverage - updated the alarm message table - fixed the -q parameter commit: [7ffe64|http://192.168.10.132:7990/projects/UI/repos/application/commits/7ffe641e90fd076e4358d56ef9aa400e9a9c1f75] - DEN-5836: Code Coverage - 99.110% (4790/4833) commit: [741b1c|http://192.168.10.132:7990/projects/UI/repos/application/commits/741b1c70f851810f2c265cdd38dfa158b7ee0c37] - DEN-5836: Code Coverage - commented as manually tested the AlarmClearedCondition. commit: [4d3220|http://192.168.10.132:7990/projects/UI/repos/application/commits/4d322070769e54c059b020f18215dca88c90acd7] - DEN-5836: Code Coverage - 99.979% (4826/4827) commit: [911882|http://192.168.10.132:7990/projects/UI/repos/application/commits/911882aaf10ee02ef646013914a2ddd6225224ca] - DEN-5836: Code Coverage - 100.000% (4827/4827) - added a unit test for the AlarmGenerator private method setBits. ----- TestSuit ----- commit : [8e2d609|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/8e2d609] - DEN-5836: Code Coverage - 99.979% (4826/4827) commit : [f29743f|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/f29743f] - DEN-5835: Integration Testing - removed the commented-out part of the test code for Priming. commit : [bb99af6|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/bb99af6] - DEN-5835: Integration Testing - Completing the Alarm Status - Removing the Alarm Color which has been covered in the AlarmStatus in a bigger area. commit : [ab2ebd8|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/ab2ebd8] - DEN-5836: Code Coverage - 99.297% (4799/4833) commit : [ef497ba|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/ef497ba] - DEN-5836: Code Coverage - Make the tests work with the application just to be able to start the testing. - Currently, the coverage is at 98.634% (4767/4833) commit : [6f1082f|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/6f1082f] - DEN-5833: Implementation - Added a Condition to test the message 63 Condition cleared - Code Cleanup commit : [d04cee8|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/d04cee8] - DEN-5833: Implementation - minor .ui changes commit : [f921975|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/f921975] - DEN-5754: Implementation - Create a single object of the HD_Simulator and setting it in the DynamicLoader parent class of the plugins. commit : [cbf0e77|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/cbf0e77] - DEN-5833: Implementation - reversed the alarm status flags bits order. commit : [15118be|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/15118be] - DEN-5754: Implementation - The last piece of making the Simulator plugin base. - Improvement: separator. commit : [9206e36|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/9206e36] - DEN-5754: Implementation - Changed the RealTimeWidget parent name to DynamicLoader. - Made DynamicLoader abstract class. - Added Create Treatment plugin. commit : [69b2aac|http://192.168.10.132:7990/projects/UI/repos/testsuites/commits/69b2aac] - DEN-5833: Implementation - Added the Alarm plugin - modified the plugins to use a unique Loader class name and then define the unique name int the __init__.py file. - using the os lib to get the current plugin path and load the unique interface.ui in the RealTmeWidget parent class. - Added check CANBus in the simulator. State: Closed Summary: Author: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)