This is a list of all comments for UI-DEN-2087-1. Review Summary: No summary ---------------------------------------- File: sources/gui/qml/components/TouchRect.qml Revision Comment by pmontazemi on 16 April 2020, 07:50 https://devapps.diality.us/cru/UI-DEN-2087-1#c1609 Interesting that the signal gets defined as signal clicked above but here it gets used as .clicked() as a function call Reply by Behrouz NematiPour on 16 April 2020, 10:45 > Yes, > This is the Qt way. > Signal is a function but Qt by #define and Qt Meta Data > classes will handle it in its own way. > By calling a signal in QML we are emitting it so the other > interested connected class can have a slot to be connected to > that signal and implement the behavior. Reply by pmontazemi on 22 April 2020, 15:16 > RESOLVED. ---------------------------------------- File: sources/canbus/messageinterpreter.cpp Revision Comment by pmontazemi on 16 April 2020, 07:37 https://devapps.diality.us/cru/UI-DEN-2087-1#c1602 Remove extra line Reply by Behrouz NematiPour on 22 April 2020, 15:24 > Done Reply by pmontazemi on 22 April 2020, 15:33 > RESOLVED. Revision Comment by pmontazemi on 16 April 2020, 07:38 https://devapps.diality.us/cru/UI-DEN-2087-1#c1603 Remove extra line Reply by Behrouz NematiPour on 22 April 2020, 15:24 > Done Reply by pmontazemi on 22 April 2020, 15:33 > RESOLVED. Revision Comment by pmontazemi on 16 April 2020, 07:39 https://devapps.diality.us/cru/UI-DEN-2087-1#c1604 I understand return is the output; however, what I more used to see is: input(s), output(s), and params for every function header. Reply by Behrouz NematiPour on 16 April 2020, 10:41 > It is the standard header. > It has standard > \params for function parameters and > \retrurn for return values. > So it has all you want, regarding output and input. > All standard cpp headers are like this. Reply by pmontazemi on 22 April 2020, 15:17 > RESOLVED. Revision Comment by pmontazemi on 16 April 2020, 07:40 https://devapps.diality.us/cru/UI-DEN-2087-1#c1605 Add function header with inputs, outputs, and params Reply by Behrouz NematiPour on 22 April 2020, 15:25 > Done Reply by pmontazemi on 22 April 2020, 15:34 > RESOLVED. Revision Comment by pmontazemi on 16 April 2020, 07:41 https://devapps.diality.us/cru/UI-DEN-2087-1#c1606 Add function header with inputs, outputs, and params Also, is this a set function? If so, please call it setPressureOcclusionData otherwise its current name makes the reviewer guess what this function does. Reply by Behrouz NematiPour on 22 April 2020, 14:47 > Regarding the function name: > - It's not the set function. > - It extracts the data out of message. > > In general I don't use set/get if not required and use the > function overloading for that purpose which make is like a > property that has a single name every where regardless of set > or get. Reply by pmontazemi on 22 April 2020, 15:14 > 1. Still need to address the missing header. > > 2. Understood, I have used function overloading in the > past. Reply by Behrouz NematiPour on 22 April 2020, 15:25 > Done Reply by pmontazemi on 22 April 2020, 15:34 > RESOLVED. Revision Comment by pmontazemi on 16 April 2020, 07:41 https://devapps.diality.us/cru/UI-DEN-2087-1#c1607 Add function header with inputs, outputs, and params Reply by Behrouz NematiPour on 22 April 2020, 15:30 > Done Reply by pmontazemi on 22 April 2020, 15:34 > RESOLVED. Revision Comment by pmontazemi on 16 April 2020, 07:42 https://devapps.diality.us/cru/UI-DEN-2087-1#c1608 Add function header with inputs, outputs, and params Reply by Behrouz NematiPour on 22 April 2020, 15:24 > Done Reply by pmontazemi on 22 April 2020, 15:35 > RESOLVED. ---------------------------------------- File: sources/gui/guiview.h Revision Comment by pmontazemi on 16 April 2020, 07:57 https://devapps.diality.us/cru/UI-DEN-2087-1#c1610 Remove extra line. Reply by Behrouz NematiPour on 22 April 2020, 15:30 > Done Reply by pmontazemi on 22 April 2020, 15:35 > RESOLVED. ---------------------------------------- File: sources/utility/format.cpp Revision Comment by pmontazemi on 16 April 2020, 08:01 https://devapps.diality.us/cru/UI-DEN-2087-1#c1612 Where is format.h? Reply by Behrouz NematiPour on 16 April 2020, 10:49 > both cpp and header are next to each other. > FishEye is adding what ever file that has been changed. Reply by pmontazemi on 22 April 2020, 15:18 > RESOLVED. Revision Comment by plucia on 22 April 2020, 13:29 https://devapps.diality.us/cru/UI-DEN-2087-1#c1626 Is the static_cast here safe? What about using dynamic_cast? Reply by Behrouz NematiPour on 22 April 2020, 14:04 > static_cast will be evaluated at compile time and if there is > any issue casting it will not even build. > dynamic_cast will do it at run time which is hard to handle > that exception, I would only use it when I need to cast an > interface of a .so file load at run-time or so. > > I think static_cast is much more safer than dynamic > (specially in our case.) Reply by plucia on 23 April 2020, 09:12 > Don't we not know the true type of vData even at runtime? > So, what is the advantage of static_cast being used if it > only checks the type at compile time? > With dynamic_cast doesn't it return null if it doesn't > contain the type casted, so you just add an if statement > checking whether it is null or not to know for a fact that > you're cast was successful Reply by Behrouz NematiPour on 23 April 2020, 16:18 > I don't know why I should use dynamic_cast and have an if > around the cast. What is the point? > What do I gain by doing so? > I'm sure checking everything in compile time is much more > safer than in run-time waiting for the code to be > executed and then handle the exception. > What is the advantage of the dynamic_cast over the > static_cast. they do the same thing if can successfully > doing it in different situation. Reply by plucia on 23 April 2020, 16:42 > RESOLVED Reply by Behrouz NematiPour on 23 April 2020, 18:21 > Please resolve the comment as well. > Also a good reference for later: > http://www.cplusplus.com/doc/tutorial/typecasting/ Reply by plucia on 24 April 2020, 09:49 > Hey, yes this can be resolved. When I first looked > over the change, I misread it and thought you were > doing a static_cast(vData) instead of > static_cast(QMetaType::Float), which > gave me pause. Since that's not the case, this change > looks good to me Reply by plucia on 24 April 2020, 10:00 > RESOLVED Revision Comment by plucia on 22 April 2020, 12:47 https://devapps.diality.us/cru/UI-DEN-2087-1#c1618 If the vData = 0 and is converted from a QVariant::UInt, QVariant::Int, or QVariant::Float to quint32, qint32, or float successfully, how would the caller of fromVariant know whether the value was converted successfully or not? Reply by Behrouz NematiPour on 22 April 2020, 12:56 > I'm not sure if understood the question correctly! > 1 - if the value is 0 it will be converted always > successfully to on of the types : Int, UInt, Float. > 2 - There is bool ok which will be set if the value cannot be > converted. > 3 - if conversion is not successful, an empty QBytearray will > return (shows the error happened in conversion because even a > value of zero has a bytearray len of 1) which will be tested > against another criteria for required length for a message. > 4 - because the type has been checked first there should > never happen any error according to documents. > I hope the explanation helped Reply by plucia on 22 April 2020, 13:11 > Say for some (unlikely) reason that vData can't be read or > converted properly any longer, and there is an error > converting it but not an error in checking its type. > If QVariant vData = 4, then you run vData.toInt(&ok) and > it's not successful and says 0. How would the caller be > able to know there is an error in the conversion just from > the message length in this case? int 4 has the same length > as int 0. Reply by Behrouz NematiPour on 22 April 2020, 14:57 > if case of error of casting 4 it doesn't return 0 it > returns nothing. > To answer the question yes the length of the returned > QByteArray is the error identification. > In some functions an uninitialized returned object is the > way of unsuccessful action like returning null or an > empty object. Reply by plucia on 23 April 2020, 09:08 > Wait so in that case mData would be written over with > the call: Types::setValue(s32, mData) > Then mData is returned, with a length of one. > Would the caller know whether the value was correctly > converted to 0, or converted to 0 in error in this > case? Reply by Behrouz NematiPour on 23 April 2020, 20:07 > OK, I made the changes. > I found the point of confusion. > The code in this branch is 2-3 weeks older than > latest code. after doing the changes and merge I > found out what is the point of the confusion. > I modified the code anyway. Reply by plucia on 24 April 2020, 10:00 > RESOLVED ---------------------------------------- File: sources/utility/types.h Revision Comment by pmontazemi on 16 April 2020, 08:01 https://devapps.diality.us/cru/UI-DEN-2087-1#c1613 Where is types.cpp? Reply by Behrouz NematiPour on 16 April 2020, 10:49 > both cpp and header are next to each other. > FishEye is adding what ever file that has been changed. Reply by pmontazemi on 22 April 2020, 15:17 > RESOLVED. ---------------------------------------- File: main.cpp Revision Comment by plucia on 22 April 2020, 12:59 https://devapps.diality.us/cru/UI-DEN-2087-1#c1622 Are these placeholders for something? Seems like they could just be removed Reply by Behrouz NematiPour on 22 April 2020, 13:03 > These are not placeholders but helps to find next command > line argument definition. > Since the type of arguments are different and for different > type there should be different implementation, it's not easy > to find each. Reply by plucia on 24 April 2020, 16:07 > Resolving as per our conv. today regarding placeholders Reply by plucia on 24 April 2020, 16:09 > RESOLVED ---------------------------------------- File: sources/storage/filehandler.cpp Revision Comment by pmontazemi on 16 April 2020, 07:59 https://devapps.diality.us/cru/UI-DEN-2087-1#c1611 Remove commented line. Reply by Behrouz NematiPour on 22 April 2020, 15:30 > Done Reply by pmontazemi on 22 April 2020, 15:35 > RESOLVED. --- ID: UI-DEN-2087-1 https://devapps.diality.us/cru/UI-DEN-2087-1 Title: UI-DEN-2087_UI Blood/Dialysate Flow Rate Adjustment Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Moderator: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)