This is a list of all comments for UI-DEN-3605-4. Review Summary: No summary General Comment by Behrouz NematiPour on 25 August 2020, 14:29 https://devapps.diality.us/cru/UI-DEN-3605-4#c3925 I see duplicate files names which should not be if the correct change sets have been chosen. I don't believe this code review is valid. Seems like Jira can't handle this correctly. Reply by plucia on 28 August 2020, 13:17 > Yes to remove the merged code from Logging2of3 I had to > create a new review. I looked into removing the your merged > code from the old review and Crucible doesn't really let you > if you create the review from a branch. > So, the duplication of files does appear to be a bug in > Crucible. The process isn't perfect in either case, but I > think I addressed and committed changes for all of your > comments from the previous review. If I missed anything > please let me know Reply by plucia on 28 August 2020, 13:39 > Here is a link to the crucible bug: > [https://jira.atlassian.com/browse/CRUC-6806]. Looking over > it, it seems they are not planning on fixing it Reply by Behrouz NematiPour on 05 October 2020, 15:02 > RESOLVED General Comment by plucia on 15 September 2020, 09:47 https://devapps.diality.us/cru/UI-DEN-3605-4#c4703 This code review was created on August 25. The design changes with MAbstract and MModel were merged to master on August 31st. Our process now discourages merging code between development branches, so the create treatment implementation doesn't incorporate the design changes that were first merged to master on August 31st. As per Behrouz's and my conversation yesterday, the work to subclass MAbstract and MModel is going to be tracked in this ticket next sprint. [http://dvm-linux02:8080/browse/DEN-4981] Reply by Behrouz NematiPour on 05 October 2020, 15:02 > Fine General Comment by Behrouz NematiPour on 07 October 2020, 10:03 https://devapps.diality.us/cru/UI-DEN-3605-4#c5322 Please add the testsuites repository in this code review. It's always part of the Code Review. Reply by plucia on 07 October 2020, 13:17 > Dara has created the review, it's available here: > [http://dvm-linux02:8060/cru/TESTSUITES-DEN-3724-1] > and is linked to this review (see "Linked Reviews" above) Reply by Behrouz NematiPour on 15 October 2020, 10:23 > RESOLVED. General Comment by Behrouz NematiPour on 07 October 2020, 16:53 https://devapps.diality.us/cru/UI-DEN-3605-4#c5399 Please put "done" when the code is pushed. When an email is sent to the reviewer it is assumed that there is something to review but putting a "done" and "will be updated" is not something that can be reviewed. Reply by plucia on 07 October 2020, 17:12 > I don't know why, but crucible creates duplicate files > sometimes. Please check on the left hand pane and see the > change(s) in the duplicate files. Reply by Behrouz NematiPour on 14 October 2020, 10:53 > RESOLVED ---------------------------------------- File: sources/canbus/messageinterpreter.cpp Revision Comment by Behrouz NematiPour on 08 September 2020, 12:20 https://devapps.diality.us/cru/UI-DEN-3605-4#c4461 messageinterpreter.cpp file in here shows "File Outdated" but not like the others have duplicate the latest. Please confirm that this file is what you meant it to be ... Reply by plucia on 08 September 2020, 12:27 > Confirmed. > > I think the latest changes to this file were part of the > merge from master to this branch. > > I excluded the merge commits when creating this review after > the request to remove merge commits from the old review: > [http://dvm-linux02:8060/cru/UI-DEN-3605-3#general-comments] Reply by Behrouz NematiPour on 08 September 2020, 18:46 > Resolved. Revision Comment by Behrouz NematiPour on 08 September 2020, 12:25 https://devapps.diality.us/cru/UI-DEN-3605-4#c4463 Please from now on use Camel Case for file names. So it should be MTreatmentParameters(.h , .cpp) Reply by plucia on 08 September 2020, 13:38 > Done Reply by Behrouz NematiPour on 05 October 2020, 16:58 > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 20:33 https://devapps.diality.us/cru/UI-DEN-3605-4#c4554 The MModel.h has been included in the MessageInterpreter.h which has the models #include to the MTreatmentParameters. Please remove this duplicate inclusion. Please follow the code structure and please notice that there is no other model included here and has to be a reason for that. Reply by Behrouz NematiPour on 05 October 2020, 17:01 > Removed so, > RESOLVED. Revision Comment by Behrouz NematiPour on 08 September 2020, 20:41 https://devapps.diality.us/cru/UI-DEN-3605-4#c4555 All the classes in this category have the naming structure of : AdjustRequestData , Why this class doesn't follow that convention? It hasn't been followed in AlarmSilenceRequestData as well, but it has Data at the end. Please follow the code structure. Reply by plucia on 09 September 2020, 16:49 > Treatment parameters are set before the start of a treatment, > so naming them with Adjust prefix is misleading of the > intended function. Reply by Behrouz NematiPour on 09 September 2020, 20:37 > This is the user adjustments of parameters in pre-treatment > which some of them will also be available in-treatment for > re-adjustment. > So please mention the Adjust. Reply by plucia on 15 September 2020, 09:52 > Done Reply by Behrouz NematiPour on 05 October 2020, 17:01 > As we talked it still needs some other mods. Reply by plucia on 06 October 2020, 12:15 > This has since been updated on the latest development > branch as follows: > > {code} > case Gui::GuiActionType::ID_EndTreatmentReq: > { > if ( count != 0 ) { > logInvalidLength(vActionId); return false; } > vPayload = Format::fromVariant(vData); > > LOG_EVENT(EndTreatmentRequestData::toString(vData)); > } break; > > case > Gui::GuiActionType::ID_CreateTreatmentReq: { > if ( ! count ) { > logInvalidLength(vActionId); return false; } > vPayload = Format::fromVariant(vData); > > LOG_EVENT(AdjustTreatmentParametersRequestData::toString(vData)); > } break; > {code} > > [http://dvm-linux02:7990/projects/UI/repos/application/browse/sources/canbus/messageinterpreter.cpp?at=refs%2Fheads%2FDEN-4598-Confirm-Priming-Begin] Reply by Behrouz NematiPour on 07 October 2020, 10:02 > Observed in the link above. > RESOLVED. Revision Comment by Behrouz NematiPour on 08 September 2020, 20:46 https://devapps.diality.us/cru/UI-DEN-3605-4#c4556 Please remove this ! Reply by plucia on 09 September 2020, 15:55 > Done Reply by Behrouz NematiPour on 05 October 2020, 16:53 > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 20:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c4557 Please follow the code structure. all the other cases are like each other and consistent why this implementation is different? All : vPayload = Format::fromVariant(vData); LOG_EVENT(AdjustSalineRequestData::toString(vData)); This : vPayload = Format::fromVariant(vData); // -------> Why data passed to this method instead of toString? TreatmentParameters params; params.fromVariantList(vData); // -------> why do you have this here? LOG_EVENT("GOT HERE"); LOG_EVENT(params.toString()); // -------> same as first comment.... Reply by plucia on 09 September 2020, 17:10 > I looked into subclassing MAbstract at the time and again > today but there's some incompatibility there that needs to be > worked through. > How about I work on subclassing MAbstract in my current > story? I don't think it would be good to change too much > structure against these revisions as the code has been > sitting for awhile and other things have changed quite a bit. > Also, I think the MAbstract class may have been added near > the end of my development on DEN-3605. > I'd like to make sure our models and code structure are all > aligned too. I think it's better to do that on the latest > code rather than the branch this code is sitting on which is > outdated at this point. Reply by plucia on 15 September 2020, 09:37 > This code review was created on August 25. > The design changes with MAbstract and MModel were merged to > master on August 31st. > Our process now discourages merging code between development > branches, so the create treatment implementation doesn't > incorporate the design changes that were first merged to > master on August 31st. > As per our convesation yesterday, the work to subclass > MAbstract and MModel, which will change the lines you're > asking about, is going to be tracked in this ticket next > sprint. [http://dvm-linux02:8080/browse/DEN-4981] Reply by Behrouz NematiPour on 05 October 2020, 16:51 > will be reviewed later on the assigned code review. > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 20:57 https://devapps.diality.us/cru/UI-DEN-3605-4#c4558 Please follow the code structure for consistency. None of the methods in this category has Data at the end, why this one has? and the ones who are not using the notify template method yet, have the adjust at the begging of the name, why this one hasn't? Reply by plucia on 09 September 2020, 16:35 > What would you like me to rename this to? Reply by plucia on 14 September 2020, 16:21 > As per our conversation today, due to the design changes with > MAbstract.h and MModel.h that were merged to master on Aug. > 31st, the create treatment models will be updated to subclass > these parent classes. > This work is going to be tracked in this ticket: > [http://dvm-linux02:8080/browse/DEN-4981] > > This function will be removed once those changes are > implemented since after being subclassed, it can use notify > instead. Reply by Behrouz NematiPour on 05 October 2020, 16:51 > will be reviewed later on the assigned code review. > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 21:01 https://devapps.diality.us/cru/UI-DEN-3605-4#c4559 Please follow the code structure for consistency. As mentioned above please follow the namings. Reply by plucia on 15 September 2020, 09:53 > This function will be deleted next sprint when I subclass > MModel and MAbstract Reply by Behrouz NematiPour on 05 October 2020, 16:47 > It will be reviewed later. > RESOLVED. ---------------------------------------- File: sources/view/VCreateTreatment.cpp Revision Comment by pmontazemi on 25 August 2020, 10:48 https://devapps.diality.us/cru/UI-DEN-3605-4#c3869 Remove extra line. Reply by plucia on 28 August 2020, 17:10 > Done - Adjust the slider to view a198262d through 810e4b1 Reply by pmontazemi on 01 September 2020, 09:18 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c3870 For all these conditions, either one-liner, or left align arguments if cascaded. Reply by plucia on 28 August 2020, 12:02 > Done Reply by pmontazemi on 28 August 2020, 13:12 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:12 https://devapps.diality.us/cru/UI-DEN-3605-4#c4194 Remove extra line. Reply by plucia on 28 August 2020, 13:29 > Done Reply by Dara Navaei on 19 October 2023, 11:33 > RESOLVED Revision Comment by pmontazemi on 25 August 2020, 10:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c3871 Remove extra line. Reply by plucia on 28 August 2020, 13:53 > Done Reply by pmontazemi on 28 August 2020, 17:04 > RESOLVED. Revision Comment by pmontazemi on 09 September 2020, 14:59 https://devapps.diality.us/cru/UI-DEN-3605-4#c4668 Remove extra line. Reply by plucia on 14 September 2020, 16:57 > Done, see ad32b29 Reply by pmontazemi on 14 September 2020, 18:55 > RESOLVED. Revision Comment by pmontazemi on 09 September 2020, 14:59 https://devapps.diality.us/cru/UI-DEN-3605-4#c4667 Remove extra line. Reply by plucia on 14 September 2020, 16:53 > Done, see 5387db3 Reply by pmontazemi on 14 September 2020, 18:54 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c3872 Remove extra line. Reply by plucia on 28 August 2020, 13:51 > Done - See commits from a19862d to 810e4b1 (drag the slider > at the top) Reply by pmontazemi on 28 August 2020, 17:04 > RESOLVED. ---------------------------------------- File: sources/view/VCreateTreatment.h Revision Comment by Behrouz NematiPour on 30 September 2020, 17:57 https://devapps.diality.us/cru/UI-DEN-3605-4#c5073 I mentioned before, but I don't see why another specific macro other than the standards have been created? Reply by Behrouz NematiPour on 06 October 2020, 14:57 > RESOLVED Revision Comment by pmontazemi on 28 August 2020, 13:13 https://devapps.diality.us/cru/UI-DEN-3605-4#c4195 Remove extra line. Reply by plucia on 28 August 2020, 13:47 > Done Reply by pmontazemi on 28 August 2020, 16:59 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c3873 Remove extra line. Reply by plucia on 28 August 2020, 13:24 > Done Reply by pmontazemi on 28 August 2020, 17:05 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c3874 Remove extra line. Reply by plucia on 28 August 2020, 13:24 > Done Reply by pmontazemi on 28 August 2020, 17:05 > RESOLVED. ---------------------------------------- File: sources/storage/filesaver.h Revision Comment by Behrouz NematiPour on 30 September 2020, 18:27 https://devapps.diality.us/cru/UI-DEN-3605-4#c5078 Why another class has been created for saving files. What is the advantage of this class other than inherited from Q_Object and can have signal/slot. It could be a simple single function in another class inherited from Q_Object. Reply by Behrouz NematiPour on 05 October 2020, 15:33 > After the review the implementation is fine. > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:48 https://devapps.diality.us/cru/UI-DEN-3605-4#c3867 Remove extra line. Reply by plucia on 28 August 2020, 12:02 > Done Reply by pmontazemi on 28 August 2020, 13:11 > RESOLVED. ---------------------------------------- File: unittests/tst_messaging.h Revision Comment by pmontazemi on 25 August 2020, 10:51 https://devapps.diality.us/cru/UI-DEN-3605-4#c3879 Remove extra line at EOF. Reply by plucia on 28 August 2020, 13:23 > Done Reply by pmontazemi on 28 August 2020, 17:04 > RESOLVED. ---------------------------------------- File: unittests/tst_views.cpp Revision Comment by Behrouz NematiPour on 30 September 2020, 16:39 https://devapps.diality.us/cru/UI-DEN-3605-4#c5053 Why added QThread? Reply by plucia on 30 September 2020, 16:52 > It's necessary for adding delays since the file writer class > runs in a separate thread. To check that a file was written, > we wait for more than enough time before checking the file > was written properly. Reply by Behrouz NematiPour on 30 September 2020, 17:00 > Do not agree with this delay. > Your implementation should not depend on a wait and if you > that important to make sure it is done you need to have > signal in your class to emit when write or read is done. > Also you are using the static method and that function is > pauseing the current thread I believe using the static > method will pause the entire test. > This is not a good test. > Please also look at the link below: > https://doc.qt.io/qt-5/qthread.html#msleep Reply by plucia on 30 September 2020, 17:09 > The _FileSaver class does not provide a filename that it > emits when a file is saved. It only emits a boolean of > whether the file was saved. > Also, connecting it to tst_views.cpp would not help > because how do I know which file to check when my slot is > called? Also, the slot would have to know which test > needs to receive a signal from _FileSaver so it can > evaluate the result. If the test should be self contained > as you mentioned in the other comment then does it make > sense to connect it to _FileSaver? Reply by Behrouz NematiPour on 30 September 2020, 17:35 > make the tst_view your class friend. > so it will have access to the private members of your > class. > Please read the existing code thoroughly and see how > some issues have been solved and please do not reinvent > the wheel. > > "********* I must emphasize that making a class a > friend of the other is only allowed for test classes. > *********" > And also please do these tests in SquishQt, not in C++, > look at the next commit. Reply by plucia on 07 October 2020, 16:29 > I looked into implementing what you suggest and it > would require modification of both _FileSaver and > _FileHandler to work properly. > You've discouraged modifying the production code to > make the tests work in the past. We would definitely > need to modify the production code in this case to > get the test to work as you describe. > Also, does it make sense to make tst_views.cpp a > friend of the FileSaver class? > Please remember this is test code not production > code, and that the test is already completely > functional, working, and passing. > Can you point me to a place in our coding standard > that discourages using QThread in non-production test > code? Reply by plucia on 14 October 2020, 13:14 > I have found a way to test this without using > QThread. So, QThread has been removed Reply by Behrouz NematiPour on 15 October 2020, 10:45 > RESOLVED Reply by Behrouz NematiPour on 30 September 2020, 17:05 > I believe it was better to do it like the other classes and > use SquishQt to test the whole stack of MVC and if that > didn't work cover more in here with simple small functions. > The test functions are so big and have so many compare in > just one function. > This is not the correct way of testing. Reply by plucia on 07 October 2020, 16:41 > These tests are very thorough and actually test the code > on a level not possible with SquishQt. > SquishQt is more of a black-box approach and for mission > critical sections of the code it cannot cover everything > as thoroughly. > This test helped me root out bugs as I was writing it. I > think it has helped in the development. Reply by plucia on 14 October 2020, 13:14 > I have found a way to test this without using QThread. > So, QThread has been removed Reply by Behrouz NematiPour on 15 October 2020, 10:48 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 17:52 https://devapps.diality.us/cru/UI-DEN-3605-4#c5072 Why this class included? Reply by plucia on 07 October 2020, 16:44 > It is deleted now Reply by Behrouz NematiPour on 07 October 2020, 16:55 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:52 https://devapps.diality.us/cru/UI-DEN-3605-4#c3881 Remove extra line. Reply by plucia on 28 August 2020, 13:23 > Done Reply by pmontazemi on 01 September 2020, 09:17 > RESOLVED. ---------------------------------------- File: unittests/tst_models.cpp Revision Comment by Behrouz NematiPour on 30 September 2020, 17:43 https://devapps.diality.us/cru/UI-DEN-3605-4#c5071 In this function, this is not the correct test data provided. it is not covering all the different data. for each byte, there should be a line and if those bytes are related to a parameter should be numbered the same and each param has a different number. So for 18 params and every 4 bytes, it should be 18x4 line of data with and extra for the message completer also do not break the lines it is a test data, not a code. And why all the parameters are 00 there should be different values used to see if the function can handle anything other than one value like 00. Reply by plucia on 14 October 2020, 14:02 > Done Reply by Behrouz NematiPour on 15 October 2020, 10:22 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:51 https://devapps.diality.us/cru/UI-DEN-3605-4#c3880 Remove extra 2 lines. Reply by plucia on 28 August 2020, 13:23 > Done Reply by pmontazemi on 28 August 2020, 17:02 > RESOLVED. ---------------------------------------- File: unittests/tst_views.h Revision Comment by Behrouz NematiPour on 30 September 2020, 16:42 https://devapps.diality.us/cru/UI-DEN-3605-4#c5054 A test should be self-contained. QTest supposes to call this test function automatically. so what happens to the parameter view? If this test needs an object it has to be created in the test function. Reply by plucia on 30 September 2020, 17:02 > This function is not a test function its a helper function > for a test function Reply by Behrouz NematiPour on 30 September 2020, 17:32 > So do you know how to tell QtTest not to run it > automatically? > Please move it to a private section of the class if it is > just a helper function. > > Please refer to the comment for the implementation. > I strongly disagree with this way of testing. Reply by plucia on 06 October 2020, 11:20 > When I tested this Qt did not run the helper functions as > test functions. It appears to be able to differentiate > between the two without any additional directives. > > As requested, I've made the helper functions private > instead of private slots to make it absolutely clear > which functions are helper functions and which aren't. Reply by Behrouz NematiPour on 07 October 2020, 09:56 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 16:45 https://devapps.diality.us/cru/UI-DEN-3605-4#c5055 A test should be self-contained. QTest supposes to call this test function automatically. so what happens to the parameter obj? If this test needs an object it has to be created in the test function. Reply by plucia on 30 September 2020, 17:02 > This function is not a test function its a helper function to > a test function Reply by Behrouz NematiPour on 07 October 2020, 09:56 > RESOLVED ---------------------------------------- File: sources/storage/storageglobals.h Revision Comment by pmontazemi on 25 August 2020, 10:48 https://devapps.diality.us/cru/UI-DEN-3605-4#c3868 Remove extra line. Reply by plucia on 28 August 2020, 12:02 > Done Reply by pmontazemi on 28 August 2020, 13:11 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/TreatmentConfirm.qml Revision Comment by Behrouz NematiPour on 30 September 2020, 22:36 https://devapps.diality.us/cru/UI-DEN-3605-4#c5103 Same comment as http://192.168.10.132:8060/cru/UI-DEN-3605-4#c5093 Reply by Behrouz NematiPour on 06 October 2020, 14:21 > Modification has been made. Will follow up on the changed > code. > RESOLVED Revision Comment by pmontazemi on 28 August 2020, 13:07 https://devapps.diality.us/cru/UI-DEN-3605-4#c4177 Remove 2 extra lines. Reply by plucia on 28 August 2020, 13:30 > Done Reply by pmontazemi on 28 August 2020, 17:02 > RESOLVED. ---------------------------------------- File: sources/gui/qml/components/RectSelectCreateTreatment.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:10 https://devapps.diality.us/cru/UI-DEN-3605-4#c5281 Please put a short description of your component and explain what it does and where it can be used. Since it is a specific or compound component and is not a general component like a just a Slider or Progressbar, it helps to understand the use case of the component. Reply by plucia on 06 October 2020, 17:42 > Done, will post the update soon Reply by Behrouz NematiPour on 07 October 2020, 09:47 > Later please use more familiar terms like "Option Button". > The layout "grid of options" is feature of "Option Button" > not the component itself. > RESOLVED ---------------------------------------- File: sources/gui/qml/components/SliderCreateTreatment.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:11 https://devapps.diality.us/cru/UI-DEN-3605-4#c5282 Please put a short description of your component and explain what it does and where it can be used. Since it is a specific or compound component and is not a general component like a just a Slider or Progressbar, it helps to understand the use case of the component. Reply by plucia on 06 October 2020, 17:36 > Done will post the update soon Reply by Behrouz NematiPour on 07 October 2020, 09:45 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/SliderDoubleCreateTreatment.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:11 https://devapps.diality.us/cru/UI-DEN-3605-4#c5283 Please put a short description of your component and explain what it does and where it can be used. Since it is a specific or compound component and is not a general component like a just a Slider or Progressbar, it helps to understand the use case of the component. Reply by plucia on 06 October 2020, 17:34 > Done, will post the update soon Reply by Behrouz NematiPour on 07 October 2020, 09:45 > RESOLVED ---------------------------------------- File: sources/gui/qml/pages/treatment/TreatmentCreate.qml Revision Comment by Behrouz NematiPour on 30 September 2020, 21:14 https://devapps.diality.us/cru/UI-DEN-3605-4#c5094 Why this component is not in the code review? Reply by Behrouz NematiPour on 05 October 2020, 15:14 > Now it's part of the code review. > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 21:16 https://devapps.diality.us/cru/UI-DEN-3605-4#c5095 Why the Slider.qml component has not been used? Reply by Behrouz NematiPour on 30 September 2020, 21:19 > I remembered we talked about it. > RESOLVED. Revision Comment by Behrouz NematiPour on 30 September 2020, 21:21 https://devapps.diality.us/cru/UI-DEN-3605-4#c5097 What are these two extra object names? If it's that much necessary can be moved inside the component implementation. You have the object name and the postfix string is a constant string. Reply by Behrouz NematiPour on 05 October 2020, 15:12 > It has been done on the other comments. > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 21:22 https://devapps.diality.us/cru/UI-DEN-3605-4#c5098 It's a very immediate value setting to sending value to the view. Also sending these values before checking the dependencies to the other values seems redundant. I suggest keeping the values and on confirm (or whatever the confirm button is) sending the final values to the view. Please apply this for performance. Reply by plucia on 05 October 2020, 09:59 > I removed all of these and just tested on the target and the > performance appears to be the same. > > The cpp code is in control of the business logic of when to > enable/disable the continue button. Making the qml in control > divides responsibility between the cpp code and qml as the > cpp code needs to enable / disable it when we get a response > back from FW. > > The cpp code keeps track of which variable has been set and > which hasn't. > > Here are a couple of relevant requirements: > > SRSUI 551: The "Continue" button shall be only enabled after > all the "Treatment Parameters" has been set by user. > SRSUI 560: The "Create Treatment" screen shall display the > "Treatment Parameters" values as has been set after user sets > the value of each "Treatment Parameter". Reply by Behrouz NematiPour on 05 October 2020, 11:23 > That is obvious that calling multiple layers of functions > on each user slider movement and value change. > Let's say we have a slider in range of -100 to +700 (which > is a valid actual range in PRS) with resolution of 1 and > user easily can drag the slider, then imagine what amount > of call will happen for a single user slide. > It may not currently visibly show a slow down but later by > adding more into the code it may cause us problems. > The SRSUI doesn't force the design it's up to designer and > developer to meet the requirement while keeps the code > performance high. > If it's absolutely necessary the please add a release > signal and handle it at that time. Reply by plucia on 07 October 2020, 13:20 > I've updated it so the signal will only be called when > the mouse press changes > > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16966] Reply by Behrouz NematiPour on 15 October 2020, 11:02 > RESOLVED. > As we talked it should be changed to released(var > vMouseEvent) signal and will be emitted in the > onReleased{} slot of the MouseArea of RangeRect. Revision Comment by Behrouz NematiPour on 30 September 2020, 22:10 https://devapps.diality.us/cru/UI-DEN-3605-4#c5100 Missing translations. Please apply to all the other occurrences, there are so many on this page, I don't put comment for all. Reply by plucia on 05 October 2020, 10:39 > Done, will post update to this review soon Reply by Behrouz NematiPour on 05 October 2020, 15:11 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 22:06 https://devapps.diality.us/cru/UI-DEN-3605-4#c5099 The translatable text should not have beginning or end spaces. Space is not part of the translation it is part of the code. Also, it would be an even better idea to put it in the Variables.qml as a constant. Later we may categorize them all like Variables.rate : qsTr("mL/min") or Variables.pressure : qsTr("mmHg") and so. Please apply to all the other occurrences. Reply by plucia on 05 October 2020, 10:46 > Done, will post update to this review soon Reply by Behrouz NematiPour on 05 October 2020, 15:08 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 22:19 https://devapps.diality.us/cru/UI-DEN-3605-4#c5101 Same comment as : http://192.168.10.132:8060/cru/UI-DEN-3605-4#c5093 Reply by plucia on 05 October 2020, 11:51 > Done will post the update soon Reply by Behrouz NematiPour on 05 October 2020, 15:11 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 22:33 https://devapps.diality.us/cru/UI-DEN-3605-4#c5102 Please remove this debug codes or comment them as // TODO to be removed ASAP. There are more of these which I didn't put comment. Please apply to all. Reply by plucia on 05 October 2020, 11:50 > Done, will post the update soon Reply by Behrouz NematiPour on 05 October 2020, 15:11 > RESOLVED Revision Comment by pmontazemi on 28 August 2020, 13:08 https://devapps.diality.us/cru/UI-DEN-3605-4#c4178 Remove extra line. Reply by plucia on 28 August 2020, 17:09 > Done Reply by pmontazemi on 01 September 2020, 09:17 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:08 https://devapps.diality.us/cru/UI-DEN-3605-4#c4179 Remove extra line. Reply by plucia on 28 August 2020, 13:50 > Done Reply by pmontazemi on 28 August 2020, 17:01 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:08 https://devapps.diality.us/cru/UI-DEN-3605-4#c4180 Remove extra line. Reply by plucia on 28 August 2020, 13:49 > Done Reply by pmontazemi on 28 August 2020, 17:01 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:08 https://devapps.diality.us/cru/UI-DEN-3605-4#c4181 Remove 2 extra lines. Reply by plucia on 28 August 2020, 13:49 > Done Reply by pmontazemi on 28 August 2020, 17:01 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:08 https://devapps.diality.us/cru/UI-DEN-3605-4#c4182 Remove 3 extra lines. Reply by plucia on 28 August 2020, 13:49 > Done Reply by pmontazemi on 28 August 2020, 17:01 > RESOLVED. ---------------------------------------- File: sources/applicationcontroller.cpp Revision Comment by Behrouz NematiPour on 08 September 2020, 20:31 https://devapps.diality.us/cru/UI-DEN-3605-4#c4553 I don't see any reference to the QJson inclusion in this file. Why these have been included? Reply by plucia on 09 September 2020, 17:01 > It's left over from when the treatment parameter saving code > was in the application controller. I've removed it now Reply by Behrouz NematiPour on 05 October 2020, 16:55 > RESOLVED ---------------------------------------- File: sources/applicationcontroller.h Revision Comment by Behrouz NematiPour on 08 September 2020, 20:29 https://devapps.diality.us/cru/UI-DEN-3605-4#c4552 The model #include is in the MModel.h by design. So don't need to be included here twice. Please follow the code structure. Reply by plucia on 09 September 2020, 16:57 > Done Reply by Behrouz NematiPour on 05 October 2020, 16:56 > RESOLVED Revision Comment by pmontazemi on 25 August 2020, 10:50 https://devapps.diality.us/cru/UI-DEN-3605-4#c3877 Remove extra line. Reply by plucia on 28 August 2020, 13:22 > Done Reply by pmontazemi on 28 August 2020, 17:05 > RESOLVED. ---------------------------------------- File: sources/canbus/messageglobals.h Revision Comment by pmontazemi on 25 August 2020, 10:12 https://devapps.diality.us/cru/UI-DEN-3605-4#c3854 Remove extra line. Reply by plucia on 28 August 2020, 11:57 > Done Reply by pmontazemi on 28 August 2020, 13:06 > RESOLVED. ---------------------------------------- File: sources/canbus/messageinterpreter.h Revision Comment by pmontazemi on 25 August 2020, 10:44 https://devapps.diality.us/cru/UI-DEN-3605-4#c3856 Remove extra line. Reply by plucia on 28 August 2020, 11:57 > Done Reply by pmontazemi on 28 August 2020, 13:07 > RESOLVED. ---------------------------------------- File: sources/gui/guicontroller.h Revision Comment by pmontazemi on 25 August 2020, 10:46 https://devapps.diality.us/cru/UI-DEN-3605-4#c3859 Remove extra line. Reply by plucia on 28 August 2020, 12:00 > Done Reply by pmontazemi on 30 September 2020, 20:05 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:46 https://devapps.diality.us/cru/UI-DEN-3605-4#c3860 Remove extra line. Reply by plucia on 28 August 2020, 12:00 > Done Reply by pmontazemi on 28 August 2020, 13:10 > RESOLVED. ---------------------------------------- File: sources/gui/qml/dialogs/NotificationDialog.qml Revision Comment by Behrouz NematiPour on 30 September 2020, 22:46 https://devapps.diality.us/cru/UI-DEN-3605-4#c5109 http://192.168.10.132:8060/cru/UI-DEN-3605-4#c5065 Reply by plucia on 06 October 2020, 17:46 > Done, will post update soon Reply by Behrouz NematiPour on 07 October 2020, 09:53 > RESOLVED ---------------------------------------- File: sources/gui/qml/main.qml Revision Comment by pmontazemi on 28 August 2020, 13:09 https://devapps.diality.us/cru/UI-DEN-3605-4#c4186 Remove 2 extra lines. Reply by plucia on 28 August 2020, 17:08 > Done Reply by pmontazemi on 01 September 2020, 09:16 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:10 https://devapps.diality.us/cru/UI-DEN-3605-4#c4187 Remove extra line. Reply by plucia on 28 August 2020, 13:47 > Done Reply by pmontazemi on 28 August 2020, 17:00 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:10 https://devapps.diality.us/cru/UI-DEN-3605-4#c4188 Remove extra line. Reply by plucia on 28 August 2020, 13:47 > Done Reply by pmontazemi on 28 August 2020, 17:00 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:10 https://devapps.diality.us/cru/UI-DEN-3605-4#c4189 Remove extra line. Reply by plucia on 28 August 2020, 13:47 > Done Reply by pmontazemi on 28 August 2020, 17:00 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/TreatmentStack.qml Revision Comment by pmontazemi on 28 August 2020, 13:09 https://devapps.diality.us/cru/UI-DEN-3605-4#c4185 Remove 3 extra lines. Reply by plucia on 28 August 2020, 13:30 > Done Reply by pmontazemi on 28 August 2020, 17:00 > RESOLVED. ---------------------------------------- File: sources/main.h Revision Comment by Behrouz NematiPour on 30 September 2020, 16:33 https://devapps.diality.us/cru/UI-DEN-3605-4#c5051 Why did you do this! Reply by plucia on 30 September 2020, 17:03 > Because it helped me debug my code during development Reply by Behrouz NematiPour on 07 October 2020, 09:58 > RESOLVED. Revision Comment by Behrouz NematiPour on 30 September 2020, 16:35 https://devapps.diality.us/cru/UI-DEN-3605-4#c5052 Why didn't use the standard predefined PROPERY? This way it's not like the other implementation. We don't use set_ and/or get_ . Reply by plucia on 30 September 2020, 16:45 > If you look at the latest revision of main.h on the > Confirm-Priming-Begin branch you'll see I've since added a > min / max, a default, as well as a resolution value as a > parameter to the TREATMENT_PARAMETER macro. > PROPERTY only has three parameters. TREATMENT_PARAMETER needs > these extra parameters, and is implemented differently to > serve the different needs of treatment parameters. > I left PROPERTY alone since other code depends on it. Reply by Behrouz NematiPour on 06 October 2020, 14:44 > RESOLVED ---------------------------------------- File: sources/storage/filehandler.h Revision Comment by Behrouz NematiPour on 30 September 2020, 19:36 https://devapps.diality.us/cru/UI-DEN-3605-4#c5084 Is this class used? Reply by plucia on 06 October 2020, 11:08 > It is now deleted Reply by Behrouz NematiPour on 07 October 2020, 09:53 > RESOLVED Revision Comment by pmontazemi on 25 August 2020, 10:47 https://devapps.diality.us/cru/UI-DEN-3605-4#c3865 Remove extra line. Reply by plucia on 28 August 2020, 13:26 > Done Reply by pmontazemi on 28 August 2020, 17:03 > RESOLVED. ---------------------------------------- File: sources/view/vview.h Revision Comment by pmontazemi on 25 August 2020, 10:50 https://devapps.diality.us/cru/UI-DEN-3605-4#c3875 Remove extra line. Reply by plucia on 28 August 2020, 13:22 > Done Reply by pmontazemi on 28 August 2020, 17:05 > RESOLVED. ---------------------------------------- File: denali.pro Revision Comment by Behrouz NematiPour on 30 September 2020, 17:30 https://devapps.diality.us/cru/UI-DEN-3605-4#c5068 What is it and where has it been used ? Reply by Behrouz NematiPour on 06 October 2020, 14:56 > remove this, please. > It's not standard. Reply by plucia on 07 October 2020, 16:26 > Done, will post update soon Reply by Behrouz NematiPour on 07 October 2020, 16:52 > Please put "done" when the code is pushed. Reply by plucia on 07 October 2020, 17:09 > Done, I don't know why, but crucible created another > denali.pro file that shows this change: > > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16969] Reply by Behrouz NematiPour on 14 October 2020, 11:04 > RESOLVED ---------------------------------------- File: sources/gui/qml/globals/Colors.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:28 https://devapps.diality.us/cru/UI-DEN-3605-4#c5287 Please change Btn to Button and re-align the ':'. In general please don't use these kinds of simplifications unless it's too obvious like Rectangle->rect. Reply by plucia on 06 October 2020, 17:45 > Done, will post update soon Reply by Behrouz NematiPour on 07 October 2020, 09:44 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 22:43 https://devapps.diality.us/cru/UI-DEN-3605-4#c5108 Please change Tr to Treatment. It is not clear if used separately from the other ones. The clear meaning is preferred. Please align afterward. Apply to all the others, please. Reply by plucia on 06 October 2020, 10:59 > Done Reply by Behrouz NematiPour on 06 October 2020, 13:32 > RESOLVED ---------------------------------------- File: sources/storage/storageglobals.cpp Revision Comment by Behrouz NematiPour on 30 September 2020, 18:22 https://devapps.diality.us/cru/UI-DEN-3605-4#c5076 Why this class is included. Reply by plucia on 07 October 2020, 13:29 > I've removed it and will post an update to this review soon. Reply by Behrouz NematiPour on 07 October 2020, 16:51 > Please put "done" when the code is pushed. Reply by plucia on 14 October 2020, 09:09 > See the duplicate storageglobals.cpp on the left Reply by Behrouz NematiPour on 14 October 2020, 11:00 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 18:23 https://devapps.diality.us/cru/UI-DEN-3605-4#c5077 Good idea but not a correct macro. There should be a macro to identify if you build for iMX8 or desktop 64 bit because you can have both Release or Debug on both platforms and this doesn't serve the purpose. Reply by plucia on 07 October 2020, 15:26 > I actually don't need it anymore, so it's deleted now. Will > post the update soon Reply by Behrouz NematiPour on 07 October 2020, 16:50 > Please put "done" when the code is pushed. Reply by plucia on 07 October 2020, 17:11 > Done, I don't know why, but crucible created a duplicate > file > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16961] Reply by Behrouz NematiPour on 14 October 2020, 11:00 > RESOLVED ---------------------------------------- File: denali.qrc Revision Comment by Behrouz NematiPour on 30 September 2020, 17:07 https://devapps.diality.us/cru/UI-DEN-3605-4#c5065 I believe you noticed that all the images should start with 'i' and with no extension. Reply by plucia on 06 October 2020, 12:11 > Done - they are renamed and the extension has been removed. > Will post the update soon Reply by Behrouz NematiPour on 07 October 2020, 09:55 > RESOLVED ---------------------------------------- File: sources/applicationcontroller.h Revision Comment by pmontazemi on 25 August 2020, 10:50 https://devapps.diality.us/cru/UI-DEN-3605-4#c3878 Remove extra line. Reply by plucia on 28 August 2020, 13:23 > Done - see > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16971] Reply by pmontazemi on 28 August 2020, 17:04 > RESOLVED. ---------------------------------------- File: sources/canbus/messageglobals.h Revision Comment by pmontazemi on 25 August 2020, 10:13 https://devapps.diality.us/cru/UI-DEN-3605-4#c3855 Remove extra line. Reply by plucia on 28 August 2020, 11:59 > Done (see the other messageglobals.h file for the change) Reply by pmontazemi on 28 August 2020, 13:15 > RESOLVED. ---------------------------------------- File: sources/canbus/messageinterpreter.h Revision Comment by Behrouz NematiPour on 08 September 2020, 18:47 https://devapps.diality.us/cru/UI-DEN-3605-4#c4548 Regarding the latest changes on the master branch,a this file needs to change. Have you merged master into your working branch to resolve this? Reply by plucia on 09 September 2020, 12:26 > I have already merged master into the working branch, and > resolved all merge conflicts Reply by Behrouz NematiPour on 05 October 2020, 16:59 > RESOLVED Revision Comment by pmontazemi on 25 August 2020, 10:44 https://devapps.diality.us/cru/UI-DEN-3605-4#c3857 Remove extra line. Reply by plucia on 28 August 2020, 11:59 > Done (see the other messageinterpreter.h for the change) Reply by pmontazemi on 28 August 2020, 13:16 > RESOLVED. ---------------------------------------- File: sources/gui/guicontroller.h Revision Comment by pmontazemi on 25 August 2020, 10:46 https://devapps.diality.us/cru/UI-DEN-3605-4#c3861 Remove extra line. Reply by plucia on 28 August 2020, 13:54 > Done, See - > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16975] Reply by pmontazemi on 28 August 2020, 17:02 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:46 https://devapps.diality.us/cru/UI-DEN-3605-4#c3862 Remove extra line. Reply by plucia on 28 August 2020, 13:54 > Done - See > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16975] Reply by pmontazemi on 28 August 2020, 17:02 > RESOLVED. ---------------------------------------- File: sources/gui/guiglobals.h Revision Comment by Behrouz NematiPour on 08 September 2020, 18:57 https://devapps.diality.us/cru/UI-DEN-3605-4#c4549 I checked out to your story branch and this comment is not even there anymore. By the way, I think by now you know that it should be here to validate parameters with HD, right? Reply by plucia on 11 September 2020, 12:26 > Yes of course, this is a very old comment made early on > during development. See the newer version of this file, > guiglobals.h, in the files list to the left where this > comment is no longer present. That should match exactly with > the branch you checked out. Reply by Behrouz NematiPour on 30 September 2020, 19:38 > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 19:01 https://devapps.diality.us/cru/UI-DEN-3605-4#c4550 Do you have all the commits added to this code review? I don't see this comment either!!! Also, all the enum names are without ID_ which your latest push on your branch shows, they have !!! Reply by plucia on 09 September 2020, 11:19 > My understanding is that I have not excluded any commits > other than the merge commits that were requested to be > excluded previously. There are two guiglobals.h in this > review. Here's the link to the other one > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16977]. I > think it has the changes you're expecting to see. > In the file list to the left it is listed right above this > file. Reply by Behrouz NematiPour on 05 October 2020, 16:59 > will be reviewed later on the assigned code review. > RESOLVED ---------------------------------------- File: sources/gui/qml/components/CircleWithText.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:08 https://devapps.diality.us/cru/UI-DEN-3605-4#c5280 Please put a short description of your component and explain what it does and where it can be used. Since it is a specific or compound component and is not a general component like a just a Slider or Progressbar, it helps to understand the use case of the component. Reply by plucia on 06 October 2020, 17:45 > Done, will post update soon Reply by Behrouz NematiPour on 07 October 2020, 09:48 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/Slider.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:14 https://devapps.diality.us/cru/UI-DEN-3605-4#c5285 Please revert this back. It's not always an integer and as you may noticed value is the alias for the _progressRect.value and if you go inside the ProgressRect the value is real and not an integer. Please think about the rest of the code when changing a basic code. if you need to have an integer value in your end code format the value in your specific situation, Also the Slider component has a decimal attribute inherited from RangeRect, see if it's useful in you situation. Thanks, Reply by plucia on 06 October 2020, 17:26 > This change fixes a bug in the slider component when the step > (resolution) is set to 1. If a step of 1 is used the set > value improperly contains 14 decimals instead of being in > increments of 1. > Let me know once you've looked into it more and how you'd > prefer that issue to be fixed. Reply by Behrouz NematiPour on 07 October 2020, 10:13 > Please remove this fix and file a bug. Reply by plucia on 07 October 2020, 11:08 > Here's the bug: > > http://dvm-linux02:8080/browse/DEN-5281 > > Removed, will post update soon Reply by Behrouz NematiPour on 14 October 2020, 10:53 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/TopMenuBarCreateTreatment.qml Revision Comment by Behrouz NematiPour on 06 October 2020, 13:11 https://devapps.diality.us/cru/UI-DEN-3605-4#c5284 Please put a short description of your component and explain what it does and where it can be used. Since it is a specific or compound component and is not a general component like a just a Slider or Progressbar, it helps to understand the use case of the component. Reply by Dara Navaei on 19 October 2023, 11:34 > RESOLVED ---------------------------------------- File: sources/gui/qml/globals/Variables.qml Revision Comment by Behrouz NematiPour on 30 September 2020, 22:37 https://devapps.diality.us/cru/UI-DEN-3605-4#c5105 Please remove these since these are not used anymore. Reply by plucia on 06 October 2020, 11:07 > They are actually still being used for the png icon height > and width. I've renamed them accordingly Reply by Behrouz NematiPour on 06 October 2020, 14:12 > Found in another Variables.qml. > RESOLVED. Revision Comment by Behrouz NematiPour on 30 September 2020, 22:38 https://devapps.diality.us/cru/UI-DEN-3605-4#c5106 http://192.168.10.132:8060/cru/UI-DEN-3605-4#c5090 Reply by plucia on 06 October 2020, 11:01 > Done, see the other Variables.qml to the left. > http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16980 Reply by Behrouz NematiPour on 06 October 2020, 15:10 > Found in another Variables.qml. > RESOLVED. Revision Comment by Behrouz NematiPour on 30 September 2020, 22:40 https://devapps.diality.us/cru/UI-DEN-3605-4#c5107 Move these definitions before // --- PRS --- section. I also moved some of them in my code. Reply by plucia on 06 October 2020, 11:02 > Done, see the other Variables.qml to the left. > http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16980 Reply by Behrouz NematiPour on 06 October 2020, 14:16 > Found in another Variables.qml. > RESOLVED. ---------------------------------------- File: sources/gui/qml/main.qml Revision Comment by pmontazemi on 25 August 2020, 10:45 https://devapps.diality.us/cru/UI-DEN-3605-4#c3858 Remove extra line. Reply by plucia on 28 August 2020, 12:00 > Done - See the bottom of main.qml here > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16981] Reply by pmontazemi on 28 August 2020, 13:16 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/TreatmentBegin.qml Revision Comment by Behrouz NematiPour on 30 September 2020, 22:36 https://devapps.diality.us/cru/UI-DEN-3605-4#c5104 http://192.168.10.132:8060/cru/UI-DEN-3605-4#c5093 Reply by Behrouz NematiPour on 06 October 2020, 14:19 > Modification has been made. Will follow up on the changed > code. > RESOLVED Revision Comment by pmontazemi on 28 August 2020, 13:07 https://devapps.diality.us/cru/UI-DEN-3605-4#c4176 Remove 2 extra lines. Reply by plucia on 28 August 2020, 13:30 > Done Reply by pmontazemi on 28 August 2020, 16:59 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/TreatmentPrime.qml Revision Comment by Behrouz NematiPour on 30 September 2020, 19:56 https://devapps.diality.us/cru/UI-DEN-3605-4#c5086 Why these two libs has been imported? Reply by plucia on 05 October 2020, 11:08 > Controls is needed for the scrollbar. Looks like Layouts > isn't needed, so I've removed it > Will post the update soon Reply by Behrouz NematiPour on 05 October 2020, 15:16 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 20:40 https://devapps.diality.us/cru/UI-DEN-3605-4#c5092 put id: in front of parenthesis. Please apply this everywhere. Reply by plucia on 05 October 2020, 10:56 > Done, will post update soon Reply by Behrouz NematiPour on 05 October 2020, 15:14 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 20:39 https://devapps.diality.us/cru/UI-DEN-3605-4#c5091 why implicitHeight is used over the regualr width? Reply by plucia on 05 October 2020, 10:57 > When I tested initially with > {code}_column.height {code}, the flickable content height was > wrong. Using {code}_column.implicitHeight{code} fixed this > issue Reply by Behrouz NematiPour on 05 October 2020, 15:15 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 20:38 https://devapps.diality.us/cru/UI-DEN-3605-4#c5090 why added 'c' at the beginning of the name? None of the other constants in the Variable doesn't have it. Reply by plucia on 05 October 2020, 10:56 > Done, will post update to the review soon Reply by Behrouz NematiPour on 05 October 2020, 15:15 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 20:15 https://devapps.diality.us/cru/UI-DEN-3605-4#c5088 Just out of curiosity why used state instead of signal/slots? Reply by plucia on 05 October 2020, 10:55 > In the Qt docs, there's an example I probably based this on: > [https://doc.qt.io/qt-5.12/qtquick-customitems-scrollbar-main-qml.html] Reply by Behrouz NematiPour on 05 October 2020, 11:16 > Thanks for the link. > As you mentioned these are just example to make/teach some > points and not necessarily are good codding practices. > Please review twice to see if there is a better way. > I'm not a fan of using states in QML code if is not > absolutely necessary. Reply by plucia on 05 October 2020, 11:53 > This has been deleted as it's not longer needed with an > attached scrollbar Reply by Behrouz NematiPour on 05 October 2020, 15:07 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 20:49 https://devapps.diality.us/cru/UI-DEN-3605-4#c5093 Why didn't use the attached scrollbar instead of non-attached? regarding the Qt doc : When using a non-attached ScrollBar, the following must be done manually: *When using a non-attached ScrollBar, the following must be done manually: Layout the scroll bar (with the x and y or anchor properties, for example). Set the size and position properties to determine the size and position of the scroll bar in relation to the scrolled item. Set the active property to determine when the scroll bar will be visible.* your code could be as simple as : {code} Flickable { ... ScrollBar.vertical: ScrollBar { id: scrollBar } ... } {code} Reply by plucia on 05 October 2020, 11:51 > I've updated it to use an attached scrollbar. Just FYI, for > some reason the attached scrollbar doesn't hide automatically > when the content height is less than the height of the > screen. > Will post my update soon Reply by Behrouz NematiPour on 05 October 2020, 12:06 > Thanks, > I couldn't find a requirement for hiding the scrollbars and > also don't see any reason why we need to do that. > It's always good to see the scrollbars if the components > are not fit in the view port of the container, so the user > immediately will catch that there are more to take care of. Reply by Behrouz NematiPour on 05 October 2020, 15:06 > RESOLVED Revision Comment by Behrouz NematiPour on 30 September 2020, 20:17 https://devapps.diality.us/cru/UI-DEN-3605-4#c5089 Please always put the id: in front of the { to be able to find object easily. I saw this in other codes but since it was the first line after { I didn't put any comment. So please follow this rule every where. Reply by plucia on 05 October 2020, 10:52 > Done, will post the update to this review soon Reply by Behrouz NematiPour on 05 October 2020, 15:15 > RESOLVED Revision Comment by pmontazemi on 28 August 2020, 13:09 https://devapps.diality.us/cru/UI-DEN-3605-4#c4183 Remove extra line. Reply by plucia on 28 August 2020, 17:12 > Done Reply by pmontazemi on 01 September 2020, 09:17 > RESOLVED. Revision Comment by pmontazemi on 28 August 2020, 13:09 https://devapps.diality.us/cru/UI-DEN-3605-4#c4184 Remove 2 extra lines. Reply by plucia on 28 August 2020, 17:12 > Done Reply by pmontazemi on 01 September 2020, 09:17 > RESOLVED. ---------------------------------------- File: sources/model/mtreatmentparametersresp.h Revision Comment by pmontazemi on 25 August 2020, 10:47 https://devapps.diality.us/cru/UI-DEN-3605-4#c3863 Remove extra line. Reply by plucia on 28 August 2020, 12:01 > Done - See > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16960] Reply by pmontazemi on 28 August 2020, 17:03 > RESOLVED. Revision Comment by pmontazemi on 25 August 2020, 10:47 https://devapps.diality.us/cru/UI-DEN-3605-4#c3864 Remove extra line. Reply by plucia on 28 August 2020, 12:01 > Done - See > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16960] Reply by pmontazemi on 28 August 2020, 17:03 > RESOLVED. ---------------------------------------- File: sources/storage/filehandler.h Revision Comment by pmontazemi on 25 August 2020, 10:47 https://devapps.diality.us/cru/UI-DEN-3605-4#c3866 Remove extra line. Reply by plucia on 28 August 2020, 12:01 > Done - See > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16985] Reply by pmontazemi on 28 August 2020, 17:03 > RESOLVED. ---------------------------------------- File: sources/view/vview.h Revision Comment by pmontazemi on 25 August 2020, 10:50 https://devapps.diality.us/cru/UI-DEN-3605-4#c3876 Remove extra line. Reply by plucia on 28 August 2020, 13:22 > Done, see > [http://dvm-linux02:8060/cru/UI-DEN-3605-4#CFR-16986] Reply by pmontazemi on 28 August 2020, 17:04 > RESOLVED. ---------------------------------------- File: sources/model/MTreatmentParameters.cpp Revision Comment by Behrouz NematiPour on 08 September 2020, 21:32 https://devapps.diality.us/cru/UI-DEN-3605-4#c4567 Please implement models like many other model examples that currently exist in the code. This is not an expected implementation. Reply by plucia on 09 September 2020, 12:20 > Are there any other changes you're requesting other than the > comments below? Reply by plucia on 14 September 2020, 14:58 > As per our conversation today, due to the design changes > with MAbstract.h and MModel.h that were merged to master on > Aug. 31st, the create treatment models will be updated to > subclass these parent classes. > This work is going to be tracked in this ticket: > [http://dvm-linux02:8080/browse/DEN-4981] Reply by Behrouz NematiPour on 30 September 2020, 18:32 > If it's going to be addressed that's fine. > RESOLVED. Revision Comment by Behrouz NematiPour on 08 September 2020, 19:04 https://devapps.diality.us/cru/UI-DEN-3605-4#c4551 Please don't use %0. It has not been documented in Qt, therefore the behavior is unpredicted. Also, it's always good to start from 1 in this case and I see no reason to do that? Reply by plucia on 09 September 2020, 12:45 > Done Reply by Behrouz NematiPour on 05 October 2020, 16:57 > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 21:30 https://devapps.diality.us/cru/UI-DEN-3605-4#c4566 Please follow the code structure for consistency. All the Model classes are checking each of the values to be convertible by GetValue. Why this check has been ignored here. Please notice that this is bytes conversion over CANBus you can receive any junk data and if you don't check the conversion correctly the you are showing incorrect data. Reply by plucia on 09 September 2020, 12:30 > This model is for a request, so we're not parsing data from > CAN, only sending it out. > There's no implementation of GetValue that works with > QVariants. In other places in the code it's only used with > QByteArrays. Reply by plucia on 14 September 2020, 16:18 > As per our conversation today, due to the design changes with > MAbstract.h and MModel.h that were merged to master on Aug. > 31st, the create treatment models will be updated to subclass > these parent classes. > This work is going to be tracked in this ticket: > http://dvm-linux02:8080/browse/DEN-4981 Reply by Behrouz NematiPour on 30 September 2020, 19:13 > fine then. > RESOLVED. ---------------------------------------- File: sources/model/MTreatmentParameters.h Revision Comment by Behrouz NematiPour on 08 September 2020, 21:05 https://devapps.diality.us/cru/UI-DEN-3605-4#c4560 Please follow the code structure for consistency. Why this class doesn't start with M like all the other Model classes? Reply by plucia on 09 September 2020, 16:46 > I've updated the class name now to MTreatmentParameters. Reply by Behrouz NematiPour on 05 October 2020, 17:02 > RESOLVED. Revision Comment by Behrouz NematiPour on 08 September 2020, 21:07 https://devapps.diality.us/cru/UI-DEN-3605-4#c4561 Please inherit from MAbstract. Reply by plucia on 09 September 2020, 17:18 > I looked into subclassing MAbstract at the time and again > today but there's some incompatibility there that needs to be > worked through. > How about I work on subclassing MAbstract in my current > story? I don't think it would be good to change too much > structure against these revisions as the code has been > sitting for awhile and other things have changed quite a bit. > Also, I think the MAbstract class may have been added near > the end of my development on DEN-3605. > I'd like to make sure our models and code structure are all > aligned too. I think it's better to do that on the latest > code rather than the branch this code is sitting on which is > outdated at this point. Reply by plucia on 14 September 2020, 16:19 > As per our conversation today, due to the design changes with > MAbstract.h and MModel.h that were merged to master on Aug. > 31st, the create treatment models will be updated to subclass > these parent classes. > This work is going to be tracked in this ticket: > http://dvm-linux02:8080/browse/DEN-4981 Reply by Behrouz NematiPour on 05 October 2020, 16:41 > will be reviewed later on the assigned code review. > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 21:14 https://devapps.diality.us/cru/UI-DEN-3605-4#c4562 Why these parameters don't match with our "Message List" document? Not even the order but also the types? It's really hard to follow! Reply by plucia on 09 September 2020, 16:37 > This code review started > 2 weeks ago and since then the > message list document was updated as per Sean's requested > changes which I've been working on in a separate branch and > story. Reply by Dara Navaei on 19 October 2023, 11:34 > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 21:16 https://devapps.diality.us/cru/UI-DEN-3605-4#c4564 Please, align the variable names vertically to easier follow the code. Reply by plucia on 09 September 2020, 12:46 > Done Reply by Behrouz NematiPour on 30 September 2020, 19:23 > RESOLVED Revision Comment by Behrouz NematiPour on 08 September 2020, 21:19 https://devapps.diality.us/cru/UI-DEN-3605-4#c4565 Please follow the code structure for consistency. The naming like all the other Model classes should be : typedef Model::::Data Data; The chosen name is not consistent and also is too short and doesn't show the content type. In this case, it should also have Adjust at the beginning as well, if I understood the class purpose correctly. Reply by plucia on 09 September 2020, 14:16 > I renamed it to TreatmentParametersData in line with the > paradigm you've described: > > {code} typedef Model::::Data Data; {code} > It's not adjusting a parameter during a treatment, so I don't > think the Adjust naming would apply in this particular case. Reply by plucia on 30 September 2020, 13:33 > Update, it's now MAdjustTreatmentParametersRequest and > AdjustTreatmentParametersRequestData as per our last > conversation Reply by Behrouz NematiPour on 05 October 2020, 16:33 > typedef Model::TreatmentParameters::Data TreatmentData; > to > typedef Model::MTreatmentParameters::Data > TreatmentParametersData; Reply by plucia on 07 October 2020, 10:49 > See 5387db3 after dragging the slider to the right on > the top > > Previously you requested I rename this to > MAdjustTreatmentParametersRequest, which is what I > changed it to in 5387db3. > > Which do you prefer, MTreatmentParameters or is > MAdjustTreatmentParametersRequest? Reply by Behrouz NematiPour on 07 October 2020, 16:58 > We talked about it in code review meetings. > Please do what has been told. Reply by plucia on 07 October 2020, 17:17 > I think you forgot what you told me previously and > you're going to tell me to change it again if I don't > remind you of why you told me to rename it to > MAdjustTreatmentParametersRequest in the first place. > > Previously, you complained that there was no Adjust > prefix and no RequestData suffix, so I've since > updated it to match that. > > For example: > > {code} > REGISTER_METATYPE( > AdjustUltrafiltrationConfirmRequestData ) \ > REGISTER_METATYPE( > AdjustSalineRequestData ) \ > REGISTER_METATYPE( > AdjustTreatmentParametersRequestData ) \ > {code} > > So you want me to rename it to this now? > > {code} > REGISTER_METATYPE( > AdjustUltrafiltrationConfirmRequestData ) \ > REGISTER_METATYPE( > AdjustSalineRequestData ) \ > REGISTER_METATYPE( > TreatmentParametersData ) \ > {code} > > Please choose. I've already updated it to match the > first so it's like the other code. Reply by Behrouz NematiPour on 13 October 2020, 17:15 > It's not about what I want, it's about code > consistency. > The comment I put up there was regarding a > conversation we had about Data at the end and how > Data is going to be in the naming and that time I > didn't care about the specific Model and Treatment > was an example. > I don't think I don't even need to tell you want it > should look like as you put your code there it needs > to be like the other ones next to it. > So please just be consistent. Reply by plucia on 14 October 2020, 09:14 > Okay so in that case I need to guess about what > you're asking for. My guess is you want it to be > named to AdjustTreatmentParametersRequestData. > This is how it's currently named. > Please indicate if you prefer otherwise or mark > resolved. Reply by Behrouz NematiPour on 15 October 2020, 10:57 > RESOLVED > Will be reviewed in later stories. ---------------------------------------- File: sources/model/MTreatmentParametersResp.cpp Revision Comment by Behrouz NematiPour on 08 September 2020, 21:33 https://devapps.diality.us/cru/UI-DEN-3605-4#c4568 Please implement models like many other model examples that currently exist in the code. This is not an expected implementation. Reply by plucia on 09 September 2020, 11:26 > These are the list of some models that currently also have > the method, toVariantList > {code} > MTreatmentAdjustUltrafiltrationEditResponse > MAlarmCleared > MAlarmStatusData > MAlarmTriggered > {code} > > Some existing models that also have fromByteArray > {code} > MAbstract > MDGDebugText > MHDDebugText > MPowerOff > MDGDrainPumpData > MDGHeatersData > MDGLoadCellReadingsData > MDGOperationModeData > MDGPressuresData > MDGROPumpData > MDGReservoirData > MDGTemperaturesData > MDGValvesStatesData > MAlarmCleared > MAlarmStatusData > MHDOperationModeData > {code} > > Some existing models that have the data() method: > {code} > MDGHeaters > MDGLoadCellReadings > MDGReservoir > MDGROPump > MAdjustSalineResponse > MAdjustUltrafiltrationConfirmResponse > {code} > > Some existing models that also have the toString() method: > {code} > MAdjustUltrafiltrationConfirmResponse > MAdjustUltrafiltrationEditREsponse > MAlarmCleared > MAlarmStatusData > MAlarmTriggered > MAbstract > {code} > > fromVariantList is used to process a FW validation response > from a QVariantList to the TreatmentParametersRespData. Did > you think something should change in this function? > > If not, could you be more specific about what is not > expected? Reply by plucia on 14 September 2020, 14:56 > As per our conversation today, due to the design changes > with MAbstract.h and MModel.h that were merged to master on > Aug. 31st, the create treatment models will be updated to > subclass these parent classes. > This work is going to be tracked in this ticket: > [http://dvm-linux02:8080/browse/DEN-4981] Reply by Behrouz NematiPour on 30 September 2020, 19:25 > Fine then, > RESOLVED ---------------------------------------- File: sources/model/MTreatmentParametersResp.h Revision Comment by Behrouz NematiPour on 08 September 2020, 21:15 https://devapps.diality.us/cru/UI-DEN-3605-4#c4563 Why these parameters don't match with our "Message List" document? Not even the order but also the types ? Reply by plucia on 09 September 2020, 12:05 > All the treatment parameter response types are unsigned int. > I think you may be looking at the Create Treatment Request in > the message lists, not the response. > There are 18 parameters listed here. There are 18 parameters > listed in Message Lists.xlsx. > Going through each one, one by one: > > 1. Request Valid 0=No 1=Yes -> requestValid > 2. Blood Flow Rate Response -> bloodFlowRate > 3. Dialysate Flow Rate Response -> dialysateFlowRate > 4. Duration Response -> duration > 5. Heparin Stop Time Response -> heparinStopTime > 6. Saline Bolus Response -> salineBolus > 7. Acid Concentrate -> acidConcentrate > 8. Bicarbonate Concentrate Response -> bicarbonateConcentrate > 9. Dialyzer Type Response -> dialyzerType > 10. Blood Pressure Measure Interval Response -> > bloodPressureMeasureInterval > 11. Rinseback Flow Rate Response -> rinsebackFlowRate > 12. Arterial Pressure Limit Low Response -> > arterialPressureLimitLow > 13. Arterial Pressure Limit High Response -> > arterialPressureLimitHigh > 14. Venous Pressure Limit Low Response -> > venousPressureLimitLow > 15. Venous Pressure Limit High Presonse -> > venousPressureLimitHigh > 16. Heparin Dispensing Rate Response -> heparinDispensingRate > 17. Heparin Bolus Volume Response -> heparinBolusVolume > 18. Dialysate Temperature Response -> dialysateTemp > > I don't see any missing. > > The order of the parameters was requested to be changed after > the sprint in which this particular implementation was done. > That ordering of the parameters change has already been > implemented in a newer story and branch which will be going > through code review soon. Therefore, the order of the > parameters you see here is reflective of the message lists as > it existed at the time this story was being worked on. Reply by Behrouz NematiPour on 30 September 2020, 19:29 > The order is so important and that the order of receiving > the information over the CANBus which going to be used the > same order everywhere even in Models and Views. > To be clarified, please confirm. > 1 - The order is changed and has been implemented correctly > in the recent modification of the class. > 2 - I expect to see later in a code review the correct > order as has been defined in the "message list" excel sheet > as below : > > 54, 0x020, 6, Rsp, Y, HD, UI, "Validate New Create > Treatment Response" > 01 - (U32)-0=No, 1=Yes > 02 - (U32) Blood Flow Rate Response > 03 - (U32) Dialysate Flow Rate Response > 04 - (U32) Duration Response > 05 - (U32) Heparin Stop Time Response > 06 - (U32) Saline Bolus Response > 07 - (U32) Acid Concentrate Response > 08 - (U32) Bicarbonate Concentrate Response > 09 - (U32) Dialyzer Type Response > 10 - (U32) Blood Pressure Measure Interval Response > 11 - (U32) Rinseback Flow Rate Response > 12 - (U32) Arterial Pressure Limit Low Response > 13 - (U32) Arterial Pressure Limit High Response > 14 - (U32) Venous Pressure Limit Low Response > 15 - (U32) Venous Pressure Limit High Response > 16 - (U32) Heparin Dispensing Rate Response > 17 - (U32) Heparin Bolus Volume Response > 18 - (U32) Dialysate Temperature Response Reply by plucia on 14 October 2020, 13:13 > Done Reply by Behrouz NematiPour on 15 October 2020, 10:42 > RESOLVED --- ID: UI-DEN-3605-4 https://devapps.diality.us/cru/UI-DEN-3605-4 Title: UI Create Treatment Statement of Objectives: State: Closed Summary: Author: plucia Moderator: plucia Reviewers: (0 active, 2 completed*) Behrouz NematiPour (*) pmontazemi (*)