This is a list of all comments for UI-DEN-3253-1. Review Summary: No summary General Comment by Behrouz NematiPour on 26 June 2020, 03:45 https://devapps.diality.us/cru/UI-DEN-3253-1#c2598 It seems Jira is doing its best to confuse the user with this whole slider of commits and not showing the comments in order and sometimes even hiding comments. I totally don't understand the purpose of these confusing features. Source versioning is so simple and don't need to be complicated by this unnecessary features. So please forgive me if because of this features I put some comments which are irrelevant. Thanks, Reply by plucia on 01 July 2020, 14:33 > No problem Behrouz. Your comment is noted. > > Since there is no action item in this comment could you > resolve it? General Comment by Behrouz NematiPour on 06 July 2020, 00:45 https://devapps.diality.us/cru/UI-DEN-3253-1#c2670 RESOLVED ---------------------------------------- File: sources/gui/qml/dialogs/Alarm.qml Revision Comment by Behrouz NematiPour on 12 June 2020, 16:24 https://devapps.diality.us/cru/UI-DEN-3253-1#c2340 Please update the Copyright regarding the filename. Please change the file name (back) to NotificationDialog.qml the first name you had was better. Reply by plucia on 15 June 2020, 13:36 > Done. Reply by Behrouz NematiPour on 22 June 2020, 22:57 > RESOLVED Revision Comment by Behrouz NematiPour on 12 June 2020, 16:11 https://devapps.diality.us/cru/UI-DEN-3253-1#c2338 Why the interval is 1 millisecond. It's too fast for any purpose on the UI. Reply by plucia on 15 June 2020, 13:40 > Done. Reply by Behrouz NematiPour on 22 June 2020, 22:57 > RESOLVED ---------------------------------------- File: sources/gui/qml/main.qml Revision Comment by Behrouz NematiPour on 12 June 2020, 14:58 https://devapps.diality.us/cru/UI-DEN-3253-1#c2335 If this case does nothing why it is here? This is the QVariantList version which we are going to use it only for debugging later and I thing you didn't even used it. Please remove. Reply by plucia on 15 June 2020, 13:38 > Thanks, I have removed it Reply by plucia on 17 June 2020, 13:25 > Requested change has been made. Ready to be resolved ---------------------------------------- File: resources/images/alarm.svg Revision Comment by pmontazemi on 11 June 2020, 15:41 https://devapps.diality.us/cru/UI-DEN-3253-1#c2239 Is there any licensing constraint associated with using *.svg files from w3.org? What is the license scheme? Reply by plucia on 11 June 2020, 15:49 > The alarm.svg is provided to us by Karten. > > The xmlns link is just pulling a resource to build the icon > from w3.org Reply by pmontazemi on 11 June 2020, 16:12 > RESOLVED. Revision Comment by Behrouz NematiPour on 12 June 2020, 14:18 https://devapps.diality.us/cru/UI-DEN-3253-1#c2321 We are no using svg. Please use only png. Reply by plucia on 12 June 2020, 14:56 > Until Karten provides us png's that match the alert color > level that is dictated by the requirements, I have to use > svg's to dynamically draw the appropriate color and > background. If we get a request to change the color, using a > png will require that they send us new png's. Using svg's > saves time so we don't have to go back and forth with them to > change the color scheme of the png's. Reply by Behrouz NematiPour on 12 June 2020, 15:42 > I see what you mean, But the png they will give us has to > have transparent background so the background of the parent > will be the background of the image and the bell/speaker > icon will have only white border. Reply by plucia on 15 June 2020, 09:41 > As per our conversation and my demo last Friday about > shifting to using svg instead of png, could you please > resolve? Reply by Behrouz NematiPour on 15 June 2020, 10:01 > Thanks for the demo. > I'm still concerned about performance and some other > things and I need to investigate more. > Until then, please use png instead. Reply by plucia on 17 June 2020, 13:23 > Dispositioned to DEN-3742 Reply by Behrouz NematiPour on 22 June 2020, 12:47 > RESOLVED ---------------------------------------- File: sources/applicationcontroller.h Revision Comment by Behrouz NematiPour on 22 June 2020, 04:32 https://devapps.diality.us/cru/UI-DEN-3253-1#c2488 Peter, First I didn't notice that #include(s) has been moved into header. Includes in the header makes compile time unnecessary longer, so if it's not required please put them back in the cpp file. Since the only change which has been made is the [do,did]FailedTransmit(). Reply by plucia on 22 June 2020, 08:53 > Done Reply by Behrouz NematiPour on 22 June 2020, 10:47 > Thanks Peter, > Out of curiosity why do you still need > #include "guicontroller.h" ? Reply by plucia on 22 June 2020, 11:02 > Sequence is used in the header file. It is defined in the > Can namespace. guicontroller.h imports messageglobals.h. > To clear it up, so there is no confusion, I've just moved > the guicontroller.h import to .cpp and directly importing > messageglobals.h in the header. Reply by Behrouz NematiPour on 22 June 2020, 11:52 > Much better thanks. > RESOLVED Revision Comment by pmontazemi on 11 June 2020, 15:50 https://devapps.diality.us/cru/UI-DEN-3253-1#c2250 Remove extra line. Reply by plucia on 11 June 2020, 16:04 > Done Reply by pmontazemi on 11 June 2020, 16:08 > RESOLVED. Revision Comment by pmontazemi on 11 June 2020, 15:51 https://devapps.diality.us/cru/UI-DEN-3253-1#c2251 Remove extra line. Reply by plucia on 11 June 2020, 16:04 > Done Reply by pmontazemi on 11 June 2020, 16:08 > RESOLVED. Revision Comment by pmontazemi on 11 June 2020, 15:51 https://devapps.diality.us/cru/UI-DEN-3253-1#c2252 Remove extra line. Reply by plucia on 11 June 2020, 16:04 > Done Reply by pmontazemi on 11 June 2020, 16:08 > RESOLVED. ---------------------------------------- File: sources/canbus/messagedispatcher.h Revision Comment by Behrouz NematiPour on 12 June 2020, 14:20 https://devapps.diality.us/cru/UI-DEN-3253-1#c2324 Please remove KeepAlive from the list. We haven't decided to use keepAlive as the check for the communication connection yet. Reply by plucia on 15 June 2020, 09:48 > Done. Reply by Behrouz NematiPour on 15 June 2020, 11:45 > RESOLVED ---------------------------------------- File: sources/canbus/messageglobals.h Revision Comment by Behrouz NematiPour on 12 June 2020, 14:23 https://devapps.diality.us/cru/UI-DEN-3253-1#c2326 Why these two are not defined in the message list spreadsheet? Where is the definition? Please collaborate with FW Team to have correct length here even if it has not been defined it needs a definition to have a placeholder. Reply by plucia on 15 June 2020, 10:46 > I have talked with Sean and the placeholder has been added to > the message lists spreadsheet. Reply by Behrouz NematiPour on 15 June 2020, 11:05 > Thanks Peter for talking to Sean to have a kind of > definitions in Messages List. > I meant a discussion on how it's going to be done and > having some types for parameters at least. > Anyway, although that's not what I meant I'm going to > resolve this. > I believe this was not a good task to start with since the > FW portion of it was not done. > Maybe better next time. > > Thanks, > > RESOLVED ---------------------------------------- File: sources/canbus/messageinterpreter.cpp Revision Comment by pmontazemi on 11 June 2020, 15:44 https://devapps.diality.us/cru/UI-DEN-3253-1#c2242 Add empty line between each case. Reply by plucia on 11 June 2020, 15:51 > This is a fallthrough - the AdjustBloodDialysateReq will fall > through to the AdjustDurationReq case statement. > > The extra white-space is intentially removed to indicate it > is a fallthrough. Other examples: > https://en.cppreference.com/w/cpp/language/switch Reply by pmontazemi on 11 June 2020, 16:09 > RESOLVED. Revision Comment by Behrouz NematiPour on 12 June 2020, 14:39 https://devapps.diality.us/cru/UI-DEN-3253-1#c2327 There was no Fall through in the original code. Please, do not remove codes even if it's the same code. Even if we are going to (which we are not) have fall through there should be Q_FALLTHROUGH(); in the code as has been stated in the C++ coding standard. Reply by plucia on 15 June 2020, 10:56 > Please see my latest commit Reply by Behrouz NematiPour on 15 June 2020, 11:21 > Thanks, > RESOLVED. Revision Comment by pmontazemi on 11 June 2020, 15:47 https://devapps.diality.us/cru/UI-DEN-3253-1#c2244 Add empty line. Reply by plucia on 15 June 2020, 14:30 > Done Reply by pmontazemi on 16 June 2020, 09:21 > RESOLVED. Revision Comment by pmontazemi on 11 June 2020, 15:47 https://devapps.diality.us/cru/UI-DEN-3253-1#c2245 Add empty line. Reply by plucia on 23 June 2020, 10:39 > Done Reply by pmontazemi on 23 June 2020, 16:40 > RESOLVED. ---------------------------------------- File: sources/gui/guicontroller.h Revision Comment by pmontazemi on 11 June 2020, 15:48 https://devapps.diality.us/cru/UI-DEN-3253-1#c2246 Remove empty line. Reply by plucia on 15 June 2020, 14:40 > Done Reply by pmontazemi on 16 June 2020, 09:25 > RESOLVED. ---------------------------------------- File: sources/gui/guiglobals.h Revision Comment by Behrouz NematiPour on 12 June 2020, 14:23 https://devapps.diality.us/cru/UI-DEN-3253-1#c2325 Why these two are not defined in the message list spreadsheet? Where is the definition? Reply by plucia on 12 June 2020, 16:55 > These are placeholders until Sean has time to put them in the > message list spreadsheet Reply by plucia on 15 June 2020, 14:38 > Talked with Sean and these are now in the message list Reply by plucia on 17 June 2020, 13:26 > Requested change has been made. Ready to be resolved. Reply by Behrouz NematiPour on 22 June 2020, 23:08 > Had a conversation and also Peman mentioned that if a > story is not ready we should not start it. > Better to be cooperated and consulted with FW team for > a clear definition of the required messages. > > RESOLVED ---------------------------------------- File: sources/gui/guiview.h Revision Comment by Behrouz NematiPour on 26 June 2020, 04:48 https://devapps.diality.us/cru/UI-DEN-3253-1#c2604 Why? Reply by plucia on 01 July 2020, 13:31 > Good catch. I have removed it Reply by Behrouz NematiPour on 06 July 2020, 00:45 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/NotificationBar.qml Revision Comment by Behrouz NematiPour on 12 June 2020, 16:07 https://devapps.diality.us/cru/UI-DEN-3253-1#c2337 Please do not use QGraphcalEffects our device display resolution and color depth is not supporting it and most of the time it makes application ugly. Reply by plucia on 15 June 2020, 12:09 > It is a required import for ColorOverlay Reply by plucia on 17 June 2020, 13:24 > Dispositioned to DEN-3742 Reply by Behrouz NematiPour on 22 June 2020, 22:23 > We had a conversation to look into the SVG images for : > - default color and change it to white (it's black) so > we don't need QGraphcalEffects and ColorOverlay to > manipulate SVG images properties. > - also see if it is a reference or the actual svg so the > code can be build without internet the content of the > file is so tiny regarding the image itself. Reply by plucia on 23 June 2020, 08:57 > I have now removed QtGraphicalEffects and ColorOverlay > and modified the svg colors in the svg source Reply by Behrouz NematiPour on 06 July 2020, 01:01 > RESOLVED Revision Comment by Behrouz NematiPour on 26 June 2020, 03:09 https://devapps.diality.us/cru/UI-DEN-3253-1#c2594 Please do not remove the comments with // SquishQt. It means it has been added to be used in SquishQt. And It shows that if we change it our unit tests in SquishQt is not working anymore since it is using this object name to identify the object properly. Reply by plucia on 26 June 2020, 08:32 > Sorry. I think in the merge this happened. I'll add it back Reply by plucia on 01 July 2020, 14:10 > Done Reply by Dara Navaei on 19 October 2023, 10:51 > RESOLVED Revision Comment by Behrouz NematiPour on 12 June 2020, 14:44 https://devapps.diality.us/cru/UI-DEN-3253-1#c2329 putting ; at the end of the line is not necessary in QML. Please, put it only if required and if not necessary don't add or remove it. Reply by plucia on 15 June 2020, 12:26 > Done. Reply by Behrouz NematiPour on 15 June 2020, 12:32 > RESOLVED. Revision Comment by Behrouz NematiPour on 12 June 2020, 14:47 https://devapps.diality.us/cru/UI-DEN-3253-1#c2331 We are no using svg. Please use only png. Reply by plucia on 15 June 2020, 12:11 > You've already commented about using svg vs. png in the > resources/images/alarm.svg, resources/images/bell-off.svg, > and resources/images/bell.svg. I don't think you need to find > every instance of the svg file to make your point. It goes > without saying that the source code will need to be > completely changed if we don't use svgs. Reply by Behrouz NematiPour on 15 June 2020, 12:33 > Thanks Peter, > My comments were redundant. > At the time I started I didn't know which one will catch > you're attention. > RESOLVED Revision Comment by Behrouz NematiPour on 12 June 2020, 14:45 https://devapps.diality.us/cru/UI-DEN-3253-1#c2330 we are not handling timeouts and timers in application. It makes application and HD out of sync. HD will send the times if required each second. That's how it has been discussed and designed. There should be a message later which has the timeout as a data and UI only updates itself by that value. If it's not for animation, please remove it. Reply by plucia on 15 June 2020, 12:14 > This is a placeholder animation until the alarms UI - HD > integration work can begin. It is necessary for SW to meet > the requirements until HD firmware has been updated. Reply by Behrouz NematiPour on 22 June 2020, 13:13 > RESOLVED Reply by Behrouz NematiPour on 22 June 2020, 13:18 > Thanks Peter, > I disagree with the last sentence. > We should not simulate the behavior to meet the > requirement. > But I agree that we need to have the simulation to see the > code is not misbehaving. Reply by pmontazemi on 23 June 2020, 16:40 > RESOLVED Revision Comment by Behrouz NematiPour on 12 June 2020, 16:14 https://devapps.diality.us/cru/UI-DEN-3253-1#c2339 These objects don't need to be children of the the MouseArea. Please only put MouseArea at the end of the page and put the other object before that. Reply by plucia on 15 June 2020, 12:16 > If it's functionally the same then why should it matter > whether they are children of MouseArea or not? Reply by Behrouz NematiPour on 22 June 2020, 13:14 > Thanks for applying that. > Also please move the MouseArea after all the components so > it becomes the highest in the z order and can grab all the > clicks. Reply by plucia on 22 June 2020, 14:13 > Done Reply by Behrouz NematiPour on 22 June 2020, 22:55 > RESOLVED Revision Comment by Behrouz NematiPour on 12 June 2020, 14:48 https://devapps.diality.us/cru/UI-DEN-3253-1#c2332 We are no using svg. Please use only png. Reply by plucia on 15 June 2020, 12:18 > Again, you've made your point many times about the svg vs. > png issue. We don't need extra comments reminding us of it or > where the svg's are in the code. Reply by Behrouz NematiPour on 22 June 2020, 13:15 > RESOLVED Revision Comment by Behrouz NematiPour on 12 June 2020, 14:49 https://devapps.diality.us/cru/UI-DEN-3253-1#c2333 Please do not use QGraphcalEffects our device display resolution and color depth is not supporting it and most of the time it makes application ugly. Reply by plucia on 15 June 2020, 12:18 > Why insist on making the same comment twice? We both know > where ColorOverlay is and that QGraphicalEffects is necessary > to import to use it. > > In my demo on Friday, the application did not look "ugly." It > was using QGraphicalEffects to draw SVG's and my > understanding was that you said it looked fine. I'm confused. > What has changed? Reply by Behrouz NematiPour on 22 June 2020, 22:33 > please refer to the reply of the comment : > [http://dvm-linux02:8060/cru/UI-DEN-3253-1#c2505] Reply by plucia on 23 June 2020, 08:59 > I have now removed QtGraphicalEffects and ColorOverlay > and modified the svg colors in the svg source Reply by Behrouz NematiPour on 26 June 2020, 03:15 > RESOLVED Revision Comment by Behrouz NematiPour on 14 June 2020, 19:25 https://devapps.diality.us/cru/UI-DEN-3253-1#c2349 Please rename this signal to "clicked" to clarify the definition and usage. It's confusing since, "Pressed" and "Released" all together are "Clicked" event. Reply by plucia on 15 June 2020, 12:21 > Clicked actually suggests that a mouse is being used. Pressed > suggested that someone pressed the screen with their finger, > which is our primary use case. Reply by Behrouz NematiPour on 26 June 2020, 03:08 > Per our conversation please change the pressed to clicked. > Thanks, Reply by plucia on 01 July 2020, 13:55 > Done Reply by Behrouz NematiPour on 06 July 2020, 00:58 > Just to clarify the conversation : > Touch is another set of events in Qt for gestures and > single or double touch are still considered > click/press/release. > So better to have clicked for single touch. > > RESOLVED ---------------------------------------- File: sources/gui/qml/components/SettingsItem.qml Revision Comment by Behrouz NematiPour on 14 June 2020, 19:19 https://devapps.diality.us/cru/UI-DEN-3253-1#c2348 MouseArea doesn't need to be parent of any object. Please move objects out of MouseArea block and move it at the end. Reply by plucia on 15 June 2020, 12:27 > Why does it matter, functionally? Reply by Behrouz NematiPour on 22 June 2020, 22:56 > Please refer to the conversation in comment : > [http://dvm-linux02:8060/cru/UI-DEN-3253-1#CFR-13849] Reply by plucia on 01 July 2020, 14:09 > Done Reply by Behrouz NematiPour on 06 July 2020, 01:00 > Clarification: > The beauty of QML is that the MouseArea doesn't have to > be bound to any sibling or even any object so it give a > lot of flexibility in desin. > To utilize this feature it has to be used that way. > > RESOLVED ---------------------------------------- File: sources/gui/qml/dialogs/NotificationDialog.qml Revision Comment by Behrouz NematiPour on 22 June 2020, 23:00 https://devapps.diality.us/cru/UI-DEN-3253-1#c2512 Same : [http://dvm-linux02:8060/cru/UI-DEN-3253-1#c2505] Reply by plucia on 23 June 2020, 09:26 > Done Reply by plucia on 23 June 2020, 10:21 > QtGraphicalEffects is now removed Reply by Behrouz NematiPour on 23 June 2020, 13:33 > perfect. > RESOLVED Revision Comment by Behrouz NematiPour on 22 June 2020, 23:01 https://devapps.diality.us/cru/UI-DEN-3253-1#c2513 Please change pressed to clicked as we discussed. Reply by plucia on 23 June 2020, 10:28 > Done Reply by Behrouz NematiPour on 23 June 2020, 13:34 > RESOLVED Revision Comment by Behrouz NematiPour on 22 June 2020, 23:01 https://devapps.diality.us/cru/UI-DEN-3253-1#c2514 same : Please change pressed to clicked as we discussed. Reply by plucia on 23 June 2020, 10:28 > Done Reply by Behrouz NematiPour on 23 June 2020, 13:34 > RESOLVED Revision Comment by Behrouz NematiPour on 22 June 2020, 22:59 https://devapps.diality.us/cru/UI-DEN-3253-1#c2511 Same here : [http://dvm-linux02:8060/cru/UI-DEN-3253-1#c2505] Reply by plucia on 23 June 2020, 10:21 > The ColorOverlay has been removed Reply by Behrouz NematiPour on 23 June 2020, 13:34 > RESOLVED ---------------------------------------- File: sources/gui/qml/globals/Colors.qml Revision Comment by Behrouz NematiPour on 15 June 2020, 11:51 https://devapps.diality.us/cru/UI-DEN-3253-1#c2379 The notification background for the normal messages should be white by default. As we have it in the UX design. https://app.zeplin.io/project/5e160353a7c41a9404596a70/screen/5e4f250562025d78ef8435b8 Please explain why it's changed? Reply by plucia on 15 June 2020, 14:36 > During development I must have run into an issue with it > being white instead of transparent. It looks like it doesn't > make a difference anymore, so I have changed it back to > white. Reply by plucia on 17 June 2020, 13:25 > Requested change has been made. Please resolve Reply by Behrouz NematiPour on 22 June 2020, 23:02 > RESOLVED Revision Comment by Behrouz NematiPour on 26 June 2020, 04:07 https://devapps.diality.us/cru/UI-DEN-3253-1#c2601 Please align with the rest. Reply by plucia on 26 June 2020, 08:48 > Good catch Reply by plucia on 01 July 2020, 13:54 > Done Reply by Behrouz NematiPour on 06 July 2020, 00:48 > RESOLVED ---------------------------------------- File: sources/gui/qml/pages/Diagnostics.qml Revision Comment by pmontazemi on 25 June 2020, 11:26 https://devapps.diality.us/cru/UI-DEN-3253-1#c2583 Why space before .? Reply by Behrouz NematiPour on 26 June 2020, 02:54 > These are my code Peman, > And has been merged from master, somehow. (Original code is > not in this screen and are in SettingsHome.qml) > As usual it helps to immediately identify that toFixed() > function has not been forgotten and been used to set decimal > point by 2 digits. Reply by pmontazemi on 26 June 2020, 08:22 > Whether it is your code or anybody else's code, it doesn't > matter, this way of writing code is simply off standard and > unprecedented, frankly. It also does not matter whether > Peter or anybody else approved it or not, all that matters > is that it is against the standard. Reply by Behrouz NematiPour on 26 June 2020, 14:31 > Sure, > I'll update that in my current working branch. > So Peter leave it as it is here and I'll fix that there. > Thanks, Reply by plucia on 01 July 2020, 14:37 > This happened as part of the merge. I think Behrouz is > already updating it in his branch. To avoid future merge > conflicts, I think it'd be best to ensure the styling > issues are fixed only on his branch instead of here too. Reply by pmontazemi on 07 July 2020, 09:11 > RESOLVED. Revision Comment by pmontazemi on 25 June 2020, 11:26 https://devapps.diality.us/cru/UI-DEN-3253-1#c2584 Same comment here. Reply by Behrouz NematiPour on 26 June 2020, 02:55 > [http://dvm-linux02:8060/cru/UI-DEN-3253-1#c2583] Reply by plucia on 06 July 2020, 11:39 > Please see my above and linked to by Behrouz. To avoid future > merge conflicts, I think it'd be best to ensure the styling > issues are fixed only on Behrouz's branch instead of here > too. If you agree, I think this and the above comment can be > resolved. Reply by pmontazemi on 07 July 2020, 09:11 > RESOLVED. Revision Comment by pmontazemi on 25 June 2020, 11:26 https://devapps.diality.us/cru/UI-DEN-3253-1#c2585 Same here. Reply by Behrouz NematiPour on 26 June 2020, 02:55 > [http://dvm-linux02:8060/cru/UI-DEN-3253-1#c2583] Reply by plucia on 06 July 2020, 13:36 > Please see my above and linked to by Behrouz. To avoid future > merge conflicts, I think it'd be best to ensure the styling > issues are fixed only on Behrouz's branch instead of here > too. If you agree, I think this and the above comment can be > resolved. Reply by pmontazemi on 07 July 2020, 09:11 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/SettingsHome.qml Revision Comment by Behrouz NematiPour on 26 June 2020, 03:55 https://devapps.diality.us/cru/UI-DEN-3253-1#c2599 Peter, Thanks for the nice menu here. But with the requests we received recently for HW support seems like they need those screens to be the way it is for quick access to debug information. We may need to revert that back to the previous navigation mode. We can have a conversation how to do that. Thanks, Reply by plucia on 26 June 2020, 08:40 > All the unit tests are passing with this menu right now. Code > coverage is almost 100%. Deleting it will break many squish > qt tests and may also affect the code coverage. The HW team > can access the diagnostics screen the same as before on the > diagnostics page, which is listed second on the settings > page, very clearly, and quickly accessible. Reply by Behrouz NematiPour on 26 June 2020, 12:52 > I understand and agree, that's extra work. > But that wasn't even part of the Alarm design. > This is the settings feature which has not been started > yet. > That's exactly why we have this interview to let you (or > any developer) know the changes before the Unit > test/Integration Test/Code Coverage is done and also to > know that against which code those test/coverage needs to > be done. Reply by Behrouz NematiPour on 06 July 2020, 00:52 > RESOLVED Reply by plucia on 01 July 2020, 14:32 > As per our email chain on 6/29, I believe this can be > resolved Reply by Behrouz NematiPour on 06 July 2020, 00:51 > RESOLVED Revision Comment by Behrouz NematiPour on 22 June 2020, 23:04 https://devapps.diality.us/cru/UI-DEN-3253-1#c2516 Why it has been imported ? Reply by plucia on 23 June 2020, 10:28 > Removed it, as I think it's not used anymore Reply by Behrouz NematiPour on 23 June 2020, 13:35 > RESOLVED ---------------------------------------- File: sources/view/valarmstatus.cpp Revision Comment by Behrouz NematiPour on 22 June 2020, 23:39 https://devapps.diality.us/cru/UI-DEN-3253-1#c2521 "alarmID" is the duplicate of the property "alarm_AlarmID" on line 45. Why has been re-defined ? Reply by plucia on 23 June 2020, 09:16 > When an alarm is received, if it is high priority, a red > notification dialog will be shown. If an alarm has been > acknowledged by the user, it's not necessary to show the > dialog every second as this will prevent the user from being > able to continue using the application. So, a previously > acknowledged alarm is not shown. alarmID is needed to obtain > the value of that alarm ID prior to updating the Q_PROPERTY > so that the QML doesn't show the previously acknowledged > alarm Reply by Behrouz NematiPour on 06 July 2020, 00:54 > Still not clear and I think there should be a better way of > having duplicate data. > It has the exact same value. > OK for now. > > RESOLVED Revision Comment by Behrouz NematiPour on 22 June 2020, 23:15 https://devapps.diality.us/cru/UI-DEN-3253-1#c2520 This class is the child of the QObject so no need for QObject::tr and tr is enough. Reply by plucia on 23 June 2020, 09:25 > Done. Do you know QObject::tr is needed in MAlarmStatus.cpp > and not here? Reply by Behrouz NematiPour on 26 June 2020, 03:05 > Models do not suppose to instantiate from QObject because > we don't what them emit signals and use slots. > They are just data models. Reply by plucia on 01 July 2020, 14:40 > Makes sense. I've finished the action item above so this > is ready to be resolved. Reply by Behrouz NematiPour on 06 July 2020, 00:57 > RESOLVED ---------------------------------------- File: sources/view/valarmstatus.h Revision Comment by Behrouz NematiPour on 22 June 2020, 23:46 https://devapps.diality.us/cru/UI-DEN-3253-1#c2522 The parameter alarmID comming from GUI is not required. This class itself has told GUI what is the alarm by the alarm top id. Let's talk about it. Reply by plucia on 01 July 2020, 13:37 > It will be required to support multiple alarms Reply by Behrouz NematiPour on 06 July 2020, 00:52 > Still not clear. > Ok for now. > > RESOLVED ---------------------------------------- File: sources/model/malarmstatus.cpp Revision Comment by pmontazemi on 11 June 2020, 15:49 https://devapps.diality.us/cru/UI-DEN-3253-1#c2248 Where was this list of alarms obtained from and how do we know it is matching the supported ones in the DG/HD Firmware? Reply by plucia on 11 June 2020, 15:57 > It was pulled from AlarmDefs.h. > > [~bnematipour] I think to Peman's point it would be better to > place these alarm messages in AlarmDefs.h. What do you think? Reply by pmontazemi on 11 June 2020, 16:09 > Agreed, would move them, there, in fact isn't AlarmDefs.h > part of common between DG FW, HD FW, and UI SW? I believe > alarms and messages are the only items that are common to > all three software items/systems. Reply by plucia on 17 June 2020, 13:27 > I spoke with Sean, he said FW doesn't need access to the > alarm message text displayed by the UI. > > Since they need to be translated, they need to reside > here so that the UI can translate them with > QObject::tr(). Reply by Behrouz NematiPour on 22 June 2020, 23:12 > These translations need to be in Application and use > the enums which has been defined by FW. > Therefore what Peter did is the correct implementation > of the translations. > Thanks, Reply by pmontazemi on 23 June 2020, 16:42 > RESOLVED. Revision Comment by Behrouz NematiPour on 26 June 2020, 04:45 https://devapps.diality.us/cru/UI-DEN-3253-1#c2603 Is there any particular reason why some of the messages have space at the end and some others don't? Reply by plucia on 26 June 2020, 08:47 > Good catch. I'll remove the spaces Reply by plucia on 01 July 2020, 13:32 > Done Reply by Behrouz NematiPour on 06 July 2020, 00:47 > RESOLVED Revision Comment by Behrouz NematiPour on 06 July 2020, 00:46 https://devapps.diality.us/cru/UI-DEN-3253-1#c2673 Typo : erro Reply by plucia on 06 July 2020, 15:19 > Done Reply by Behrouz NematiPour on 06 July 2020, 15:24 > RESOLVED Revision Comment by Behrouz NematiPour on 06 July 2020, 00:46 https://devapps.diality.us/cru/UI-DEN-3253-1#c2672 Typo ? inconsisten Reply by plucia on 06 July 2020, 15:20 > Done Reply by Behrouz NematiPour on 06 July 2020, 15:24 > RESOLVED ---------------------------------------- File: main.cpp Revision Comment by pmontazemi on 11 June 2020, 15:51 https://devapps.diality.us/cru/UI-DEN-3253-1#c2253 Why false? Reply by plucia on 11 June 2020, 15:56 > Our current drivers don't require keep-awake CAN traffic > between UI and FW, so I've set it to be disabled by default. Reply by pmontazemi on 11 June 2020, 16:08 > RESOLVED. Revision Comment by Behrouz NematiPour on 26 June 2020, 04:31 https://devapps.diality.us/cru/UI-DEN-3253-1#c2602 What is the use case of disabling the alarms? It makes application kind of mute like some one who can't talk or communicate. Seems like a dangerous switch. I prefer not to be kept as a switch, if needs for debugging better to use #ifdef instead. Reply by plucia on 26 June 2020, 08:14 > The use case is for us to be able to run the code coverage > tests without the UI raising alarms when the HD is not > connected. This is so the other squishqt tests will be able > to run normally. Otherwise, squishqt can't find the qml > components when the HD communication timeout alarm shows and > some of the other squishqt tests will fail. Reply by Behrouz NematiPour on 26 June 2020, 12:49 > Later when we have the API integrated in the Dialin, we > need to send Ack back and then we have to remove this. > The reason is application has to be tested exactly the way > it is used in the field with no extra switch to bypass any > check just to check. > Otherwise we are not testing the real application. Reply by plucia on 26 June 2020, 14:18 > Yes, I agree. For now, I've updated it so the flag will > only disable the HD communication timeout error, instead > of all the alarms. This will reduce the scope of the flag > and also prevent it from affecting the other squishqt > tests before we move things to Dialin. Reply by Behrouz NematiPour on 06 July 2020, 00:48 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/ModalDialog.qml Revision Comment by Behrouz NematiPour on 12 June 2020, 14:43 https://devapps.diality.us/cru/UI-DEN-3253-1#c2328 Putting ; at the end of the line is not necessary in QML. Please put it only if required and if not necessary don't add or remove it. Reply by plucia on 15 June 2020, 12:07 > FYI: AS of today, the QML coding standard > (X:\Engineering\Denali\06- Software Design\Software > Documentation\Coding Standards) doesn't currently dictate > when and when not to use semicolons in QML. > > I've removed them here as requested. Reply by Behrouz NematiPour on 15 June 2020, 12:31 > Thanks Peter, > You're right. > Since I'm updating that doc I'll add a reference to that. > > RESOLVED. ---------------------------------------- File: sources/gui/qml/globals/Fonts.qml Revision Comment by pmontazemi on 25 June 2020, 11:25 https://devapps.diality.us/cru/UI-DEN-3253-1#c2581 Remove extra space before : 22 to align with other lines. Reply by Behrouz NematiPour on 26 June 2020, 03:01 > Thank god. > It was killing me. > I couldn't help to not to say that ;) Reply by plucia on 06 July 2020, 11:42 > Done. Reply by pmontazemi on 07 July 2020, 09:12 > RESOLVED. ---------------------------------------- File: resources/images/bell-off.svg Revision Comment by pmontazemi on 11 June 2020, 15:41 https://devapps.diality.us/cru/UI-DEN-3253-1#c2240 Is there any licensing constraint associated with using *.svg files from w3.org? What is the license scheme? Reply by plucia on 11 June 2020, 15:50 > This bell-off.svg icon is obtained from: > https://feathericons.com/ > > It is an open source icon and uses the MIT license Reply by pmontazemi on 11 June 2020, 16:10 > RESOLVED. Revision Comment by Behrouz NematiPour on 12 June 2020, 14:18 https://devapps.diality.us/cru/UI-DEN-3253-1#c2322 We are not using svg. Please use only png. Reply by plucia on 15 June 2020, 09:44 > As per our conversation and my demo last Friday about > shifting to using svg instead of png, could you please > resolve? Reply by Behrouz NematiPour on 15 June 2020, 10:00 > Thanks for the demo. > I'm still concerned about performance and some other things > and I need to investigate more. > Until then, please use png instead. Reply by plucia on 15 June 2020, 10:45 > On Friday you said we should completely shift to using > svg's. What changed since then? Reply by Behrouz NematiPour on 15 June 2020, 10:48 > You're almost right. > If we are going to use SVG we need to completely move > to SVG and not partially using svg or png here and > there. > I don't want to jump to a new solution without > investigating enough. > I spent some time in weekend reading about using SVG > for embedded systems and I didn't find any complete > suggestions and recommendation other than one single > guy named "Stanley Moriss" who on 2014 decided to use > SVG in Fuji project (as one his presentation pages > shows) in BIT-Group. > Last thing I remember there was no png used in Fuji so > what happened to that idea I'm not sure. > That concerns me a little that we are going the first > one and I don't our project to be the "crash test > dummy" for other people. > Thanks again for the great demo and how it works we > will move to SVG as soon as we are completely sure > that's a correct approach. > So please stick to the plan (using png) until then. Reply by plucia on 15 June 2020, 11:19 > You'll probably disagree with using svg's for small > icons and png's for large images, but this is in > alignment with the recommendation of the Qt company: > > > Use SVG images for small icons. While larger SVGs > can be slow to render, small ones work well. Vector > images avoid the need to provide several versions of > an image, as is necessary with bitmap images. > > The bell and alarm icons are small, so I have used > svg to avoid adding many versions of them to support > all the alarm priority colors, sizes, and ease future > changes during integration work several months from > now. > > https://www3.sra.co.jp/qt/relation/doc-snapshot/qtquick/qtquick-bestpractices.html Reply by plucia on 17 June 2020, 13:21 > Dispositioned to > http://dvm-linux02:8080/browse/DEN-3742 Reply by Behrouz NematiPour on 22 June 2020, 12:48 > RESOLVED ---------------------------------------- File: resources/images/bell.svg Revision Comment by pmontazemi on 11 June 2020, 15:41 https://devapps.diality.us/cru/UI-DEN-3253-1#c2241 Is there any licensing constraint associated with using *.svg files from w3.org? What is the license scheme? Reply by plucia on 11 June 2020, 15:46 > The Bell icons are obtained from here: > https://feathericons.com/ > > It is an open source icon and uses the MIT license Reply by pmontazemi on 11 June 2020, 16:10 > RESOLVED. Revision Comment by Behrouz NematiPour on 12 June 2020, 14:18 https://devapps.diality.us/cru/UI-DEN-3253-1#c2323 We are not using svg. Please use only png. Reply by plucia on 15 June 2020, 09:44 > As per our conversation and my demo last Friday about > shifting to using svg instead of png, could you please > resolve? Reply by Behrouz NematiPour on 15 June 2020, 10:01 > Thanks for the demo. > I'm still concerned about performance and some other things > and I need to investigate more. > Until then, please use png instead. Reply by plucia on 17 June 2020, 13:24 > Dispositioned to DEN-3742 Reply by Behrouz NematiPour on 22 June 2020, 12:48 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/TouchArea.qml Revision Comment by pmontazemi on 25 June 2020, 11:25 https://devapps.diality.us/cru/UI-DEN-3253-1#c2580 Not a big fan of numeration on variables, functions/methods, etc. Reply by Behrouz NematiPour on 26 June 2020, 03:02 > Agree and also please look at this comment > [http://dvm-linux02:8060/cru/UI-DEN-3253-1#c2582] Reply by pmontazemi on 29 June 2020, 15:29 > RESOLVED. Revision Comment by Behrouz NematiPour on 26 June 2020, 04:05 https://devapps.diality.us/cru/UI-DEN-3253-1#c2600 [http://dvm-linux02:8060/cru/UI-DEN-3253-1#CFR-14076] Reply by Behrouz NematiPour on 06 July 2020, 00:51 > RESOLVED ---------------------------------------- File: sources/gui/qml/pages/treatment/adjustments/TreatmentAdjustmentBase.qml Revision Comment by pmontazemi on 25 June 2020, 11:25 https://devapps.diality.us/cru/UI-DEN-3253-1#c2582 Same comment. Reply by Behrouz NematiPour on 26 June 2020, 02:57 > Hummmm, > Same as which comment? > Sorry seems like Jira doesn't show comments in order I can't > follow the comments in order. > > But I have another question why we have "Notification2" ? > We suppose to have one and only one NotificationBar. > It will break all the other unit tests... > Could be more clear on this? Reply by pmontazemi on 26 June 2020, 08:26 > Same comment as O am not a big fan of iterating on variable > names, function names, etc. This is also not Jira, but > Crucible. Reply by plucia on 26 June 2020, 08:27 > I've updated the unit tests so they are all passing now. > I'm just working on increasing the code coverage Reply by plucia on 26 June 2020, 08:30 > Yes, I renamed it to NotificationBarSmall.qml after > seeing your comment Peman. I agree NotificationBar2 was > not the best name - I was in a bit of a rush to merge the > code so I could address the code coverage and unit tests. > All the unit tests are now passing but I'm still working > on the code coverage Reply by plucia on 06 July 2020, 11:37 > Hi Peman I believe I have finished the action item related to > your comment - it is now named NotificationBarSmall Reply by pmontazemi on 07 July 2020, 09:11 > RESOLVED. ---------------------------------------- File: sources/gui/qml/components/NotificationBarSmall.qml Revision Comment by Behrouz NematiPour on 26 June 2020, 03:22 https://devapps.diality.us/cru/UI-DEN-3253-1#c2596 Could you please explain what is this component and why we have it. We should have only one NotificationBar. But as it seems it is only being used for notifications on each sections of MainTreatment screen. If that is the case, It is a good idea but then we need to have a kind of parent and child components structure in which the parent is the simple one and the children have the extra features. like what we currently have in Dialogs. Please let me review what would be the good implementation. Peter, if you want to change these kind of things please collaborate, negotiate and let's review the rational behind the existence of a component and then change it together. Thanks, Reply by plucia on 26 June 2020, 08:34 > Yes, for now it's separated so I didn't break the > ultrafiltration notification bar. A base and subcomponent > structure makes sense. I'll take a look at doing that. Wasn't > trying to change it without looping you in I was just trying > to get the merge to compile and then ensure the unit tests > passed. Reply by Behrouz NematiPour on 26 June 2020, 12:58 > Peter, > Could you please explain why it needs to be separated. > The current unit tests on server are using the same object > name and identification and working. > The only thing needs to be done is to separate them by > their container which the location it has been instantiated > and used. > So this way you're working with objects not classes. Reply by plucia on 01 July 2020, 14:13 > It needs to be separated at least for now because the > icon placement is different now that we are putting the > alarm silence icon at the very left of the alarm bar. The > small notification bar, which is primarily used inside > alerts instead of alarms, does not have alarm specific > functions and animation needed for high priority alarms. Reply by plucia on 01 July 2020, 14:15 > Also there are two icons in NotificationBar.qml, the > alert icon and the alarm silence icon. The small > notification bar only has one icon Reply by Behrouz NematiPour on 06 July 2020, 00:51 > I don't agree with the implementation. > OK for now. > > RESOLVED ---------------------------------------- File: unittests/tst_messaging.cpp Revision Comment by Behrouz NematiPour on 06 July 2020, 01:09 https://devapps.diality.us/cru/UI-DEN-3253-1#c2687 The name is too general please change the name that reflects the test. If it's not one test please separate them. Reply by plucia on 06 July 2020, 13:38 > Done Reply by Behrouz NematiPour on 06 July 2020, 13:50 > RESOLVED Revision Comment by pmontazemi on 29 June 2020, 16:10 https://devapps.diality.us/cru/UI-DEN-3253-1#c2631 Remove extra line. Reply by plucia on 01 July 2020, 13:46 > Done Reply by pmontazemi on 07 July 2020, 09:10 > RESOLVED. ---------------------------------------- File: unittests/tst_models.cpp Revision Comment by pmontazemi on 29 June 2020, 16:11 https://devapps.diality.us/cru/UI-DEN-3253-1#c2632 Remove extra lines. Reply by plucia on 01 July 2020, 13:46 > Done Reply by pmontazemi on 07 July 2020, 09:10 > RESOLVED. ---------------------------------------- File: unittests/tst_models.h Revision Comment by pmontazemi on 29 June 2020, 16:11 https://devapps.diality.us/cru/UI-DEN-3253-1#c2633 Remove extra line. Reply by plucia on 01 July 2020, 13:45 > Done Reply by pmontazemi on 07 July 2020, 09:10 > RESOLVED. --- ID: UI-DEN-3253-1 https://devapps.diality.us/cru/UI-DEN-3253-1 Title: UI-DEN-3253_UI Alarm Design Statement of Objectives: State: Closed Summary: Author: plucia Moderator: plucia Reviewers: (6 active, 2 completed*) Behrouz NematiPour (*) pmontazemi (*) Sean Nash vduong Tiffany Mejia Michael Garthwaite Dara Navaei msuleiman