kasemir / org.csstudio.display.builder

Update of org.csstudio.opibuilder.*
Eclipse Public License 1.0
2 stars 10 forks source link

Missing labels in ByteMonitor #438

Closed claudio-rosati closed 6 years ago

claudio-rosati commented 6 years ago

My colleagues are complaining about the impossibility to add labels to the ByteMonitor, thing possible in BOY.

From what I've seen, in all the cases they are using a PV providing a (static) array of strings. So I'm thinking to 2 possible alternative implementations:

I personally like the second option, that leaves the ByteMonitor untouched, and allows for placing the labels on both sides of a ByteMonitor.

@kasemir What do you think about?

claudio-rosati commented 6 years ago

Hello @kasemir I've merged your changes on this issue, and I think many of my users will appreciate it. Thank you. I'm just waiting to have a valid PV to test it and I'll close the issue.

claudio-rosati commented 6 years ago

Hello @kasemir Importing is ByteMonitor with labels doesn't work as expected. Here the BOY example:

screenshot 2018-10-30 at 13 59 11

and here the imported one:

screenshot 2018-10-30 at 14 00 57

The main problems are the following:

I'll patch it: #464

kasemir commented 6 years ago

Worked for me, the case that had to be handled was using the "square" option and "reversed bits":

a

b

Can you check if that still works with your changes?

claudio-rosati commented 6 years ago

Now yes:

screenshot 2018-10-30 at 15 05 25

screenshot 2018-10-30 at 15 14 48

claudio-rosati commented 6 years ago

@kasemir In my original JIRA issue I've found the following:

BOY ByteMonitor allows for labels to be specified. It is a property of the widget and its type is String[], allowing to be set via rule directly from PV. This feature is missing in Display Builder's ByteMonitor widget.

Clearly the bold sentence cannot be done in Display Builder. I'll add the bit index to the label name. Do you think it can help?

claudio-rosati commented 6 years ago

Probably it is not worth: the name should contain also the name of the ByteMonitor itself, thus becoming too long and not really good for script/rule manipulation.

kasemir commented 6 years ago

I've reconsidered: Instead of converting .opi files into .bob where label widgets are added to the bye monitor, I'll add a "labels" property to the byte monitor as in BOY.

claudio-rosati commented 6 years ago

OK, that will solve also the possibility of updating the labels as a whole from PV. Thank you

claudio-rosati commented 6 years ago

I'm sorry @kasemir but I don't like the new implementation:

screenshot 2018-11-01 at 10 04 56

So the new implementation has the advantages of having back the Labels property, but has lost the flexibility of the separated labels (font and position).

I add my test files here:

CSSTUDIO-817.zip

kasemir commented 6 years ago

Font can be set: 3323cbadb3e7db6bf3f461453ab57d3fb25db74c

kasemir commented 6 years ago

As for the vertical alignment, your right, isn't it curious that the alignment is off? I cannot see why, though. Maybe the Text bounds are not valid at the time they're used? But then why is the horizontal alignment correct?

As for the position, I'll look at adding an option.

kasemir commented 6 years ago

BOY didn't have a 'position' option. Instead, square LEDs used the widget size, centering the labels in the LED. Round LEDs were always circular, placing the label on the side, requiring you to pick the correct widget size, otherwise the labels are truncated.

leds

claudio-rosati commented 6 years ago

Yes, it's true... but we can do better or just be compatible.

claudio-rosati commented 6 years ago

@kasemir My PR #465 should solve the vertical alignment of the labels.

kasemir commented 6 years ago

Now it's as in BOY, except for a minor detail how vertical labels get truncated when they're too long.

bytes

claudio-rosati commented 6 years ago

Perfect. I've tested it and now the conversion it's fine. Thank you