This is a list of all comments for UI-DEN-7135-1. Review Summary: No summary ---------------------------------------- File: denali.pro Revision Comment by plucia on 05 April 2021, 09:48 https://devapps.diality.us/cru/UI-DEN-7135-1#c8990 I added these already to staging, but they are named BUILD_FOR_DESKTOP and BUILD_FOR_TARGET. So when you merge staging to your branch or vice versa be sure to rename all of mine or make sure all of them are named the same since there's no need for duplicates. Also, are we still going to review merges as I don't think there is a merge commit yet from staging to this branch in this code review. Reply by Behrouz NematiPour on 05 April 2021, 10:00 > Thanks for reminding me that, Yes I saw that. > I Will taking care of it. Reply by plucia on 07 April 2021, 09:49 > Thanks. Are we going to need to review your merge commits > as well? Reply by Behrouz NematiPour on 09 April 2021, 22:10 > If needed, sure. > Every commit has to be reviewed otherwise the bamboo > scripts will not let the build happen. Reply by plucia on 12 April 2021, 10:05 > RESOLVED ---------------------------------------- File: resources/settings/instructions/1.png Revision Comment by plucia on 05 April 2021, 09:27 https://devapps.diality.us/cru/UI-DEN-7135-1#c8987 This image filename could be more descriptive. Will it have to be renamed if the order is changed? Reply by Behrouz NematiPour on 05 April 2021, 09:51 > These are test files for now. > Later when we have a real file designed these will be > replaced with some real names. Reply by plucia on 07 April 2021, 09:52 > RESOLVED ---------------------------------------- File: resources/settings/instructions/readme.md Revision Comment by pmontazemi on 01 April 2021, 09:24 https://devapps.diality.us/cru/UI-DEN-7135-1#c8901 This...required...application...instruction-based. Reply by Behrouz NematiPour on 09 April 2021, 22:22 > done Reply by pmontazemi on 10 April 2021, 15:27 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:23 https://devapps.diality.us/cru/UI-DEN-7135-1#c8899 It..., for application... Reply by Behrouz NematiPour on 09 April 2021, 22:22 > done Reply by pmontazemi on 10 April 2021, 15:28 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:25 https://devapps.diality.us/cru/UI-DEN-7135-1#c8902 When...VM, (comma after VM)...the application Reply by Behrouz NematiPour on 09 April 2021, 22:22 > done Reply by pmontazemi on 10 April 2021, 15:28 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:25 https://devapps.diality.us/cru/UI-DEN-7135-1#c8904 , the application Reply by Behrouz NematiPour on 09 April 2021, 22:22 > done Reply by pmontazemi on 10 April 2021, 15:28 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:25 https://devapps.diality.us/cru/UI-DEN-7135-1#c8905 ...that has...for each instruction-based... Reply by Behrouz NematiPour on 09 April 2021, 22:22 > done Reply by pmontazemi on 10 April 2021, 15:28 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:26 https://devapps.diality.us/cru/UI-DEN-7135-1#c8906 ...instruction-based... Reply by Behrouz NematiPour on 09 April 2021, 22:22 > done Reply by pmontazemi on 10 April 2021, 15:28 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:26 https://devapps.diality.us/cru/UI-DEN-7135-1#c8907 ...each instruction... Reply by Behrouz NematiPour on 09 April 2021, 22:21 > done Reply by pmontazemi on 10 April 2021, 15:29 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:26 https://devapps.diality.us/cru/UI-DEN-7135-1#c8908 Note...will not (instead of won't informal language)...for more than... Reply by Behrouz NematiPour on 09 April 2021, 22:21 > done Reply by pmontazemi on 10 April 2021, 15:29 > RESOLVED. Revision Comment by pmontazemi on 01 April 2021, 09:27 https://devapps.diality.us/cru/UI-DEN-7135-1#c8909 ...cannot... Reply by Behrouz NematiPour on 09 April 2021, 22:21 > done Reply by pmontazemi on 10 April 2021, 15:29 > RESOLVED. ---------------------------------------- File: sources/model/settings/MSettings.cpp Revision Comment by plucia on 05 April 2021, 10:01 https://devapps.diality.us/cru/UI-DEN-7135-1#c8995 Docstrings are missing in this class Reply by Behrouz NematiPour on 09 April 2021, 22:18 > Sorry, forgot. > Added. Reply by plucia on 12 April 2021, 10:05 > RESOLVED Revision Comment by plucia on 05 April 2021, 10:01 https://devapps.diality.us/cru/UI-DEN-7135-1#c8994 vGroup is being used so I think you can remove Q_UNUSED Reply by Behrouz NematiPour on 09 April 2021, 22:18 > Removed. Reply by plucia on 12 April 2021, 10:05 > RESOLVED ---------------------------------------- File: sources/model/settings/MSettings.h Revision Comment by plucia on 05 April 2021, 10:02 https://devapps.diality.us/cru/UI-DEN-7135-1#c8996 Is this class really needed? QSettings is already thread-safe. What does this class do that QSettings doesn't already do? Reply by Behrouz NematiPour on 09 April 2021, 21:25 > I was hoping the same but in our setting, the order is so > important but QSettings doesn't guaranty the reading order > and will reorder the read key, value pairs. > So I had to implement our own reader. > For the groups, it doesn't matter much but for the key, > values which are going to be the instruction pair of title, > image the order is so important. > please don't confuse that the designated key and values are > always correct but the lines are not in correct order. Reply by plucia on 12 April 2021, 10:06 > RESOLVED ---------------------------------------- File: sources/storage/FileHandler.cpp Revision Comment by plucia on 05 April 2021, 09:41 https://devapps.diality.us/cru/UI-DEN-7135-1#c8988 Not sure I follow the comment you added here, can you clarify? Reply by Behrouz NematiPour on 05 April 2021, 09:53 > I was trying to say the sorting may require to change later > because the birthTime is not returning a valid date. > And maybe sorting on the name which always returns a valid > value is better. > Also has a note that if it is going to change, be careful > that it is going to use for different purposes other than > only logging. Reply by plucia on 05 April 2021, 10:06 > I see, could you update the comment to say "other than only > just logging" so it's clearer? I was totally confused by it Reply by Behrouz NematiPour on 09 April 2021, 22:19 > Added your suggested wording. Reply by plucia on 12 April 2021, 10:05 > RESOLVED ---------------------------------------- File: sources/storage/Settings.cpp Revision Comment by plucia on 05 April 2021, 10:13 https://devapps.diality.us/cru/UI-DEN-7135-1#c9001 This function only returns 0 so it can be void Reply by Behrouz NematiPour on 09 April 2021, 21:31 > It only returns 0 for now. > Later I have the plan to improve the parser and will use the > return value for the error handling. Reply by plucia on 12 April 2021, 10:07 > RESOLVED Revision Comment by plucia on 05 April 2021, 10:12 https://devapps.diality.us/cru/UI-DEN-7135-1#c9000 Is there protection in case the file is being written to while you are trying to read it? Doesn't QSettings do all of this for us already? Reply by Behrouz NematiPour on 09 April 2021, 21:32 > I'm not sure that QSettings does that for us. > But the design, for now, is that the settings are for reading > only at boot up and no to write. > Later when we decided to have it save the settings will do > the rest for the write/read protection. Reply by plucia on 12 April 2021, 10:07 > RESOLVED Revision Comment by plucia on 05 April 2021, 10:23 https://devapps.diality.us/cru/UI-DEN-7135-1#c9006 Will '[' and ']' always be on the same line? Reply by Behrouz NematiPour on 09 April 2021, 22:08 > Yes, it has to be on the same line to be the group. Reply by plucia on 12 April 2021, 10:07 > RESOLVED Revision Comment by plucia on 05 April 2021, 10:15 https://devapps.diality.us/cru/UI-DEN-7135-1#c9002 If '=' is the last character or first character in the line this won't work Reply by Behrouz NematiPour on 09 April 2021, 21:34 > You are half right. > The intention is not to force to have always a key or a value > since the use case might be different in various situations. > But a good catch that in our specific case if the > value(image) is empty qml was complaining about not finding > the image which fixed in: > InstructionView.qml:~97 Reply by plucia on 12 April 2021, 10:07 > RESOLVED Revision Comment by plucia on 05 April 2021, 10:17 https://devapps.diality.us/cru/UI-DEN-7135-1#c9003 Don't think prepend("/") is needed here since according to https://doc.qt.io/qt-5/qfileinfo.html#absolutePath, "On Unix the absolute path will always begin with the root, '/', directory." Reply by Behrouz NematiPour on 09 April 2021, 21:38 > It was the residue of the previous algorithm. > Removed. Reply by plucia on 12 April 2021, 10:04 > RESOLVED Revision Comment by plucia on 05 April 2021, 10:10 https://devapps.diality.us/cru/UI-DEN-7135-1#c8998 Is this needed or can it be deleted? Reply by Behrouz NematiPour on 09 April 2021, 22:16 > Removed. Reply by plucia on 12 April 2021, 10:04 > RESOLVED ---------------------------------------- File: sources/storage/Settings.h Revision Comment by plucia on 05 April 2021, 10:22 https://devapps.diality.us/cru/UI-DEN-7135-1#c9005 Can this be deleted? Reply by Behrouz NematiPour on 09 April 2021, 22:17 > Removed. Reply by plucia on 12 April 2021, 10:05 > RESOLVED ---------------------------------------- File: sources/storage/StorageGlobals.cpp Revision Comment by pmontazemi on 06 April 2021, 07:17 https://devapps.diality.us/cru/UI-DEN-7135-1#c9010 Why Latin1 and not Latin? Are there two or more Latins? Reply by Behrouz NematiPour on 06 April 2021, 13:34 > it is an alias name for ISO/IEC_8859-1 character encoding > standard. > https://en.wikipedia.org/wiki/ISO/IEC_8859-1 Reply by pmontazemi on 07 April 2021, 09:35 > RESOLVED. ---------------------------------------- File: sources/view/hd/data/VPreTreatmentStatesData.cpp Revision Comment by plucia on 01 April 2021, 17:04 https://devapps.diality.us/cru/UI-DEN-7135-1#c8940 When I tried to test out your branch, I got a compilation error saying that DRY_SELF_TESTS_SYRINGE_PUMP_PRIME_STATE doesn't currently exist in HDDefs.h. My common has the latest changes Reply by Behrouz NematiPour on 01 April 2021, 20:53 > It's working on the Sprint37 branch of the Common. > FW hasn't updated on the master yet. Reply by plucia on 05 April 2021, 09:47 > RESOLVED ---------------------------------------- File: sources/gui/GuiGlobals.h Revision Comment by pmontazemi on 01 April 2021, 10:29 https://devapps.diality.us/cru/UI-DEN-7135-1#c8915 Clarify comment..."...and will a response..." Not clear on what that means. Reply by Behrouz NematiPour on 09 April 2021, 22:20 > Tried to Clarified. > When wanted to explain in only one line very briefly it > becomes confusing. Reply by pmontazemi on 10 April 2021, 15:29 > RESOLVED. --- ID: UI-DEN-7135-1 https://devapps.diality.us/cru/UI-DEN-7135-1 Title: UI-DEN-7135_UI Pre Treatment Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)