DiamondLightSource / cs-web-lib

Library form of control system web UI prototype
Apache License 2.0
1 stars 1 forks source link

Update .bob parsing and widget defaults #52

Closed abigailalexander closed 7 months ago

abigailalexander commented 8 months ago

I made several updates to widgets to reflect Phoebus default and behaviours

In addition I have updated the bob parsing, so all necessary props for existing widgets can be parsed in both .bob and .opi.

abigailalexander commented 8 months ago

Additional changes:

rjwills28 commented 8 months ago

Thanks for this PR. I tested by creating a BOB screen and a OPI screen with these different widgets. I compared the display in the web UI vs Phoebus/CSS. I found a few difference which I list below with examples:

  1. ActionButton: When I dragged an ActionButton widget from the palette in Phoebus to the screen and did not resize it (i.e. left it as default size), the widget rendered very big in the web UI ( see left in image below). The size was OK after I resized it in Phoebus (right in image below). actionButton
  2. Arc: The Arc widget default size is also very big (as above, when dragged from palette in Phoebus and not resized). Shown below on left in image below. When arc is resized it is OK (right). arc
  3. Arc: The arc widget outline does not surround the whole shape as it does in Phoebus (see above image). This behaviour is different in CSS (CSS matches the current web UI where it is just around the arc).
  4. BoolButton: When the BoolButton widget is dragged from the palette in Phoebus and left as the default size it displays strangely in the web UI where the LED and button aren't joined (see below, far left). boolbutton
  5. BoolButton: After resizing the BoolButton so that it displays correctly you can see that it puts the label on the left of the LED, whereas Phoebus puts it on the right (see middle 'BoolButton' in above image). Note that CSS puts it on the left as the web UI currently does so behaviour does differ between them.
  6. BoolButton: When the property 'label from PV' is true it should take the 'OFF' label from the PV name (see middle Phoebus BoolButton in image above). However this does not work in the web UI for BOB files or OPI files as you can see it still displays the text 'Offf', which has come from the BOB file.
  7. BoolButton: When the 'Off label' is left empty the web UI still displays 'OFF', which differs from Phoebus which leaves it blank (see far right in the image above). This difference is also true of OPIs in the web UI vs CSS.
  8. ByteMonitor: The square format of the ByteMonitor does not work in the BOB file, see below. OK for OPIs. bytemonitor
  9. Polygon: I noticed (by mistake) that if there are no points in the polygon then the BOB file fails to load with the error that the 'points' property is required but is undefined. This might be the intended behaviour but in Phoebus such a polygon can exist in the editor and it just not rendered at runtime.
rjwills28 commented 8 months ago

Unrelated to changes made in this PR but I did notice in the Firefox console that a warning is thrown from the opiParser on line 560 where it complains that props.points is undefined. I think line https://github.com/dls-controls/cs-web-lib/blob/b8dfe4e4f8d2707255e615b7fcfd01a2502e0070/src/ui/widgets/EmbeddedDisplay/opiParser.ts#L560 needs changing from: props.points.point.forEach((point: any) => { to props.points?.point.forEach((point: any) => {

rjwills28 commented 8 months ago

Finally, I wasn't entirely sure how to test the EmbeddedDisplay resize property. I added it to the JSON file and changed the value between 1,2,3,4 but didn't notice a particular change in the web UI. Was I testing this incorrectly?

abigailalexander commented 8 months ago

Thanks for pointing those out Becky! These last few commits should fix most of the issues you raised, with the exception of 7 - it seems like cs-web-lib is discarding props that are empty strings, which is why instead of BoolButton receiving the value from the .bob file of "", it instead uses the default "OFF" value set in the widget component.

Do we want to change the behaviour to allow for empty strings to be used as props? It seems like in this use case it makes sense.

Also regarding the embeddedDisplay resize change, I previously tested it by having a .bob file with an embeddedDisplay widget in the screen like so:

<widget type="embedded" version="2.0.0">
    <name>Embedded Display_1</name>
    <file>my_file.bob</file>
    <x>30</x>
    <y>550</y>
  </widget>

It could be that the changes I made aren't supported well by json files, so I'll look into that as well.

rjwills28 commented 8 months ago

Thanks for the fixes Abii. I've picked up a few bits relating to these changes.

  1. The behaviour between CS-Studio and Phoebus differs for the widgets below. Are we mandating that they always follow the Phoebus behaviour in the web UI or do we need to handle them differently in the opi & bob parsers?
    1. Arc: in CSS the border does not go around the whole shape as it does in Phoebus (and now does in the web UI) - instead only shows around the arc.
    2. BoolButton: the label is now to the right of the LED in the web UI (as it is in Phoebus) but in CSS the label is displayed to the left.
  2. My BOB file failed to load as the web UI claimed the ByteWidget required both the width and height properties. In Phoebus you do not necessarily require these if using the default size so it is possible that these are missing from the widget and the web UI should allow it and use some default values.
  3. In the BOB file, the BootButton shows the LED and label very close together. This gets worse if the text is longer. See below: Phoebus ^button_space_ph
    Web UI >button_space_web
  4. If the BoolButton label is left as the default then the web UI seems to use OFF and ON all in uppercase whereas Phoebus uses sentence case (see above images displaying 'OFF' vs 'Off').

The two remaining issues are:

  1. Empty string in the BoolButton label. I feel like we probably should follow what Phoebus does and allow this although I appreciate it does mean a fairly central change to the web UI parsing.
  2. 'Value from PV' does not work for BoolButton labels - neither in CSS or Phoebus.

And finally, thank you for the tip on testing the resize option - I've been able to test that now.

abigailalexander commented 7 months ago

Thanks for your comments Becky. I think we're planning to prioritise Phoebus appearance, as the plan is to use .bob files in the Web UI. I've hopefully now sorted the issues with the led on the BoolButton widget, and allowed empty string parsing for property fields which could deliberately be left empty. This list of "allowed empty" widgets is stored as a list in the parser.ts file.

The ByteMonitor height and width can be undefined now as well - I had accidentally changed them from optional props to mandatory. Thank you again for your help in reviewing these widget changes

rjwills28 commented 7 months ago

Thanks for the further changes Abii. The LED position on the BoolButton now looks great and scales appropriately similar to as it does in Phoebus. Defaults also look good now. It is just the 'Labels from PV' on the BoolButton that don't appear to be working in the web version but I think this is a feature that has never been implemented (neither in the opiParser) and may be a bit more involved so do you think it would be fair to call that a separate issue and merge these current changes?