This is a list of all comments for UI-DEN-6349-1. Review Summary: No summary General Comment by plucia on 08 January 2021, 17:43 https://devapps.diality.us/cru/UI-DEN-6349-1#c7192 On 1.8.2021: Code Coverage: 100% SquishQt Tests: 100% passing General Comment by plucia on 08 January 2021, 18:16 https://devapps.diality.us/cru/UI-DEN-6349-1#c7194 Reviewers please note: According to the qt virtual keyboard technical styling guide, [here | https://doc.qt.io/qt-5.12/technical-guide.html] it is recommended to use {code} Src/qtvirtualkeyboard/src/virtualkeyboard/content/styles/default {code} as a base for the custom style. The Qt virtual keyboard styling files (called default and retro) I could find are located here on our VM: {code} /opt/Qt5.12.5/5.12.5/Src/qtvirtualkeyboard/src/virtualkeyboard/content/styles/default {code} You'll notice that the Qt company has only provided the button images in the svg format. It appears they did this to allow for greater customization of the keyboard styling in the qml code. I'm aware it has been decided to use pngs in the denali application. If requested, I can change the default Qt-provided keyboard style.qml file to use pngs instead of svgs. It would be helpful before I do so to get feedback on the look and feel of the numeric keyboard so we have an agreed-upon style, size, and layout of the numeric keyboard, as converting the svgs to pngs will hard-code the styling. General Comment by Behrouz NematiPour on 08 January 2021, 22:17 https://devapps.diality.us/cru/UI-DEN-6349-1#c7195 Why does it even need styling? Couldn't we use the keyboard default styling? Please provide two of the default and styled screenshot versions of the keyboard for comparison. Reply by plucia on 11 January 2021, 14:51 > You can't adjust the size of the keyboard, change what > buttons are available, or adjust the opaque black background > that's blocking half the screen without adjusting the > styling. > > In here you will find a couple of screenshots for comparison: > > Default numeric styling: > X:\Users\PeterL\ForBehrouz\Vitals_Entry_Default_Numeric_Keyboard.png > > Custom numeric styling: > X:\Users\PeterL\ForBehrouz\Vitals_Numeric_Keyboard.png > > Full Qwerty Keyboard with default styling: > X:\Users\PeterL\ForBehrouz\Full_Qwerty_Keyboard.png Reply by Behrouz NematiPour on 01 February 2021, 12:38 > RESOLVED General Comment by Behrouz NematiPour on 11 January 2021, 15:26 https://devapps.diality.us/cru/UI-DEN-6349-1#c7222 Thanks for providing the screenshots to compare. I would vote for the default style which covers half the screen since it is more standard and would be the same all over the application instead of having different keyboard combinations and locations in different situations and prefer to relocate screen components and labeling to fit in the screen. It can be like : {code} Vitals Entry ------------------ systolic [ ___ ] diastolic [ ___ ] BPM [ ___ ] {code} (or labels at top) and hide the keyboard on last entry BPM, 'enter' touch. Also, please on the user 'enter' touch go to the next entry. Reply by plucia on 11 January 2021, 15:35 > Okay, sure no problem. Do you want the full qwerty keyboard > then or numeric-only for this screen? Reply by Behrouz NematiPour on 01 February 2021, 12:37 > RESOLVED General Comment by Behrouz NematiPour on 11 January 2021, 15:48 https://devapps.diality.us/cru/UI-DEN-6349-1#c7224 Thanks, only a Numeric keyboard in this case. Please also take a look at the KDAB training about the QtQuick keyboard handling, in https://www.youtube.com/watch?v=ilCekEOpNYM @ 5:40 using the Keys API to be able to navigate on 'enter' pressed. that might be helpful. I think it should be "enterPressed", "returnPressed", or both. Please also take a look at the UX desing in the link : https://app.zeplin.io/project/5db0c175acfeac55e3cb879e/screen/5dbc8461bab7462c152cb34e Reply by plucia on 11 January 2021, 16:41 > Thanks, these links are helpful Reply by Behrouz NematiPour on 01 February 2021, 12:38 > RESOLVED ---------------------------------------- File: sources/gui/qml/dialogs/VitalsEntry.qml Revision Comment by Behrouz NematiPour on 08 January 2021, 22:27 https://devapps.diality.us/cru/UI-DEN-6349-1#c7199 Please don't use ";" until it's absolutely necessary, especially in qml codes. Reply by plucia on 13 January 2021, 10:00 > Semicolons have been removed Reply by Behrouz NematiPour on 27 January 2021, 10:42 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:31 https://devapps.diality.us/cru/UI-DEN-6349-1#c7200 why QtQuick.Controls imported? Reply by plucia on 12 January 2021, 14:56 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:42 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:26 https://devapps.diality.us/cru/UI-DEN-6349-1#c7198 please use "Item" instead of "Rectangle" with color as transparent when possible. Reply by plucia on 12 January 2021, 14:55 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:42 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:38 https://devapps.diality.us/cru/UI-DEN-6349-1#c7202 Where these values come from and please define them in the Variables file under the PRS section if applicable? Reply by plucia on 13 January 2021, 09:16 > I've added the min/max values for systolic, diastolic, and HR > to the Variables.qml file. They come from PRS 451, 452, and > 453 Reply by Behrouz NematiPour on 27 January 2021, 10:40 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:39 https://devapps.diality.us/cru/UI-DEN-6349-1#c7203 Please don't forget the translation for all the translatable strings. Reply by plucia on 13 January 2021, 09:13 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:41 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:40 https://devapps.diality.us/cru/UI-DEN-6349-1#c7204 same here. Reply by plucia on 13 January 2021, 09:43 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:41 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:40 https://devapps.diality.us/cru/UI-DEN-6349-1#c7206 same here. Reply by plucia on 13 January 2021, 09:43 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:41 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:40 https://devapps.diality.us/cru/UI-DEN-6349-1#c7207 same here. Reply by plucia on 13 January 2021, 09:44 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:41 > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 17:39 https://devapps.diality.us/cru/UI-DEN-6349-1#c7347 where the acceptableInput comes from the TextEntry component doesn't have such a property? Reply by plucia on 13 January 2021, 17:44 > acceptableInput is a property of the TextInput component. > The TextInput component in TextEntry is accessed via the > textInput alias Reply by Behrouz NematiPour on 18 January 2021, 11:28 > RESOLVED ---------------------------------------- File: sources/gui/qml/globals/Colors.qml Revision Comment by Behrouz NematiPour on 13 January 2021, 17:28 https://devapps.diality.us/cru/UI-DEN-6349-1#c7344 Why this is imported? Reply by plucia on 13 January 2021, 17:31 > This is your change actually. It looks like you added this in > 45ce6e78 > The alarm colors won't work without it Reply by Behrouz NematiPour on 27 January 2021, 10:38 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:47 https://devapps.diality.us/cru/UI-DEN-6349-1#c7208 Where the color has been picked from? If it is from UX design from zeplin, keep that in mind it wasn't a final design and has been put there to imply that the page requires a keyboard. what is the default color? Reply by plucia on 13 January 2021, 10:02 > When I removed the keyboard styling I removed this variable > as well Reply by Behrouz NematiPour on 13 January 2021, 17:29 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/FieldInput.qml Revision Comment by Behrouz NematiPour on 08 January 2021, 22:34 https://devapps.diality.us/cru/UI-DEN-6349-1#c7201 Please change the file name to TextEntry or TextEdit to make it more general (but not TextInput which conflicts with QtQuick). Field most of the time refers to a database field which is not the case all the time for this component. Reply by plucia on 12 January 2021, 14:53 > This file is now deleted Reply by Behrouz NematiPour on 12 January 2021, 16:22 > It still applied to the newly created file. > I would prefer to have something more general like: > - TextEntry > And please don't use a name like "Denali" which is subject > to change. Reply by plucia on 13 January 2021, 09:10 > It has been updated to TextEntry Reply by Behrouz NematiPour on 13 January 2021, 17:26 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:50 https://devapps.diality.us/cru/UI-DEN-6349-1#c7209 Please look at the [zeplin, 3.3 Name New Treatment Screen|https://app.zeplin.io/project/5db0c175acfeac55e3cb879e/screen/5dbc8461bab7462c152cb34e] for the text entry UX design. Reply by plucia on 12 January 2021, 14:53 > This file is now deleted Reply by Behrouz NematiPour on 12 January 2021, 16:21 > It still applied to the newly created file. Reply by plucia on 13 January 2021, 09:06 > Yes, the styling has been updated in the new file Reply by Behrouz NematiPour on 13 January 2021, 17:26 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:25 https://devapps.diality.us/cru/UI-DEN-6349-1#c7197 please use "Item" instead of "Rectangle" with color as transparent when possible. Reply by plucia on 13 January 2021, 09:11 > Done - see TextEntry.qml Reply by Behrouz NematiPour on 13 January 2021, 17:26 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:21 https://devapps.diality.us/cru/UI-DEN-6349-1#c7196 please whenever possible use the QtQuick versions instead of the QtQuick.controls. So in this case TextInput can be used instead of TextField. And then please remove the "import QtQuick.Controls 2.12" Reply by plucia on 13 January 2021, 09:12 > Done Reply by Behrouz NematiPour on 13 January 2021, 17:27 > RESOLVED ---------------------------------------- File: sources/gui/qml/main.qml Revision Comment by pmontazemi on 12 January 2021, 15:20 https://devapps.diality.us/cru/UI-DEN-6349-1#c7231 Space between functions. Reply by plucia on 13 January 2021, 10:11 > Done Reply by pmontazemi on 19 January 2021, 08:39 > RESOLVED. Revision Comment by pmontazemi on 12 January 2021, 15:21 https://devapps.diality.us/cru/UI-DEN-6349-1#c7232 Move comment outside of if statement. Reply by plucia on 13 January 2021, 10:03 > Done Reply by pmontazemi on 19 January 2021, 08:39 > RESOLVED. Revision Comment by Behrouz NematiPour on 13 January 2021, 17:21 https://devapps.diality.us/cru/UI-DEN-6349-1#c7339 This is my comment and it is specific to the exact line below the comment. Please revert it back to be able to identify which line in the code it refers to. *"This code review includes so many unrelated branched to the code review subject"* look at the list of the commits and branches included in this page http://dvm-linux02:8060/project/UI-DEN-6349?max=100&projectKey=UI-DEN-6349&view=fe Reply by plucia on 13 January 2021, 17:58 > I've moved the comment back so it's inside the if statement Reply by Behrouz NematiPour on 24 January 2021, 12:19 > RESOLVED ---------------------------------------- File: sources/view/VBluetooth.cpp Revision Comment by Behrouz NematiPour on 08 January 2021, 23:14 https://devapps.diality.us/cru/UI-DEN-6349-1#c7219 Very dangerous use of pointers please consider using none pointer variables. as an example why _lastSelectedDevice has been defined as a pointer? Please keep that in mind there is no guaranty that your code will reach the pointer release, and exceptions can happen anytime. Reply by plucia on 13 January 2021, 10:13 > The approach comes from Qt's documentation: It's the > QObjectList-based model approach > [https://doc.qt.io/qt-5/qtquick-modelviewsdata-cppmodels.html], > which you can see creates a list of QObject* pointers with > values that are accessible as named properties in the qml. > This class has been tested and is working fine > cppcheck doesn't report any errors in this file Reply by Behrouz NematiPour on 24 January 2021, 12:27 > This is not a prefered approach and there are better ways > of doing it. > As Qt mentions in the document: > {quote} > QAbstractItemModel Subclass > A model can be defined by subclassing QAbstractItemModel. > This is the *best approach* if you have a more complex > model that cannot be supported by the other approaches. A > QAbstractItemModel can also automatically notify a QML view > when the model data changes. > {quote} Reply by Behrouz NematiPour on 27 January 2021, 10:46 > will be done later. > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 18:33 https://devapps.diality.us/cru/UI-DEN-6349-1#c7363 Please use macros for the connections, whenever possible. Reply by plucia on 18 January 2021, 13:46 > Done Reply by Behrouz NematiPour on 18 January 2021, 17:12 > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 18:42 https://devapps.diality.us/cru/UI-DEN-6349-1#c7364 I checked out into the code and seems like the VBluetoothDeviceInfo class is not a view class although it has properties. So, why it has V in the naming, and why it has property? Views are only intended to be used in QML to interact with the C++ backend. Reply by plucia on 14 January 2021, 17:53 > Like the other view classes that have 'V' as a prefix, the > VBluetoothDeviceInfo has properties that are accessed > directly by the QML > It could be called BluetoothDeviceInfo instead? Or, what > would you prefer? Reply by Behrouz NematiPour on 15 January 2021, 11:38 > I checked out to the branch and I don't see the usage of > this class in any QML file. Reply by plucia on 18 January 2021, 14:02 > For VBluetoothDeviceInfo's usage in QML, see > SettingsBluetooth.qml lines 204, lines 212 > > {code} text: modelData.name {code} > and > {code} text: modelData.address {code} > > These properties are defined in VBluetoothDeviceInfo.h Reply by Behrouz NematiPour on 27 January 2021, 10:47 > will be done later. > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 18:07 https://devapps.diality.us/cru/UI-DEN-6349-1#c7351 How come a function has been added in the cpp but the header is not automatically added to the code review? Reply by plucia on 14 January 2021, 17:59 > I've added it now, looks like it was accidently omitted Reply by Behrouz NematiPour on 15 January 2021, 11:40 > The code review has to be automatic. > It has to be added to the code review by Crucible not the > developer as of our process. > Please create code reviews that are automatic and not > modified by the developer otherwise it would be out of > context and useless. Reply by plucia on 15 January 2021, 11:53 > Noted Reply by Behrouz NematiPour on 24 January 2021, 12:21 > *it has been done before please don't do this again* > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 18:09 https://devapps.diality.us/cru/UI-DEN-6349-1#c7352 the 'on' prefix shall be used for the private slots only. seems like this is not the case here. A better naming would be notifyStatusUpdate(). Reply by plucia on 14 January 2021, 17:47 > Done Reply by Behrouz NematiPour on 15 January 2021, 11:42 > RESOLVED Revision Comment by Behrouz NematiPour on 18 January 2021, 17:16 https://devapps.diality.us/cru/UI-DEN-6349-1#c7509 This is a leak/bug. Leak: A pointer to an object has been removed from the list and the object still exists and utilizing the memory (bug: and may even still doing some tasks). Reply by plucia on 18 January 2021, 17:34 > Fixed Reply by Behrouz NematiPour on 18 January 2021, 18:59 > Much better, that should work. > it's always a good practice to set the pointer to nullptr > after delete, but since it is a local pointer that is fine. > > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:55 https://devapps.diality.us/cru/UI-DEN-6349-1#c7210 please use nullptr instead of the NULL. Reply by plucia on 12 January 2021, 15:58 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:35 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 23:05 https://devapps.diality.us/cru/UI-DEN-6349-1#c7218 Why a simple name, address value set has been converted to a Jason Document by that complicated dereferencing and casting of pointers and passed over all the stacks of signal/slots instead of a simple struct? Reply by plucia on 12 January 2021, 16:02 > The json document holds the information about the BP cuffs > we've paired with. > It is also helpful to format the paired device information > into json so it can be stored to disk and then re-loaded > easily so that we can automatically re-connect to the last > selected BP cuff after a reboot. > The dereferencing and casting is needed to work with the > ObjectList-based model approach. > All of the slots are there to receive data from the > BLEScanner, so we can display available devices to connect > to. > We emit signals when we want to connect/reconnect to a > device, scan for devices, or request to save the paired > devices to disk. Reply by Behrouz NematiPour on 27 January 2021, 10:48 > needs to investigate more or done later. > RESOLVED ---------------------------------------- File: sources/view/VVitals.cpp Revision Comment by Behrouz NematiPour on 13 January 2021, 18:28 https://devapps.diality.us/cru/UI-DEN-6349-1#c7360 I remember I mentioned somewhere in the code to use the QObject internal timer and also mentioned MainTimer as an example. But I didn't mean to use the MainTimer. Please look at the MainTimer as an example of how to use the QObject internal timer and use it instead of the MainTimer itself. Reply by plucia on 18 January 2021, 14:01 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:37 > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 18:32 https://devapps.diality.us/cru/UI-DEN-6349-1#c7362 Please use macros for the connections, whenever possible. Reply by plucia on 18 January 2021, 13:51 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:36 > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 18:31 https://devapps.diality.us/cru/UI-DEN-6349-1#c7361 refer to the above comment about the MainTimer. Reply by plucia on 18 January 2021, 14:01 > Done Reply by Behrouz NematiPour on 27 January 2021, 10:37 > RESOLVED ---------------------------------------- File: sources/view/VVitals.h Revision Comment by Behrouz NematiPour on 08 January 2021, 23:01 https://devapps.diality.us/cru/UI-DEN-6349-1#c7216 please use #pragma once Reply by plucia on 12 January 2021, 15:51 > Done Reply by Behrouz NematiPour on 13 January 2021, 18:26 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 23:02 https://devapps.diality.us/cru/UI-DEN-6349-1#c7217 why 9999 used for default value instead of zero? Reply by plucia on 12 January 2021, 15:52 > Zero works too. I've updated it to zero Reply by Behrouz NematiPour on 13 January 2021, 18:26 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:57 https://devapps.diality.us/cru/UI-DEN-6349-1#c7211 please use the 'v' for the argument name beginning. Reply by plucia on 13 January 2021, 10:50 > Done Reply by Behrouz NematiPour on 13 January 2021, 18:25 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:58 https://devapps.diality.us/cru/UI-DEN-6349-1#c7212 same here for arguments name Reply by plucia on 12 January 2021, 15:56 > Done Reply by Behrouz NematiPour on 13 January 2021, 18:25 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 22:58 https://devapps.diality.us/cru/UI-DEN-6349-1#c7213 please a complete argument (const TYPE &vArgument) Reply by plucia on 12 January 2021, 15:56 > Done Reply by Behrouz NematiPour on 13 January 2021, 18:25 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 23:00 https://devapps.diality.us/cru/UI-DEN-6349-1#c7214 please use (const &) for all the slots down here. Reply by plucia on 12 January 2021, 15:56 > Done Reply by Behrouz NematiPour on 13 January 2021, 18:26 > RESOLVED Revision Comment by Behrouz NematiPour on 08 January 2021, 23:01 https://devapps.diality.us/cru/UI-DEN-6349-1#c7215 please move all the private member variables to the default private section of the class. Reply by plucia on 12 January 2021, 15:57 > Done Reply by Behrouz NematiPour on 13 January 2021, 18:26 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/DenaliTextInput.qml Revision Comment by Behrouz NematiPour on 12 January 2021, 16:18 https://devapps.diality.us/cru/UI-DEN-6349-1#c7254 please put the even verb at the end. pressedEnter => enterPressed eg. clickedBack => backClicked clickedContinue => continueClicked ***"please follow this everywhere in the code."*** Reply by plucia on 13 January 2021, 09:04 > Done, see TextEntry.qml Reply by Behrouz NematiPour on 18 January 2021, 11:39 > RESOLVED ---------------------------------------- File: sources/view/VBluetooth.h Revision Comment by Behrouz NematiPour on 24 January 2021, 12:34 https://devapps.diality.us/cru/UI-DEN-6349-1#c7575 Why the doGetDevices is a public slots, therefore, is publicly accessible while you have a property defined for that? That's one of the reasons why I insisted to use macros to avoid this kind of inconsistencies. Also, the name of the property/signal/slot does not match. if the property type is vTYPE and its name is vVARIABLE and the vSIGNAL is "Changed" then by PROPERTY macro definition it has to be like below with slot defined as private. {code} Q_PROPERTY( vTYPE vVARIABLE \ READ vVARIABLE \ WRITE vVARIABLE \ NOTIFY vVARIABLE##vSIGNAL) \ {code} The do/did/on prefixes will be used for none property matters. Reply by plucia on 27 January 2021, 10:47 > Resolved as per our code review 1.27.2021 Reply by Behrouz NematiPour on 27 January 2021, 10:49 > will be done later. > RESOLVED ---------------------------------------- File: .gitignore Revision Comment by Behrouz NematiPour on 24 January 2021, 12:44 https://devapps.diality.us/cru/UI-DEN-6349-1#c7576 what is this? why created and why ignored? Reply by plucia on 27 January 2021, 09:58 > You asked for this change: > {quote} > We need to have a folder next to the application for all the > settings since it is not related to the treatment it is only > settings configuration files. > Since the SD card might be damaged and we still need to store > settings. > Please change it to refer to the application location and > name the folder as configuration or settings. > {quote} > > Here's a screenshot: X:\Users\PeterL\ForBehrouz\settings.png Reply by Behrouz NematiPour on 27 January 2021, 10:23 > it may contain other settings so please don't ignore it in > .gitignore. Reply by plucia on 29 January 2021, 16:20 > I have updated .gitignore so only bledevices.conf is > ignored since it will be different for each device. Reply by Behrouz NematiPour on 01 February 2021, 12:36 > RESOLVED --- ID: UI-DEN-6349-1 https://devapps.diality.us/cru/UI-DEN-6349-1 Title: UI-DEN-6349_BP HR Manual Entry Statement of Objectives: State: Closed Summary: Author: plucia Reviewers: (0 active, 2 completed*) Behrouz NematiPour (*) pmontazemi (*)