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

Alarm tree display broken as side effect of configurable background color #2608

Closed
kasemir opened this issue Mar 22, 2023 · 5 comments · Fixed by #2612
Closed

Alarm tree display broken as side effect of configurable background color #2608

kasemir opened this issue Mar 22, 2023 · 5 comments · Fixed by #2612
Assignees

Comments

@kasemir
Copy link
Collaborator

kasemir commented Mar 22, 2023

#2449 added configurable background colors based on alarm severity to the alarm tree, alarm table, PV tree etc.

Turns out that broke the alarm tree display.

The tree view had a small triangle to the left of intermediate nodes that allow opening/closing the sub tree, and the background was used to indicate the selected cell when clicking on a cell or traversing the three using cursor keys:

Screenshot 2023-03-22 at 3 33 55 PM

Now that the tree cell backgrounds are set based on the alarm colors, clicking on "Demo" in the above example will remove(!) that expand/collapse triangle, and there is no more visual feedback of the selected item:

Screenshot 2023-03-22 at 3 33 00 PM

That was actually a known caveat because I guess using setBackground had been tried before,

// Note: Cannot use background because that's used by style sheet and selection/cursor

        // Note: Cannot use background because that's used by style sheet and selection/cursor
@shroffk
Copy link
Member

shroffk commented Mar 23, 2023

Sorry,
I should have paid more attention to the comments.

The initial attempt to implement the background was to set the sytle sheet ( I have added a TODO about this ) however this did not work. Once I am back in a few weeks I will try to redo this feature using the style sheet... hopefully the move to the latest javafx might have addresses the issues I was facing in the past.

@georgweiss
Copy link
Collaborator

Speaking of latest JavaFX... If you mean v 20 then we need to bring up Java version.

@kasemir
Copy link
Collaborator Author

kasemir commented Mar 23, 2023

I tried this with JFX 20 and there was no difference in the alarm tree.
The modena.css for the tree simply includes changes to the background for the selection/cursor, and it also uses the background for the 'arrow', so setting the cell background clashes with that.
The fix on the branch is to have our own Label or HBox(Icon, Label) and then you can update the background of that Label while the tree can continue to do what it wants with the overall background.

@shroffk
Copy link
Member

shroffk commented Mar 24, 2023

Thank you for fixing this @kasemir

When I suggested the redoing this I was planning of replacing the programmatic background sets with calls to set the css -fx-control-inner-background, which should hopefully work more consistently with the rest of the widgets behaviours.

Anyway, your solution might be simpler and cleaner so we should keep that

@kasemir
Copy link
Collaborator Author

kasemir commented Mar 24, 2023

The missing expand/collapse triangle was minor, what really caused trouble is the missing cell updates, #2611
I was looking at that when I noticed this issue.

I think it's quite common that you have some section of the alarm tree open with many PVs, only partially visible because the list exceeds the window height, and then those that are not shown end up with the wrong value once scrolled into the viewport. I can reproduce that with a simple demo that suggests TreeItem.setValue() simply doesn't always work, you need to replace the TreeItem. Fixed that in #2615, but now overall disenchanted with the TreeView.

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

Successfully merging a pull request may close this issue.

3 participants