This is a list of all comments for UI-DEN-7035-1.

Review Summary: No summary


----------------------------------------
File: scripts/date_time_set.sh
Revision Comment by Behrouz NematiPour on 24 March 2021, 10:36 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8702

  Please put more description of why we should have these two
  lines?

  Reply by plucia on 24 March 2021, 13:46
  > From my design meeting notes I have it written down that we
  > will always be in pacific time as no other timezones will be
  > supported. So, these lines just force the timezone to PST.
  > I've moved this to setup.sh since it really only needs to be
  > run once. Then later more work can be done to add support for
  > timezone adjustment.

    Reply by Behrouz NematiPour on 24 March 2021, 15:30
    > No problem with what has been done here, just please add
    > some description on top to make it clear it is regarding
    > that.
    > echo out is clear too.
    > RESOLVED.

Revision Comment by Behrouz NematiPour on 24 March 2021, 10:37 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8703

  couldn't we use the date command instead which is more common?

  Reply by plucia on 24 March 2021, 13:45
  > Sure, I've updated it to use the date command.

    Reply by Behrouz NematiPour on 24 March 2021, 15:32
    > Great.
    > RESOLVED

----------------------------------------
File: sources/gui/qml/pages/SettingsDateTimeSet.qml
Revision Comment by Behrouz NematiPour on 24 March 2021, 11:19 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8708

  Please give a better name to this rect. like _contentRect so it
  gives the idea of why it has been defined.

  Reply by plucia on 24 March 2021, 14:19
  > Done

    Reply by Behrouz NematiPour on 24 March 2021, 15:33
    > RESOLVED

Revision Comment by Behrouz NematiPour on 24 March 2021, 11:20 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8709

  - since you are setting the bottom margin we need to think
  about the object that are showing up on the bottom of the
  screen and since the mainMenu comes on top then it's height may
  not be relevant to the bottom of the screen area.
  Could be better to use notificationHeight which is used for the
  notifications that are showing up at the bottom.
   - why it has been used by twise the size?

  Reply by plucia on 24 March 2021, 14:24
  > Twice the size of the main menu height (140) happened to
  > leave a good sized margin between minimized alarm + main menu
  > and the save button. 
  > I've updated it to be defined more clearly so the
  > notification height (60) + the main menu height (70) +
  > settingsNotificationMargin (10) decides the bottomMargin
  > instead of just 2x the main menu height.

    Reply by Behrouz NematiPour on 24 March 2021, 16:09
    > RESOLVED

Revision Comment by Behrouz NematiPour on 24 March 2021, 12:07 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8714

  I see that you nicely put the _keyboard.setVisible(true) in the
  TextEntry component.
  Don't you think it would be better to put
  _keyboard.setVisible(false) on onEnteredPressed in the
  TextEntry component as well, somewehere next to the lines
  below?
  {code}
  onFocusChanged: {
      if (focus) {ome whereo
          selectAll()
          _keyboard.setVisible(true)
      }
  }
  
  onAccepted: {
      _root.enterPressed()
  } 
  {code}

  Reply by plucia on 24 March 2021, 14:31
  > There is one use case with the manual BP / HR entry where the
  > keyboard is kept visible after the user presses 'Enter'. This
  > allows the focus to move from systolic -> diastolic -> HR
  > automatically without causing it to flash invisible and then
  > visible. So at the time it was left it up to the developer of
  > the component to decide when to minimize the keyboard in case
  > it would need to stay up as they kept moved between input
  > fields. 
  > 
  > If we remove automatically transitioning from one input field
  > to the next in the manual BP / HR entry pop up, then I'd say
  > there's no reason not to hide the keyboard inside the
  > TextEntry component when Enter is pressed.

    Reply by Behrouz NematiPour on 24 March 2021, 15:35
    > Good Point.
    > Isn't the BLE case apply everywhere?
    > I was thinking why we even want to hide the keyboard while
    > users can hide it by the keyboard button on the keyboard.
    > We may give it more thoughts.
    > RESOLVED

----------------------------------------
File: sources/gui/qml/pages/SettingsHome.qml
Revision Comment by Behrouz NematiPour on 24 March 2021, 11:51 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8712

  Do we need to set the background color?
  I think the default ScreenItem backgroundColor would be fine!

  Reply by plucia on 24 March 2021, 14:44
  > Sounds good, I've removed setting the background color

    Reply by Behrouz NematiPour on 24 March 2021, 15:39
    > RESOLVED

Revision Comment by Behrouz NematiPour on 24 March 2021, 11:52 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8713

  Why onPressed?
  Should be onClicked.

  Reply by plucia on 24 March 2021, 14:46
  > Done

    Reply by Behrouz NematiPour on 24 March 2021, 15:39
    > RESOLVED

