This is a list of all comments for UI-DEN-3149-1. Review Summary: No summary ---------------------------------------- File: sources/gui/qml/components/UltrafiltrationButton.qml Revision Comment by pmontazemi on 19 June 2020, 08:48 https://devapps.diality.us/cru/UI-DEN-3149-1#c2423 Remove extra line. Reply by Behrouz NematiPour on 22 June 2020, 12:46 > done Reply by pmontazemi on 23 June 2020, 16:43 > RESOLVED. Revision Comment by pmontazemi on 19 June 2020, 08:48 https://devapps.diality.us/cru/UI-DEN-3149-1#c2424 Remove extra line. Reply by Behrouz NematiPour on 22 June 2020, 12:46 > done Reply by pmontazemi on 23 June 2020, 16:43 > RESOLVED. ---------------------------------------- File: denali.pro.user Revision Comment by plucia on 10 June 2020, 13:21 https://devapps.diality.us/cru/UI-DEN-3149-1#c2219 Is it not possible to remove .user file from git tracking? Reply by plucia on 11 June 2020, 09:37 > Talked with Behrouz. RESOLVED. ---------------------------------------- File: sources/applicationcontroller.h Revision Comment by plucia on 11 June 2020, 17:23 https://devapps.diality.us/cru/UI-DEN-3149-1#c2290 What is the purpose of this comment? Reply by Behrouz NematiPour on 11 June 2020, 20:06 > this not a code comment. > It's like a section separator to me. Reply by Behrouz NematiPour on 12 June 2020, 13:12 > Put informtion Reply by plucia on 15 June 2020, 09:23 > RESOLVED. ---------------------------------------- File: sources/canbus/messagedispatcher.cpp Revision Comment by pmontazemi on 11 June 2020, 09:20 https://devapps.diality.us/cru/UI-DEN-3149-1#c2224 spelling: duration Reply by Behrouz NematiPour on 12 June 2020, 13:09 > Thanks for catching that. > Corrected. Reply by pmontazemi on 12 June 2020, 13:23 > RESOLVED. ---------------------------------------- File: sources/canbus/messageglobals.h Revision Comment by pmontazemi on 12 June 2020, 13:24 https://devapps.diality.us/cru/UI-DEN-3149-1#c2318 Spelling: TemperatureSensors Reply by Behrouz NematiPour on 14 June 2020, 19:40 > Thanks for catching again. > The spellchecker has been stopped working and I can't make it > run again. > Corrected. Reply by pmontazemi on 17 June 2020, 08:50 > Please work on getting it to work again. Reply by pmontazemi on 17 June 2020, 08:50 > RESOLVED. ---------------------------------------- File: sources/canbus/messageinterpreter.cpp Revision Comment by pmontazemi on 11 June 2020, 13:32 https://devapps.diality.us/cru/UI-DEN-3149-1#c2228 canBusFaultCountData? Reply by Behrouz NematiPour on 11 June 2020, 16:46 > That's what I have for EMC test. > I would like to keep it so as I mentioned in the task for EMC > support I pushed the code here to be reviewed and kept. Reply by pmontazemi on 11 June 2020, 17:00 > RESOLVED. Revision Comment by pmontazemi on 11 June 2020, 13:33 https://devapps.diality.us/cru/UI-DEN-3149-1#c2229 Why space? Reply by Behrouz NematiPour on 11 June 2020, 16:48 > Sometimes it's easier to read for me. Reply by pmontazemi on 11 June 2020, 17:00 > I believe it should be consistent throughout the code. It > makes the review of it easier for everyone. Ultimately, > this should be driven by the C++ coding standard. Reply by Behrouz NematiPour on 12 June 2020, 13:10 > Removed. Reply by pmontazemi on 12 June 2020, 13:23 > RESOLVED. ---------------------------------------- File: sources/canbus/messageinterpreter.h Revision Comment by pmontazemi on 11 June 2020, 13:34 https://devapps.diality.us/cru/UI-DEN-3149-1#c2230 canBusFaultCountData? Reply by Behrouz NematiPour on 11 June 2020, 16:48 > explained in another comment. Reply by pmontazemi on 12 June 2020, 08:11 > RESOLVED. Revision Comment by pmontazemi on 12 June 2020, 13:24 https://devapps.diality.us/cru/UI-DEN-3149-1#c2319 Spelling: temperatureSensorsData Reply by Behrouz NematiPour on 14 June 2020, 19:39 > Thanks for catching again. > The spellchecker has been stopped working and I can't make it > run again. > Corrected. Reply by pmontazemi on 17 June 2020, 08:51 > RESOLVED. ---------------------------------------- File: sources/gui/guicontroller.h Revision Comment by pmontazemi on 11 June 2020, 13:38 https://devapps.diality.us/cru/UI-DEN-3149-1#c2236 What is this comment for? Reply by Behrouz NematiPour on 11 June 2020, 16:50 > To find the section easier. > Something like a line. > not a comment thought. Reply by pmontazemi on 11 June 2020, 16:58 > What is so special about this line that it needs this > comment? And, what guarantees that there are not going be > other lines in the future that also need to be found? I > suggest finding a better method than adding comments in the > code. Reply by Behrouz NematiPour on 11 June 2020, 17:10 > It's not a comment. > I am just using // in my codes. Reply by pmontazemi on 12 June 2020, 08:10 > This is not part of our coding standard, please remove. Reply by Behrouz NematiPour on 12 June 2020, 13:11 > put information Reply by pmontazemi on 12 June 2020, 13:22 > RESOLVED. ---------------------------------------- File: sources/gui/guiglobals.h Revision Comment by pmontazemi on 12 June 2020, 13:25 https://devapps.diality.us/cru/UI-DEN-3149-1#c2320 Spelling: TemperatureSensors Reply by Behrouz NematiPour on 14 June 2020, 19:37 > Thanks for catching again. > The spellchecker has been stopped working and I can't make it > run again. > Corrected. Reply by pmontazemi on 17 June 2020, 08:57 > RESOLVED. ---------------------------------------- File: sources/view/vtreatmentultrafiltration.h Revision Comment by pmontazemi on 11 June 2020, 13:41 https://devapps.diality.us/cru/UI-DEN-3149-1#c2237 spelling of "don't" for subject we and ",": "... , which we currently don't have." Reply by Behrouz NematiPour on 12 June 2020, 13:11 > Thanks again for catching that, > Corrected. Reply by pmontazemi on 12 June 2020, 13:22 > RESOLVED. ---------------------------------------- File: sources/gui/qml/components/TouchRect.qml Revision Comment by plucia on 15 June 2020, 11:40 https://devapps.diality.us/cru/UI-DEN-3149-1#c2377 Semicolon Reply by Dara Navaei on 19 October 2023, 10:52 > RESOLVED ---------------------------------------- File: sources/model/mtreatmentadjustultrafiltrationstateresponse.cpp Revision Comment by plucia on 11 June 2020, 17:18 https://devapps.diality.us/cru/UI-DEN-3149-1#c2288 thisfilenameisquitelongandhardtoreaditwouldbebetterifitwascamelcasewhichIthinkyouarealreadygoingtodowitheveryfile,right? Reply by Behrouz NematiPour on 11 June 2020, 20:04 > yesyouarerightiamamgoingtodothatanywaybecauseitissohardtomeaswellanditislikethisbecauseitwasthedefaultqtcreatoroptionandididnotchangeitatthattimebutineedatimetochangeitassoonaspossible. Reply by pmontazemi on 23 June 2020, 17:26 > I had the same comment, should we change our C++ coding > standard, too? Behrouz mentioned that this is the > recommended way. I still believe this is really hard to > read and against other coding standard I have worked in the > past. Reply by Behrouz NematiPour on 23 June 2020, 17:29 > I'll absolutely change this default setting and will > definitely change the files naming. Reply by Dara Navaei on 19 October 2023, 10:53 > RESOLVED ---------------------------------------- File: sources/model/mtreatmentadjustultrafiltrationstateresponse.h Revision Comment by pmontazemi on 23 June 2020, 16:44 https://devapps.diality.us/cru/UI-DEN-3149-1#c2546 1. Response (instead of responce, also fix spell checker); 2. accepted (instead of accpted). Reply by pmontazemi on 23 June 2020, 17:24 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/adjustments/TreatmentAdjustmentBase.qml Revision Comment by plucia on 15 June 2020, 11:37 https://devapps.diality.us/cru/UI-DEN-3149-1#c2374 Do you need a semicolon here? Reply by Behrouz NematiPour on 15 June 2020, 13:10 > Thanks Peter for catching that. > Removed. Reply by plucia on 16 June 2020, 09:26 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/treatment/adjustments/TreatmentAdjustmentFlow.qml Revision Comment by plucia on 15 June 2020, 11:38 https://devapps.diality.us/cru/UI-DEN-3149-1#c2375 The semicolon usage is difference between these two functions. Shouldn't we not have semicolons in QML? Reply by Behrouz NematiPour on 15 June 2020, 13:11 > Thanks Peter for catching that. > Removed. Reply by plucia on 16 June 2020, 09:27 > RESOLVED Revision Comment by plucia on 15 June 2020, 11:38 https://devapps.diality.us/cru/UI-DEN-3149-1#c2376 Semicolon Reply by Behrouz NematiPour on 15 June 2020, 13:11 > Thanks Peter for catching that. > Removed. Reply by plucia on 16 June 2020, 09:27 > RESOLVED ---------------------------------------- File: sources/gui/guicontroller.cpp Revision Comment by pmontazemi on 11 June 2020, 13:35 https://devapps.diality.us/cru/UI-DEN-3149-1#c2231 What is this comment for? Reply by Behrouz NematiPour on 11 June 2020, 16:49 > Just a separator from the other part of the codes to find > that section easier. Reply by pmontazemi on 12 June 2020, 08:10 > Refer to my other comment on this type of extra comments. Reply by Behrouz NematiPour on 12 June 2020, 13:10 > put information Reply by pmontazemi on 12 June 2020, 13:23 > RESOLVED. ---------------------------------------- File: sources/main.h Revision Comment by plucia on 11 June 2020, 17:22 https://devapps.diality.us/cru/UI-DEN-3149-1#c2289 How many classes will use the new macros? Using too many macros to generate code (e.g. the signal slot connections) obfuscates the connections between classes. It adds many layers of indirection where the intent of the code is no longer clear since it is generated. I really don't see the benefit of adding even more. Just define these in their respective classes so when you view those classes you don't have to come here and deduce the written code. Reply by Behrouz NematiPour on 14 June 2020, 19:42 > The benefit of this defines are to guard the code. > It has been defined to make developers to only use the code > the way it has been defined. > Since the QML can't use template classes they have been > defined with the #define. > Please only use the defines in correct way to protect the > code. Reply by plucia on 15 June 2020, 09:24 > I took a look at the current C++ coding standard, here: > X:\Engineering\Denali\06- Software Design\Software > Documentation\Coding Standards, but the section on Macros > is lacking. Therefore, I consulting an outside coding > standard, called C++ Coding Standards 101 Rules, > Guidelines, and Best Practices by Herb Sutter and Andrei > Alexandrescu. Chapter 16 of this book is entitled “Avoid > macros.” There is a quote in this book from Bjarne > Stroustrup, creator of the C++ programming language: > > “Macros are almost never necessary in C++…” > “…The first rule about macros is: Don’t use them unless you > have to. Almost every macro demonstrates a flaw in the > programming language, in the program or in the programmer.” > > Additionally, the authors of the book have these comments > on macros: > > “An error in a macro can only be reported after the macro > is expanded and not when it is defined.” > “Always given them SCREAMING_UPPERCASE_AND_UGLY names, and > avoid putting them in headers” > > If the creator of C++ says don’t use macros unless you have > to, I think we should use them sparingly. Each time one of > these macros is used in a .h or .cpp file, you have to come > back to main.h and deduce the generated code from 2+ layers > of macro indirection. This I am sure is very hurtful to the > readability of the code. Our coding standard encourages > readability, and so I think it is in the spirit of it to > limit the use of macros as much as possible. Reply by Behrouz NematiPour on 15 June 2020, 10:02 > Thanks Peter for your consult. > > This is the link which is used for our C++ coding > standard and has been already mentioned as a reference in > the doc. > This codding standard and this implementation has been > done by having those comments in mind, > > There are so many debates on so many c++ features that > might be the case people are prefer to use Java or python > sometimes. > For example threads are strongly suggested to not to be > used until you really need them because they are very > hard (or almost impossible) to debug and needs so much > work to keep them safe. > But we are using them because we need them. > > I put those #define in code to guard the code and I'm > willing to keep them at least for quite a while until we > are going to refactor them and implement the actual slots > when required. > So until then we are not removing them and it remains as > it is. > > If it's still not clear to you how to use them after I > explained, come to me and ask please. > > Thanks, Reply by plucia on 15 June 2020, 10:14 > Thanks Behrouz, > > Almost always you can write an ordinary function > instead of a macro. The macros are not necessary. They > attract bugs, make the code more complicated to debug, > and can cause multiple false positives of static code > analyzers since they cannot differentiate correct sly > code from erroneous code. > > I am simply suggesting to limit their usage as much as > possible. You have 24 macros now in this file. What is > the attachment to using them? > > We all want to guard the code. Moreover, it's equally > important that we write safe code that is clear to > debug. Macros put into jeopardy the safety of our code > because they make debugging much more difficult. Reply by Behrouz NematiPour on 15 June 2020, 10:39 > Thanks Peter for your great suggestions. > > These macros keeping the code simple to avoid > multiple repetitive codes which are almost the same > and have only one parameters different. > Those are mostly Qt related Signal/Slot definitions > which are not caught by any static code analyzer and > are not anything that they can suggest to change any > other way than Qt documented. > So other usage of this macros are to simplify the > code. > I still would like to keep them and don't want to > remove them until the time comes. > thanks, Reply by plucia on 15 June 2020, 12:51 > The macros do not simplify the code. They make > debugging much more difficult. > > What would be a situation in which you think they > should be removed? Reply by plucia on 17 June 2020, 13:13 > Future changes will be tracked in: > > http://dvm-linux02:8080/browse/DEN-3743 > > RESOLVED. ---------------------------------------- File: main.cpp Revision Comment by pmontazemi on 19 June 2020, 08:51 https://devapps.diality.us/cru/UI-DEN-3149-1#c2425 Where in the code it shows that we are actually sending a non-zero value to disable this patch? I believe we should completely remove this functionality when the timing is right. Reply by Behrouz NematiPour on 22 June 2020, 08:59 > I agree. > When time comes this needs to be removed. > I wasn't even sure that it was a solution. > It's in frameInterface@290 which is not in code review since > had no change. Reply by pmontazemi on 23 June 2020, 17:06 > RESOLVED. ---------------------------------------- File: unittests/tst_views.cpp Revision Comment by pmontazemi on 23 June 2020, 16:53 https://devapps.diality.us/cru/UI-DEN-3149-1#c2554 regarding Reply by pmontazemi on 23 June 2020, 17:23 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:53 https://devapps.diality.us/cru/UI-DEN-3149-1#c2555 If each word starts with capital letter make consistent, other first letter of first word can be capital and all the rest lower cap. Reply by Behrouz NematiPour on 23 June 2020, 17:00 > Unable to Resume Ultrafiltration or already running > U for the first word and "Resume Ultrafiltration" as a term > for the current action. Reply by pmontazemi on 23 June 2020, 17:24 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:54 https://devapps.diality.us/cru/UI-DEN-3149-1#c2556 Regarding Reply by pmontazemi on 23 June 2020, 17:24 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:54 https://devapps.diality.us/cru/UI-DEN-3149-1#c2557 Same comment here. Reply by Behrouz NematiPour on 23 June 2020, 17:01 > Same as "Resume Ultrafiltration". Reply by pmontazemi on 23 June 2020, 17:24 > RESOLVED. ---------------------------------------- File: todos Revision Comment by pmontazemi on 11 June 2020, 13:42 https://devapps.diality.us/cru/UI-DEN-3149-1#c2238 Typically, TODOs are handled by searching the entire codebase for the keyword "TODO" and list shall be reduced to 0 count before any formal release. What is this additional list of TODOs for? Reply by Behrouz NematiPour on 11 June 2020, 16:51 > it's my general personal comments and todos like what we have > as general comments in crucible. > just as a reminder for me. Reply by pmontazemi on 11 June 2020, 16:57 > I would keep them outside the code base. Reply by Behrouz NematiPour on 11 June 2020, 17:08 > It was, I just added them to repo to keep track of them. Reply by pmontazemi on 12 June 2020, 08:07 > Please remove, only files that are needed for the IDEs > and builds should be part of the repos to keep them > clean. You can use OneNoe, X;/ drive, OneDrive, and > your local drive for storing personal TODO lists. The > other option is to make TODO comments in the code. Reply by Behrouz NematiPour on 12 June 2020, 13:13 > Comment removed on appropriate place and the file > itself has been removed. Reply by pmontazemi on 12 June 2020, 13:22 > RESOLVED. ---------------------------------------- File: unittests/tst_messaging.h Revision Comment by pmontazemi on 23 June 2020, 16:49 https://devapps.diality.us/cru/UI-DEN-3149-1#c2548 Remove extra lines. Reply by pmontazemi on 23 June 2020, 17:24 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:50 https://devapps.diality.us/cru/UI-DEN-3149-1#c2549 Remove extra line. Reply by pmontazemi on 23 June 2020, 17:25 > RESOLVED. ---------------------------------------- File: unittests/tst_models.cpp Revision Comment by pmontazemi on 23 June 2020, 16:50 https://devapps.diality.us/cru/UI-DEN-3149-1#c2550 msg complete (instead of msg compelete) Reply by pmontazemi on 23 June 2020, 17:25 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:51 https://devapps.diality.us/cru/UI-DEN-3149-1#c2551 msg compelete? Reply by pmontazemi on 23 June 2020, 17:25 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:51 https://devapps.diality.us/cru/UI-DEN-3149-1#c2552 msg complete Reply by pmontazemi on 23 June 2020, 17:23 > RESOLVED. Revision Comment by pmontazemi on 23 June 2020, 16:51 https://devapps.diality.us/cru/UI-DEN-3149-1#c2553 msg complete Reply by pmontazemi on 23 June 2020, 17:25 > RESOLVED. ---------------------------------------- File: unittests/tst_canbus.cpp Revision Comment by pmontazemi on 23 June 2020, 16:47 https://devapps.diality.us/cru/UI-DEN-3149-1#c2547 removeFirst (instead of removeFist)? Reply by pmontazemi on 23 June 2020, 17:23 > RESOLVED. --- ID: UI-DEN-3149-1 https://devapps.diality.us/cru/UI-DEN-3149-1 Title: UI-DEN-3149_Treatment Ultrafiltration Adjustment Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Moderator: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)