Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Legacy rectangle widget with 'None' border style #2203

Closed
tanviash opened this issue Apr 4, 2022 · 4 comments
Closed

Legacy rectangle widget with 'None' border style #2203

tanviash opened this issue Apr 4, 2022 · 4 comments

Comments

@tanviash
Copy link
Collaborator

tanviash commented Apr 4, 2022

We have some legacy rectangle widgets that have a 'None' border style but a non-zero border_width. These rectangles when opened with Display Builder have a zero line_width as enforced here https://github.com/ControlSystemStudio/phoebus/blob/master/app/display/model/src/main/java/org/csstudio/display/builder/model/widgets/OutlineSupport.java#L58.

Can we have the 'None' case to also follow other cases like 'Line' etc. so border_width doesn't get overlooked?

Also, border colors get overridden with NamedWidgetColors.TEXT for 'Line' and other border_styles but what if border color is set to some other color. Why not copy the border_color from BOY rectangle widget to here?

@kasemir
Copy link
Collaborator

kasemir commented Apr 6, 2022

The BOY rectangle has "Line" as well as "Border", which is a bit odd:

Screen Shot 2022-04-06 at 12 07 46 PM

The display builder only has one surrounding element for the rectangle, and that's the "Line".
It imports the width of a "Border" as the width of the "Line", and I agree that the color should then be imported as well instead of defaulting to black resp. the "Text" color:

Screen Shot 2022-04-06 at 12 10 34 PM

But not sure what to do if legacy displays use both "Line" and "Border".
Maybe update the legacy displays to just use "Line", since that will work in both incarnations?

@kasemir
Copy link
Collaborator

kasemir commented Apr 6, 2022

Result would be this, keeping the "border color", and still allowing the "line".
If there's both a border and a line, the one with larger "width" wins.
If "border" and "line" have the same width, the "border" wins, considering that's more important because Kunal's last change completely ignored the "line" and always went with the "border".

Screen Shot 2022-04-06 at 12 34 15 PM

@kasemir
Copy link
Collaborator

kasemir commented Apr 6, 2022

As for "border_style" being "None" yet the border_width is set, I'd consider that an error.

Update the legacy displays to set border style to line, or use "line_width" and "line_color", either way it should then work in both.

kasemir pushed a commit that referenced this issue Apr 6, 2022
.. whichever has a larger "width".
Honor the "border_color" instead of always using black resp. "Text"
color.

#2203
@tanviash
Copy link
Collaborator Author

tanviash commented Apr 7, 2022

Great, thanks Kay!!

@tanviash tanviash closed this as completed Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants