•  

Comment Results

Review Name Created Custom Fields Content
UI-DEN-3605-4 06 Oct 2020

When I tested this Qt did not run the helper functions as test functions. It appears to be able to differentiate between the two without any additional directives.

As requested, I've made the helper functions private instead of private slots to make it absolutely clear which functions are helper functions and which aren't.

DIALIN-DEN-4690-1 05 Oct 2020

Ultrafiltionation?

UI-DEN-4690-1 07 Oct 2020

Doesn't indicate when it returns true

UI-DEN-3605-4 07 Oct 2020

Here's the bug:

http://dvm-linux02:8080/browse/DEN-5281

Removed, will post update soon

UI-DEN-3605-4 07 Oct 2020

RESOLVED.

UI-DEN-4690-1 07 Oct 2020

What do you mean by :
"if init() changes, this function will behave differently, but it won't be reflected in the documentation"
the function document says the same thing:
"returns the return value of the init() method"
so if init() changes this function return changes because it's an overloaded method of the init() function.

DIALIN-DEN-4308-1 08 Oct 2020

RESOLVED

DIALIN-DEN-4690-1 07 Oct 2020

This should be removed

UI-DEN-4438-1 25 Aug 2020

Absolutely,
I'm doing it incrementally and slowly to not to make the code reviews harder and confusing git.
I did for so many and this file will get its turn soon.

UI-DEN-4438-1 25 Aug 2020

We had discussed the namespace indentation in another comment and resolved.
"This is the namespace brace and the namespaces braces are not indenting the code. so are at the same column as class brace in this case."

DIALIN-DEN-4438-1 25 Aug 2020

I tried to remove it but seems like Jira doesn't listen.
sadly after we discussed in a conversation we had that Sarina arranged, when you told me to merge the staging branch into my working branch I didn't envision at that moment Jira doesn't let us remove it after it has been added.
Fortunately what we currently can do is to only review files in dialin/squish folder.

Thanks,

UI-DEN-4438-1 25 Aug 2020

This is the namespace brace and the namespaces braces are not indenting the code. so are at the same column as class brace in this case.

HD-DEN-4308-3 29 Aug 2020

Could you please be more specific?

LEAHI-TESTSUITES-LDT-1218-1 16 Oct 2025

Aligned

DIALIN-DEN-3421-1 10 Dec 2020

Done

HD-DEN-4308-3 22 Sep 2020

Changed to self.

HD-DEN-4308-3 29 Aug 2020

If the number of homing was exceeded and we failed, we go back to Idle, but the valve cannot go anywhere. It cannot receive any command to go to any of the positions. When the device turned on, we go straight to homing not started state though.

DIALIN-DEN-3421-1 10 Dec 2020

Done

HD-DEN-4211-1 31 Aug 2020

RESOLVED in CODE WALKTHROUGH.

DG-DEN-3504-1 30 Oct 2020

Why removed?

HD-DEN-4308-3 01 Sep 2020

Done

UI-DEN-3605-4 30 Sep 2020

Just out of curiosity why used state instead of signal/slots?

HD-DEN-4308-3 01 Sep 2020

Copyright will be automatically added in the Bamboo build.

UI-DEN-3605-4 09 Sep 2020

This code review started > 2 weeks ago and since then the message list document was updated as per Sean's requested changes which I've been working on in a separate branch and story.

DIALIN-DEN-4211-1 01 Sep 2020

RESOLVED.

DIALIN-DEN-4211-1 01 Sep 2020

RESOLVED.

DG-DEN-4322-1 25 Aug 2020

The code in this header file should be added to the SystemCommMessages group for now. This header file will be going away at some point and its definitions distributed to other header files.

DG-DEN-4322-1 25 Aug 2020

There are more public functions below this line. I generally prefer public functions on top and private function below, but this is not required and would conflict with other considerations like keeping Dialin functions (which are public) at very bottom of module. Recommend removing this comment.

DG-DEN-4322-1 25 Aug 2020

These belong in the 0x8... section above (for Dialin/HD messages.

HD-DEN-4308-3 01 Sep 2020

Added a flag that is set to true if homing fails. This way if homing failed, the state will go back to homing not started and not idle.

UI-DEN-3605-4 30 Sep 2020

Please remove this debug codes or comment them as // TODO to be removed ASAP.
There are more of these which I didn't put comment.
Please apply to all.

HD-DEN-4308-3 22 Sep 2020

Done

HD-DEN-4308-3 05 Oct 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-4308-3 22 Sep 2020

When will we un-comment the for loop?

UI-DEN-3605-4 11 Sep 2020

Yes of course, this is a very old comment made early on during development. See the newer version of this file, guiglobals.h, in the files list to the left where this comment is no longer present. That should match exactly with the branch you checked out.

HD-DEN-4308-3 05 Oct 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-4308-3 23 Sep 2020

RESOLVED in CODE WALKTHROUGH.

UI-DEN-3605-4 05 Oct 2020

That is obvious that calling multiple layers of functions on each user slider movement and value change.
Let's say we have a slider in range of -100 to +700 (which is a valid actual range in PRS) with resolution of 1 and user easily can drag the slider, then imagine what amount of call will happen for a single user slide.
It may not currently visibly show a slow down but later by adding more into the code it may cause us problems.
The SRSUI doesn't force the design it's up to designer and developer to meet the requirement while keeps the code performance high.
If it's absolutely necessary the please add a release signal and handle it at that time.

HD-DEN-4308-3 23 Sep 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-4308-3 04 Sep 2020

If payload is 2 U32s, why is memcpy below only copying one U32? Also, why is payload typed as VALVE_POSITION_T? So I see 3 different versions of what the payload is here.

HD-DEN-4308-3 23 Sep 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-4308-3 23 Sep 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-4308-3 23 Sep 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-5053-1 23 Sep 2020

I thought we are not going to have a space after Input/Output and ":".

UI-DEN-3605-4 30 Sep 2020

If it's going to be addressed that's fine.
RESOLVED.

UI-DEN-3605-4 06 Oct 2020

RESOLVED

UI-DEN-3605-4 06 Oct 2020

Found in another Variables.qml.
RESOLVED.

UI-DEN-3605-4 30 Sep 2020

http://192.168.10.132:8060/cru/UI-DEN-3605-4#c5065

DIALIN-DEN-4690-1 07 Oct 2020

RESOLVED.

DIALIN-DEN-4690-1 05 Oct 2020

Misspelling in all three lines for word ultrafiltration