CSSTUDIO-3538 Bugfix: Redraw Linear Meter widget when the range or alarm limits are updated #3649
Conversation
…arm limits are updated.
abrahamwolk
requested review from
georgweiss,
jacomago,
jasonbrudvikess,
kasemir,
marcoleorato-ess and
shroffk
jacomago
left a comment
jacomago
left a comment
There was a problem hiding this comment.
Some suggestions on how I might refactor.
| } | ||
|
|
||
| private void drawNewValue(double newValue) { | ||
| private void drawNewValue(double newValue, boolean forceRedraw) { |
There was a problem hiding this comment.
Shouldn't this method be renamed now?
There was a problem hiding this comment.
No: the method draws a new value. Before this pull request, it was "optimized too much": drawNewValue() only actually drew a new value if it determined that the indicator had moved at least one pixel, which didn't account for the case where the range or the alarm limits had changed.
| private double valueWaitingToBeDrawn = Double.NaN; | ||
| /** @param newValue Current value */ | ||
| public void setCurrentValue(double newValue) | ||
| public void setCurrentValue(double newValue, boolean forceRedraw) |
There was a problem hiding this comment.
I'm not a fan of boolean flags, I would write an enum
enum DrawAction {
ForceRedraw
Draw
NoRender
}
or something like this.
There was a problem hiding this comment.
An enum could be used, but since the data is still essentially only true and false and the parameter name of meter.setCurrentValue() describes the meaning well, I prefer to use a boolean.
| && controlRange.getMaximum() - controlRange.getMinimum() > 0.0) { | ||
| if (meter.linearMeterScale.getValueRange().getLow() != controlRange.getMinimum() || meter.linearMeterScale.getValueRange().getHigh() != controlRange.getMaximum() || !meter.getValidRange()) { | ||
| meter.setRange(controlRange.getMinimum(), controlRange.getMaximum(), true); | ||
| linearMeterScaleHasChanged = true; |
There was a problem hiding this comment.
This makes me feel like this should be a property of the linearmeter, so this should be something like:
meter.setScaleUpdated()
There was a problem hiding this comment.
I disagree: I prefer explicit passing of values (in contrast to implicit passing of values through update of state). In particular, linearMeterScaleHasChanged is passed to meter.setCurrentValue() on line 386: https://github.com/ControlSystemStudio/phoebus/pull/3649/files/c37e695e063c8014201fceb374da86198dbf2aa3#diff-99f66c9c7631b64fc8906945f212e452266b923ac7c5a7214eb7f604df210cbfR386
This pull request fixes a bug in the Linear Meter: before this pull request, the Linear Meter would only be redrawn when the indicator position had changed at least one pixel. In particular, an update to the range or to the alarm limits in the connected PV did not force a redrawing of the Linear Meter widget.
I have tested the bugfix manually using a soft IOC.