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

Small fixes / QOL updates #3193

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DenSinH
Copy link

@DenSinH DenSinH commented Nov 18, 2024

Hi all,

Internally, we have made a bunch of updates to Phoebus, some of which we figured would be good to port back to the main branch of phoebus. I was not sure if you would prefer separate PRs or a single one, since the changes are all fairly small, but opening 6 single-commit PRs might also be annoying. A brief overview of the changes:

  • Change general DisplayModel scene to fix transformed widgets: If you make a tall, narrow display, with a very tall, 90 degree rotated label (which fits within the bounds of this tall, narrow display, the width of the display is computed with the unrotated width of the label, adding an unnecessary horizontal scrollbar. By nesting the widgets in a group, the transformation is accounted for and the width is computed properly, not giving this scrollbar.
  • Don't try to call zoom listener if none is set: the zoom listener was being called in our own class extending JFXRepresentation, even though no zoom listener was set, and this field is kind of optional anyway.
  • The logentry-add-16@2x icons were different from the 16x16 size versions, which is either a mistake in naming or the wrong icon was used.
  • Add a search bar to the PV table: there was no search bar, but it would be nice to be able to just search with a search bar, especially if your PV table starts growing quite large.
  • Drag & drop non-writeable PV widgets without ctrl pressed. This is more of a small quality of life change we liked internally, where non-editable PV widgets (like text update) can be dragged to the databrowser without pressing control.
  • The code dialog for scripts and rules was quite unwieldy, sporting a non-monospace font and no extra functionality at all. We made the font monospace and added (very basic) tab functionality.

@kasemir
Copy link
Collaborator

kasemir commented Nov 18, 2024

Separate PRs might be easier if we run into concerns with the laundry list, but we can try starting with just one PR.

As for nesting the widgets in a group to fix the width/height computation: This looks like a bug in the JFX pane, but we're unlikely to get that fixed. When first reading that you add a group, I was worried about one added group for each widget, something we'd want to avoid. But it looks like there's only one new group in the overall representation of each display, plus one per embedded display, so that should not make much of a difference for the scene graph size.

As for Ctrl-drag to start dragging a PV name (Command-drag on Mac): Consistency would be good. Always control-drag is consistent. Having to note which type of widget it is and then using Ctrl-drag or plain drag is iffy. Having said that, the need to remember Ctrl-drag on all but Mac, then Command-drag on Mac is also iffy (On Mac, Ctrl-click is like right-click, it opens the context menu, so had to use Command-drag for the PV name...)

Would there be another consistent option? On Linux, you can middle-click to copy the PV name into the clipboard. Could support middle-drag, but that means everybody needs to have mouse, can't do that with a touchpad on a laptop...

@DenSinH
Copy link
Author

DenSinH commented Nov 18, 2024

Indeed, only a single group is added for every (embedded) representation of a display. I looked into it, to see if I could simply replace the parent pane with a group, but it was not possible as there are some properties that are used, which a group does not have (like setting the background if I recall correctly) and I could not just set those properties on the model root (scrollpane) instead either, as the behavior would be different from before.

Regarding the ctrl + drag, I agree, we already figured this might be an issue, since it may not be clear for a user when they can or cannot drag and drop pvs without ctrl. Perhaps Alt drag is an option, though to be honest, ctrl click seems the best to me as well, I think it might be the first thing I try after a plain drag and drop does not work.

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 this pull request may close these issues.

2 participants