This is a list of all comments for UI-DEN-8085-1. Review Summary: No summary ---------------------------------------- File: sources/gui/GuiGlobals.h Revision Comment by Behrouz NematiPour on 29 April 2021, 10:53 https://devapps.diality.us/cru/UI-DEN-8085-1#c9666 Regarding our discussion, this will be replaced by a new enum ID_AlarmVolumeSetRsp next to ID_AlarmVolumeSetReq. Reply by plucia on 04 May 2021, 14:47 > Done Reply by Behrouz NematiPour on 06 May 2021, 16:29 > RESOLVED ---------------------------------------- File: sources/gui/qml/pages/SettingsHome.qml Revision Comment by Behrouz NematiPour on 29 April 2021, 09:50 https://devapps.diality.us/cru/UI-DEN-8085-1#c9650 Please modify the signal's name so it always has the Clicked/Pressed/... at the end. Please apply for every other occurance. Reply by plucia on 04 May 2021, 11:00 > Done Reply by Behrouz NematiPour on 06 May 2021, 17:54 > RESOLVED ---------------------------------------- File: sources/model/hd/adjustment/MTreatmentAdjustRequests.h Revision Comment by Behrouz NematiPour on 29 April 2021, 10:30 https://devapps.diality.us/cru/UI-DEN-8085-1#c9660 What is this line used for? Reply by plucia on 04 May 2021, 12:37 > I've removed it now. It's the same CAN channel as in the > parent class so no need to override it. Reply by Behrouz NematiPour on 06 May 2021, 15:40 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/SettingsAlarmVolume.qml Revision Comment by Behrouz NematiPour on 29 April 2021, 09:52 https://devapps.diality.us/cru/UI-DEN-8085-1#c9652 Could you please explain why do you need this flag? Reply by plucia on 29 April 2021, 10:09 > It prevents updates to alarmVolume by the HD broadcast while > we're editing the alarm volume. > Otherwise, alarmVolume could be incorrectly updated by an HD > broadcast of a set alarm volume that the user decided to > cancel. Reply by Behrouz NematiPour on 29 April 2021, 10:23 > Interesting. > Why it is a broadcast instead of a request/response? > Why does UI need a broadcast message while it will always > be adjusted by UI? Reply by plucia on 04 May 2021, 12:33 > It is removed now Reply by Behrouz NematiPour on 06 May 2021, 16:41 > RESOLVED ---------------------------------------- File: sources/view/VAlarmVolume.h Revision Comment by Behrouz NematiPour on 29 April 2021, 09:55 https://devapps.diality.us/cru/UI-DEN-8085-1#c9653 What is Temp mean, if it means default please put def, and if it is the step please put Step. Or if it is the previous value you kept for back to be sent back to HD, Please use prev/previous. Reply by plucia on 29 April 2021, 09:59 > alarmVolumeTemp is the temporary volume being selected by the > user while they are adjusting. > alarmVolume is the kept alarm volume when the user exits the > alarm volume screen. > alarmVolume is set to alarmVolumeTemp only if the user hits > confirm > Upon confirm, alarmVolume is updated to be the same value as > alarmVolumeTemp and is then sent down to the HD. > Upon back / cancel, alarmVolume is left unchanged and is sent > down to the HD to undo the temporary setting > FW does not expose a play alarm volume without set message so > we have to set the volume each time it is played for the user > while they are adjusting. Reply by plucia on 04 May 2021, 12:34 > AlarmVolumeTemp has been removed Reply by Behrouz NematiPour on 06 May 2021, 18:28 > RESOLVED ---------------------------------------- File: scripts/brightness_set.sh Revision Comment by Behrouz NematiPour on 29 April 2021, 10:47 https://devapps.diality.us/cru/UI-DEN-8085-1#c9664 Was there a reason the tee command is being used, instead of simply using > like : {code}echo "$1" > /sys/class/backlight/backlight-mipi/brightness{code} ? Reply by plucia on 04 May 2021, 13:06 > That works too. No need for tee here. I've updated it Reply by Behrouz NematiPour on 06 May 2021, 16:28 > RESOLVED ---------------------------------------- File: scripts/setup.sh Revision Comment by Behrouz NematiPour on 29 April 2021, 10:50 https://devapps.diality.us/cru/UI-DEN-8085-1#c9665 please also use the 'a' not only it is consistent with most of the cases but also makes it executable for other user groups(if any). Reply by plucia on 04 May 2021, 14:53 > Done Reply by Behrouz NematiPour on 06 May 2021, 16:28 > RESOLVED ---------------------------------------- File: sources/view/VBrightness.cpp Revision Comment by Behrouz NematiPour on 29 April 2021, 11:23 https://devapps.diality.us/cru/UI-DEN-8085-1#c9676 The one which has been used in the VDateTime is much cleaner: {code} if (_process.state() == QProcess::Running) { return; } {code} Reply by plucia on 04 May 2021, 12:54 > I think VDateTime.h is less safe in this case. It should be > updated to match > {code}if (_process.state() != QProcess::NotRunning) { return; > }{code} > to prevent re-starting the process when its already in the > "Starting" state as well as the "Running" state. > {code} > enum ProcessState { > NotRunning, > Starting, > Running > }; {code} Reply by Behrouz NematiPour on 06 May 2021, 18:39 > RESOLVED Revision Comment by Behrouz NematiPour on 29 April 2021, 10:56 https://devapps.diality.us/cru/UI-DEN-8085-1#c9667 Please don't use "%0" as we discussed. Reply by plucia on 04 May 2021, 14:38 > Fixed Reply by Behrouz NematiPour on 06 May 2021, 18:39 > RESOLVED --- ID: UI-DEN-8085-1 https://devapps.diality.us/cru/UI-DEN-8085-1 Title: UI-DEN-8085_Alarm Volume Brightness Statement of Objectives: State: Closed Summary: Author: plucia Reviewers: (0 active, 2 completed*) Behrouz NematiPour (*) pmontazemi (*)