----------------------------------------
File: sources/view/VDateTime.cpp
Revision Comment by Behrouz NematiPour on 24 March 2021, 10:57 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8705

  - QStringLiteral should be used for a constant string that we
  know is not going to change so those will be created at compile
  time and not every time.
     Here seem all the options have to run and build at runtime.
   - Why used QLatin1Char instead of only "0"?
     This is a constant leading zero.

  Reply by plucia on 24 March 2021, 15:24
  > These were created early on but they are no longer being used
  > so I've now deleted them.

    Reply by Behrouz NematiPour on 24 March 2021, 15:41
    > RESOLVED

Revision Comment by Behrouz NematiPour on 24 March 2021, 11:03 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8707

  This process is called in the main thread (GUI thread), and any
  issue will freeze the UI.
  Let's talk about it before any decision.
  I need to know more about the code structure.

  Reply by Behrouz NematiPour on 24 March 2021, 16:13
  > We talked about a settingcontroller to be implemented later.
  > RESOLVED

Revision Comment by Behrouz NematiPour on 24 March 2021, 11:30 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8710

  Please use a consistent "else" style.
  we are currently using Stroustrup style which is like:
  {code}
  }
  else {
  {code}
  or use something like:
  {code}
  } else {
  {code}

  Reply by plucia on 24 March 2021, 14:40
  > Done

    Reply by Behrouz NematiPour on 24 March 2021, 15:41
    > RESOLVED

----------------------------------------
File: sources/view/VDateTime.h
Revision Comment by Behrouz NematiPour on 24 March 2021, 10:42 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8704

  Did you do the Code Coverage?
  If not please remove these lines which I think are the
  leftovers of copy/paste.
  so later we test them.

  Reply by plucia on 24 March 2021, 14:17
  > Done

    Reply by Behrouz NematiPour on 24 March 2021, 15:42
    > RESOLVED

----------------------------------------
File: sources/model/hd/adjustment/MAdjustHDDateTimeResponse.h
Revision Comment by Behrouz NematiPour on 24 March 2021, 11:44 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8711

  Recently I'm removing the "Response" from the log's "infoText"
  function, since the log already has the source we will see it
  is coming from HD/DG and it make the "Response" useless and
  gets more space only.
  Please apply the same for the DG.
  And don't forget to update the doxygen comment as well.

  Reply by plucia on 24 March 2021, 14:41
  > Done

    Reply by Behrouz NematiPour on 24 March 2021, 15:40
    > RESOLVED

----------------------------------------
File: sources/model/hd/adjustment/MTreatmentAdjustRequests.h
Revision Comment by pmontazemi on 22 March 2021, 11:20 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8460

  No Msg Box?

  Reply by plucia on 24 March 2021, 13:21
  > The mail box has been removed from all message actually, as
  > they are no longer needed. I've now removed it from the
  > docstring here as well

    Reply by pmontazemi on 24 March 2021, 16:03
    > RESOLVED.

Revision Comment by pmontazemi on 22 March 2021, 11:20 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8461

  No Msg Box?

  Reply by plucia on 24 March 2021, 13:25
  > Same as above

    Reply by pmontazemi on 24 March 2021, 16:04
    > RESOLVED.

----------------------------------------
File: sources/gui/qml/globals/Variables.qml
Revision Comment by Behrouz NematiPour on 24 March 2021, 10:29 
https://devapps.diality.us/cru/UI-DEN-7035-1#c8694

  Wouldn't be better to define just one set of common
  "settings..." variables and bind the others to that instead of
  having a chain of binding?
  Preffered:
  {code}
  settings<1>: 85
  settings<2>: 75
  
  settingsBLE<1>:settings<1>
  ....
  settingsWifi<1>: settings<1>
  ...
  {code}
  instead of the chain of:
  {code}
  settings<1>: 85
  settings<2>: 75
  
  settingsBLE<1>:settings<1>
  ....
  settingsWifi<1>: settingsBLE<1>
  ...
  {code}

  Reply by plucia on 24 March 2021, 14:16
  > Sure, I've updated it to remove the chain of bindings

    Reply by Behrouz NematiPour on 24 March 2021, 15:33
    > Thanks,
    > RESOLVED


---

ID: UI-DEN-7035-1 https://devapps.diality.us/cru/UI-DEN-7035-1

Title: UI-DEN-7035_Date Time Set
Statement of Objectives:


State: Closed

Summary: 

Author: plucia
Reviewers: (2 active, 0 completed*)
     Behrouz NematiPour
     pmontazemi