This is a list of all comments for UI-DEN-4860-BLE-1. Review Summary: No summary General Comment by plucia on 30 December 2020, 15:15 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7095 As of today, 12.30.2020: Code Coverage: 100% SquishQt tests: 100% passing Unit Tests: 100% passing ---------------------------------------- File: sources/gui/qml/dialogs/NotificationDialog.qml Revision Comment by Behrouz NematiPour on 12 January 2021, 18:54 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7278 please don't use the clip, it is not recommended. Also, there should not be an issue with the silent button, it has been tested! What are you trying to achieve? Reply by plucia on 13 January 2021, 14:44 > This isn't a change I had made. > You added this in cd769413344091cea88a30861b49188c8c147cba > I don't know why it's showing up here. Seems like crucible > messed up > Do you want me to remove it anyway? Reply by Behrouz NematiPour on 18 January 2021, 16:38 > Please note that the code review has to be automatic, by > making it manual and mixed up with other features, this may > happen. > RESOLVED Revision Comment by Behrouz NematiPour on 12 January 2021, 18:57 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7279 the coverage is done without these object names on the Alarm design branch please remove them. Reply by plucia on 13 January 2021, 14:16 > This objectName is necessary for the added alarm tests to > work, which also guarantees 100% code coverage on my branch > It is very helpful for Systems / SW V&V to have as many > object names defined so they can write their own squishqt > tests > In order for Systems / SW V&V to interact with the alarms and > so the alarm tests can function, I think it should be kept Reply by Behrouz NematiPour on 18 January 2021, 16:41 > When we add the correct object dictionary in the names.py > then sys/v&v/... can use it and don't need the object name > anymore to do so. > Not 100% agree. > Also, we need to have a consistent naming for the currently > existing objectName in the code, I'll later provide a > standard. > > RESOLVED Revision Comment by Behrouz NematiPour on 12 January 2021, 18:58 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7280 same Reply by plucia on 13 January 2021, 14:34 > See response above Reply by Behrouz NematiPour on 18 January 2021, 16:42 > RESOLVED Revision Comment by Behrouz NematiPour on 12 January 2021, 18:58 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7281 same Reply by plucia on 13 January 2021, 14:34 > See response above Reply by Behrouz NematiPour on 18 January 2021, 16:42 > RESOLVED Revision Comment by Behrouz NematiPour on 12 January 2021, 18:59 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7282 same Reply by plucia on 13 January 2021, 14:34 > See response above Reply by Behrouz NematiPour on 18 January 2021, 16:42 > RESOLVED Revision Comment by Behrouz NematiPour on 12 January 2021, 18:59 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7283 same Reply by plucia on 13 January 2021, 14:34 > See response above Reply by Behrouz NematiPour on 18 January 2021, 16:42 > RESOLVED ---------------------------------------- File: sources/bluetooth/BLEScanner.cpp Revision Comment by Behrouz NematiPour on 12 January 2021, 18:32 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7275 QObject has a built-in time. override the : void ::timerEvent(QTimerEvent *) \sa MainTimer Reply by plucia on 13 January 2021, 15:52 > Done Reply by Behrouz NematiPour on 18 January 2021, 16:52 > RESOLVED Revision Comment by Behrouz NematiPour on 18 January 2021, 16:45 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7505 I am not sure killing the QObject timer would be a good idea, but also I couldn't find any recommendation not to do it. Since any object has only one timer I have a feeling that by killing the object's only timer, then any refreshing or chores for any kind of signal/slot maintenance will be stoped. I prefer to have a boolean flag to return from the virtual method in case we don't need it to handle the functionality we put there. Reply by plucia on 18 January 2021, 17:06 > {quote} > Since any object has only one timer I have a feeling that by > killing the object's only timer, then any refreshing or > chores for any kind of signal/slot maintenance will be > stoped. > {quote} > > That's the intention - to stop the timer. Since the UI is > continually trying to reconnect to the last paired device > while it is off, if a user goes into the bluetooth settings > page and hits "Scan for devices", the timer is stopped during > the scan to avoid conflicts between the timer and the scan. > > {quote} > I prefer to have a boolean flag to return from the virtual > method in case we don't need it to handle the functionality > we put there. > {quote} > > This doesn't make sense to me, can you clarify? Reply by Behrouz NematiPour on 18 January 2021, 17:29 > 1 - I mean that timer is not only for handling our > implementation it may have another purpose which we are > stopping by killing the only timer it has. > 2 - in your > {code} > timerEvent { > ... > if( ! _timerIsActive ) return; > ... > } > {code} Reply by plucia on 20 January 2021, 11:57 > Done Reply by Behrouz NematiPour on 26 January 2021, 01:42 > RESOLVED ---------------------------------------- File: sources/gui/qml/dialogs/Alert.qml Revision Comment by Behrouz NematiPour on 13 January 2021, 11:42 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7310 Why this specific dialog has been created and not NotificationDialog used and what is special about that? Please provide a screenshot to compare. Reply by plucia on 13 January 2021, 14:39 > Having a separate component prevents alerts from covering up > alarms and ensures the alarm takes precedence Reply by Behrouz NematiPour on 18 January 2021, 16:35 > It's not the components themselves that do what you want > it's how you call and manage it and having another > component was unnecessary. > RESOLVED regardless. ---------------------------------------- File: scripts/run.sh Revision Comment by Behrouz NematiPour on 13 January 2021, 11:30 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7308 why this folder has to exist and why this naming has been chosen? Reply by plucia on 13 January 2021, 16:06 > This folder contains treatment related information such as > parameter ranges as well as paired BLE devices. > Would you prefer we store the bluetooth devices in a separate > folder called bluetooth instead? Reply by Behrouz NematiPour on 14 January 2021, 16:40 > We need to have a folder next to the application for all > the settings since it is not related to the treatment it is > only settings configuration files. > Since the SD card might be damaged and we still need to > store settings. > Please change it to refer to the *application location* and > name the folder as *configuration* or *settings*. Reply by plucia on 15 January 2021, 10:34 > Done Reply by Behrouz NematiPour on 18 January 2021, 16:37 > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 11:29 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7307 Please put a comment about why sleep 5 has been used and what needs to be done to remove this sleep. Reply by plucia on 13 January 2021, 16:27 > Done Reply by Behrouz NematiPour on 14 January 2021, 16:43 > Perfect, > RESOLVED. Revision Comment by Behrouz NematiPour on 13 January 2021, 11:27 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7306 By connmand did you mean command? Is this correct? and if it is what is that? Does it mean connection manager daemon? Reply by plucia on 13 January 2021, 15:22 > "connmand" is correct > It's used to manage internet connections on embedded devices > and causes problems if it is already running while bluetooth > is starting up Reply by Behrouz NematiPour on 14 January 2021, 16:44 > So we may need to be careful when we are adding the WiFi > driver in here later. > RESOLVED. ---------------------------------------- File: denali.pro Revision Comment by Behrouz NematiPour on 13 January 2021, 11:47 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7313 Please move it after # CANBus section. Reply by plucia on 13 January 2021, 15:18 > Done Reply by Behrouz NematiPour on 18 January 2021, 16:33 > RESOLVED Revision Comment by Behrouz NematiPour on 13 January 2021, 11:47 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7314 same Reply by plucia on 13 January 2021, 15:19 > Done Reply by Behrouz NematiPour on 18 January 2021, 16:33 > RESOLVED ---------------------------------------- File: resources/images/alert.png Revision Comment by Behrouz NematiPour on 13 January 2021, 11:32 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7309 Why this color has been chosen, I don't see it in the UX design? Also, we already have the same icon with a white background. Reply by plucia on 13 January 2021, 15:20 > It's in the UX design here: > https://app.zeplin.io/project/5e160353a7c41a9404596a70/screen/5e5060d3f75589612aee8b68 Reply by Behrouz NematiPour on 14 January 2021, 16:39 > Oh I see. > RESOLVED ---------------------------------------- File: sources/gui/qml/components/ScreenItem.qml Revision Comment by Behrouz NematiPour on 12 January 2021, 18:39 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7276 This component is so general and not all the use cases need a back button. Please remove it from here and move it somewhere more suitable. Reply by plucia on 13 January 2021, 16:46 > Done Reply by Behrouz NematiPour on 15 January 2021, 11:47 > RESOLVED ---------------------------------------- File: sources/gui/qml/components/SettingsItem.qml Revision Comment by Behrouz NematiPour on 12 January 2021, 18:53 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1#c7277 Please Simply use the iArrow. why rotating two line to shape an arrow ?! Reply by plucia on 13 January 2021, 16:38 > Done - Updated to use iArrow now Reply by Behrouz NematiPour on 14 January 2021, 20:22 > RESOLVED --- ID: UI-DEN-4860-BLE-1 https://devapps.diality.us/cru/UI-DEN-4860-BLE-1 Title: UI-DEN-4860_BLE Statement of Objectives: State: Closed Summary: Author: plucia Reviewers: (0 active, 2 completed*) Behrouz NematiPour (*) pmontazemi (*)