This is a list of all comments for UI-DEN-3875-1. Review Summary: No summary ---------------------------------------- File: sources/canbus/frameinterface.cpp Revision Comment by pmontazemi on 12 August 2020, 14:42 https://devapps.diality.us/cru/UI-DEN-3875-1#c3305 Indent line one character to the left to align with upper line for increased legibility. Reply by Behrouz NematiPour on 20 August 2020, 16:46 > done Reply by pmontazemi on 20 August 2020, 21:21 > RESOLVED. ---------------------------------------- File: sources/canbus/messageinterpreter.cpp Revision Comment by pmontazemi on 12 August 2020, 14:38 https://devapps.diality.us/cru/UI-DEN-3875-1#c3294 Align arguments under each other to increase legibility. Reply by Behrouz NematiPour on 20 August 2020, 16:47 > done Reply by pmontazemi on 20 August 2020, 21:17 > RESOLVED. Revision Comment by plucia on 07 August 2020, 09:36 https://devapps.diality.us/cru/UI-DEN-3875-1#c2990 The code reaches column 189 here. From the C++ coding standard 11.1: "File content should be kept within 100 number of columns...." Reply by Behrouz NematiPour on 20 August 2020, 16:50 > Right, > The coding style changed. > With the current monitors everyone using nowadays it's waste > of space not to use extra available space. Reply by pmontazemi on 20 August 2020, 21:14 > Correct, our latest C++ Coding Standard Point 51. states: > "File content should be kept within 200 number of columns." Reply by plucia on 21 August 2020, 10:01 > RESOLVED. ---------------------------------------- File: sources/canbus/messagedispatcher.cpp Revision Comment by pmontazemi on 12 August 2020, 14:39 https://devapps.diality.us/cru/UI-DEN-3875-1#c3297 LOGGINF? Reply by Behrouz NematiPour on 20 August 2020, 16:47 > done Reply by pmontazemi on 20 August 2020, 21:18 > RESOLVED. Revision Comment by pmontazemi on 12 August 2020, 14:40 https://devapps.diality.us/cru/UI-DEN-3875-1#c3299 "unused" instead of "not used" Reply by Behrouz NematiPour on 20 August 2020, 16:47 > done Reply by pmontazemi on 20 August 2020, 21:18 > RESOLVED. ---------------------------------------- File: sources/gui/qml/dialogs/NotificationDialog.qml Revision Comment by pmontazemi on 12 August 2020, 14:28 https://devapps.diality.us/cru/UI-DEN-3875-1#c3280 Add empty line. Reply by Behrouz NematiPour on 20 August 2020, 16:48 > done Reply by pmontazemi on 20 August 2020, 21:15 > RESOLVED. Revision Comment by pmontazemi on 12 August 2020, 14:28 https://devapps.diality.us/cru/UI-DEN-3875-1#c3281 Always have one empty line at eof. Reply by Behrouz NematiPour on 20 August 2020, 16:49 > End of lines can't be not to be there.(which currently is > there) > QtCreator by defaults adds one if it has been removed. > It's there and Crucible is not showing it. Reply by pmontazemi on 20 August 2020, 21:15 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/SettingsHome.qml Revision Comment by plucia on 14 August 2020, 14:29 https://devapps.diality.us/cru/UI-DEN-3875-1#c3442 The percent sign should go after the number, not before it Reply by Behrouz NematiPour on 20 August 2020, 16:41 > Sure, but it's a debug code and percent at left makes it not > moving much and is readable. Reply by plucia on 21 August 2020, 10:02 > Ok, RESOLVED. Revision Comment by plucia on 14 August 2020, 14:30 https://devapps.diality.us/cru/UI-DEN-3875-1#c3443 The percent sign should go after the number, not before it Reply by Behrouz NematiPour on 20 August 2020, 16:41 > Sure, but it's a debug code and percent at left makes it not > moving much and is readable. Reply by plucia on 21 August 2020, 10:04 > Ok, RESOLVED. Revision Comment by plucia on 14 August 2020, 14:30 https://devapps.diality.us/cru/UI-DEN-3875-1#c3444 The percent sign should go after the number, not before it Reply by Behrouz NematiPour on 20 August 2020, 16:40 > Sure, but it's a debug code and percent at left makes it not > moving much and is readable. Reply by plucia on 21 August 2020, 10:03 > Ok, RESOLVED. ---------------------------------------- File: unittests/tst_messaging.cpp Revision Comment by plucia on 14 August 2020, 13:37 https://devapps.diality.us/cru/UI-DEN-3875-1#c3440 With QVERIFY commented out, this isn't testing anything Reply by Behrouz NematiPour on 20 August 2020, 16:42 > Sure, > I'll review it in the Saline Bolus story. > If I change it hear it may require so many codes to change. > Some of these get_<> test functions need to be removed. Reply by plucia on 21 August 2020, 10:04 > Ok, RESOLVED ---------------------------------------- File: sources/canbus/caninterface.cpp Revision Comment by pmontazemi on 12 August 2020, 14:42 https://devapps.diality.us/cru/UI-DEN-3875-1#c3306 Align arguments under each other for increased legibility. Reply by Behrouz NematiPour on 20 August 2020, 16:46 > done Reply by pmontazemi on 20 August 2020, 21:21 > RESOLVED. ---------------------------------------- File: sources/canbus/messagebuilder.cpp Revision Comment by pmontazemi on 12 August 2020, 14:41 https://devapps.diality.us/cru/UI-DEN-3875-1#c3303 Align arguments under each other for increased legibility. Reply by Behrouz NematiPour on 20 August 2020, 16:46 > done Reply by pmontazemi on 20 August 2020, 21:18 > RESOLVED. ---------------------------------------- File: sources/canbus/messagebuilder.h Revision Comment by pmontazemi on 12 August 2020, 14:40 https://devapps.diality.us/cru/UI-DEN-3875-1#c3301 Remove extra empty lines. Reply by Behrouz NematiPour on 20 August 2020, 16:47 > done Reply by pmontazemi on 20 August 2020, 21:18 > RESOLVED. ---------------------------------------- File: sources/canbus/messageglobals.h Revision Comment by pmontazemi on 20 August 2020, 21:27 https://devapps.diality.us/cru/UI-DEN-3875-1#c3669 Change sentence to: It is preferred to keep it as is so that the initialization is independent of data. Reply by Behrouz NematiPour on 20 August 2020, 21:39 > done Reply by pmontazemi on 20 August 2020, 21:41 > RESOLVED. Revision Comment by pmontazemi on 20 August 2020, 21:26 https://devapps.diality.us/cru/UI-DEN-3875-1#c3668 Change sentence to (so it becomes non-personal): It is preferred to keep it as is so that the initialization is independent of data. Reply by Behrouz NematiPour on 20 August 2020, 21:39 > done Reply by pmontazemi on 20 August 2020, 21:41 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/ManagerHome.qml Revision Comment by pmontazemi on 12 August 2020, 14:33 https://devapps.diality.us/cru/UI-DEN-3875-1#c3290 Remove all commented lines. Reply by Behrouz NematiPour on 20 August 2020, 16:48 > done Reply by pmontazemi on 20 August 2020, 21:17 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/TreatmentStack.qml Revision Comment by pmontazemi on 12 August 2020, 14:31 https://devapps.diality.us/cru/UI-DEN-3875-1#c3287 Create utility module (if not already done) with method fromMinToHrs(float min) that converts minutes to hours and call that from here with passed in argument vTreatmentTime.time_Total. Note: Since this is the JavaScript portion of QML, I leave it up to your best judgment where to define this method, local (only used in this *.qml file) vs. global (used across *.qml files). Reply by Behrouz NematiPour on 20 August 2020, 16:53 > This is the only and only place uses this value as hour and > not minute. > When more places use it I'll absolutely create a function. > Currently a comment that what is 60 will suffice. Reply by pmontazemi on 20 August 2020, 21:16 > RESOLVED. Revision Comment by pmontazemi on 12 August 2020, 14:33 https://devapps.diality.us/cru/UI-DEN-3875-1#c3289 }}? Reply by Behrouz NematiPour on 20 August 2020, 16:48 > done Reply by pmontazemi on 20 August 2020, 21:16 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/adjustments/TreatmentAdjustmentUltrafiltrationEdit.qml Revision Comment by pmontazemi on 12 August 2020, 14:29 https://devapps.diality.us/cru/UI-DEN-3875-1#c3283 If this is to be removed, add TODO comment with keyword "Remove when ...". Demo code should never remain in release code. Other option is to create ifndef/endif one case with Demo Build, other for no Demo Build. Reply by Behrouz NematiPour on 20 August 2020, 16:48 > done Reply by pmontazemi on 20 August 2020, 21:15 > RESOLVED. ---------------------------------------- File: sources/view/vtreatmentadjustmentultrafiltrationconfirm.cpp Revision Comment by pmontazemi on 12 August 2020, 14:48 https://devapps.diality.us/cru/UI-DEN-3875-1#c3311 Create utility class with fromMLtoL method (local or global) Reply by Behrouz NematiPour on 20 August 2020, 17:00 > The mL to L is only used in ultrafiltration and only for > volume and only for display on screen. > it wasn't worth including utility class here which costs more > code and build time only for a division/multiplication. > comment will suffice. Reply by pmontazemi on 20 August 2020, 21:22 > RESOLVED. Revision Comment by pmontazemi on 12 August 2020, 14:48 https://devapps.diality.us/cru/UI-DEN-3875-1#c3314 Create utility class with fromLtoML method (local or global) Reply by Behrouz NematiPour on 20 August 2020, 16:59 > The mL to L is only used in ultrafiltration and only for > volume and only for display on screen. > it wasn't worth including utility class here which costs more > code and build time only for a division/multiplication. > comment will suffice. Reply by pmontazemi on 20 August 2020, 21:22 > RESOLVED. Revision Comment by pmontazemi on 12 August 2020, 14:49 https://devapps.diality.us/cru/UI-DEN-3875-1#c3315 Create utility class with fromLtoML method (local or global) Reply by Behrouz NematiPour on 20 August 2020, 16:54 > The mL to L is only used in ultrafiltration and only for > volume and only for display on screen. > it wasn't worth including utility class here which costs more > code and build time only for a division/multiplication. > comment will suffice. Reply by pmontazemi on 20 August 2020, 21:23 > RESOLVED. ---------------------------------------- File: sources/view/vtreatmentultrafiltration.cpp Revision Comment by pmontazemi on 12 August 2020, 14:49 https://devapps.diality.us/cru/UI-DEN-3875-1#c3317 Create utility class with fromMLtoL method (local or global) Reply by Behrouz NematiPour on 20 August 2020, 17:01 > The mL to L is only used in ultrafiltration and only for > volume and only for display on screen. > it wasn't worth including utility class here which costs more > code and build time only for a division/multiplication. > comment will suffice. Reply by pmontazemi on 20 August 2020, 21:23 > RESOLVED. Revision Comment by pmontazemi on 12 August 2020, 14:49 https://devapps.diality.us/cru/UI-DEN-3875-1#c3318 Create utility class with fromMLtoL method (local or global) Reply by Behrouz NematiPour on 20 August 2020, 17:01 > The mL to L is only used in ultrafiltration and only for > volume and only for display on screen. > it wasn't worth including utility class here which costs more > code and build time only for a division/multiplication. > comment will suffice. Reply by pmontazemi on 20 August 2020, 21:23 > RESOLVED. ---------------------------------------- File: sources/view/vtreatmentultrafiltration.h Revision Comment by pmontazemi on 12 August 2020, 14:50 https://devapps.diality.us/cru/UI-DEN-3875-1#c3319 1) I would make these hard requirements (0.600, 0.500, etc. constants defined part of a critical data file and called here. This way, we have them all in one place. 2) Shouldn't the minimum be 100 mL (0.100 L)? Reply by Behrouz NematiPour on 20 August 2020, 16:45 > Absolutely, > As // TODO says it has to be part of the pre-treatment flow > getting the data from prescription parameters. Reply by pmontazemi on 20 August 2020, 21:24 > RESOLVED. ---------------------------------------- File: unittests/tst_utilities.cpp Revision Comment by pmontazemi on 12 August 2020, 14:53 https://devapps.diality.us/cru/UI-DEN-3875-1#c3320 Remove extra line. Reply by Behrouz NematiPour on 20 August 2020, 16:44 > done Reply by pmontazemi on 20 August 2020, 21:24 > RESOLVED. ---------------------------------------- File: unittests/tst_utilities.h Revision Comment by pmontazemi on 12 August 2020, 14:54 https://devapps.diality.us/cru/UI-DEN-3875-1#c3321 Remove extra line. Reply by Behrouz NematiPour on 20 August 2020, 16:44 > done Reply by pmontazemi on 20 August 2020, 21:25 > RESOLVED. --- ID: UI-DEN-3875-1 https://devapps.diality.us/cru/UI-DEN-3875-1 Title: UI-DEN-3875_Logging_2of3 Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Moderator: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)