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

fix: DisplayRuntime window doesn't use width set by display properties #2387

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

cjenkscybercom
Copy link
Contributor

As discussed in #1926

  • does not introduce a compile-time dependency to the DisplayRuntime module
  • introduces a dimension hint to the AppInstance interface. Since a lot of applications (most, probably?) just want to use the parent window dimensions, the default is to return an empty optional.
  • implements the hint method on the DisplayRuntimeInstance impl, using the width and height properties if defined on the display

@cjenkscybercom
Copy link
Contributor Author

cjenkscybercom commented Sep 15, 2022

One thing I'm wondering @kasemir is if the dimensions should be bound to a Property on the runtime instance...But as I thought about it, the window dimensions are set when it is undocked so I don't imagine there's much need for it? As it already works, you need to refresh the display anyway when you make any changes to it in the Editor. So, I am guessing fetching them once is sufficient here.

@kasemir
Copy link
Collaborator

kasemir commented Sep 15, 2022

For the most part, that looks great!

You used getDimensionHint instead of the getSizeHint that I had suggested, but I'm perfectly willing to let that slide ;-)
I would, however, use for example javafx.geometry.Dimension2D, not the AWT java.awt.Dimension, and use an explicit import instead of import java.awt.*;

The detached display will now open with a configurable size, but still in a pretty random location.
Do you also want to check the model's X and Y location, so the hint could provide the complete bounding rectangle. That's what EDM does in a similar case.

@kasemir
Copy link
Collaborator

kasemir commented Sep 15, 2022

..bound to a Property on the runtime instance...fetching them once is sufficient here.

I think that's fine as it is. Set when pane is created and then leave it.

@cjenkscybercom
Copy link
Contributor Author

use an explicit import instead of import java.awt.*;

Yep, missed it in that one file. Turns out IntelliJ has yet another exception specifically for java.awt and a few other libraries. I nuked em, but forgot to check the other files. That said, I'll use Dimension2D instead thank you!

@cjenkscybercom
Copy link
Contributor Author

Wait, @kasemir using javafx.geometry.Dimension2D would introduce a dependency on javafx-graphics in the phoebus-core module; do we want this?

@kasemir
Copy link
Collaborator

kasemir commented Sep 15, 2022

Good point. We don't want "AWT" per se, but it kinda comes for free.
Also, what do you think about setting all of X, Y, WIDTH, HEIGHT?
Return an int[] with 2 (WIDTH, HEIGHT) or 4 (X, Y, WIDTH, HEIGHT) elements??

@kasemir
Copy link
Collaborator

kasemir commented Sep 15, 2022

Maybe the AWT Dimension is fine. We're not touching the AWT event loop which could conflict with the JFX event loop, we're just using that POD type.
Still, use 'Bounds' or 'Rectangle' to set X, Y, WIDTH, HEIGHT?

@cjenkscybercom
Copy link
Contributor Author

Exactly, it should just be a DTO.
I could throw in x/y position as well, that's a good point for extension later.

@kasemir
Copy link
Collaborator

kasemir commented Sep 15, 2022

Let's return a rectangle or bounds, that is X, Y, WIDTH, HEIGHT.
If X and Y are zero, as they tend to be when nothing has been entered, ignore those.
Otherwise use them to set the X/Y position of the pane.
That allows opening standalone windows in fixed locations, something EDM could do, and some imported/converted EDM displays thus have X/Y set.

@cjenkscybercom
Copy link
Contributor Author

Yep!
And in particular I'll use Rectangle2D (as I don't think we get Bounds for free...); it looks like Rectangle's impl has some static blocks that load/init some AWT toolkit/library stuff.

@cjenkscybercom
Copy link
Contributor Author

cjenkscybercom commented Sep 15, 2022

Sanity-check -- Do we want to actually implement position in this work as well @kasemir ?
E.g., have we seen requests to be able to set window position in addition to size / could this be feature creep for this request or is it a no-brainer feature that should be tacked on here? (I don't have as much context/experience on this)
Otherwise I can just leave the capability in there in the API for now but not have it utilized yet in DisplayRuntime.

…ossible to also specify window position in addition to size if later needed.
@kasemir
Copy link
Collaborator

kasemir commented Sep 15, 2022

For the "target=window" we now support "target=window@.." to set location and size.
EDM uses the display X, Y in addition to WIDTH, HEIGHT when opening a detached window, and we're converting EDM displays which thus have X, Y set.
By default, displays are created with X, Y set to zero, so if we ignore the 0, 0 position there won't be a visible change to existing displays.

@cjenkscybercom
Copy link
Contributor Author

cjenkscybercom commented Sep 16, 2022

@kasemir I've implemented the position stuff now, using position if not the default values (0 and 0).

But I do wonder, what if people want to use the upper left corner (0,0) of their display?
Could it work better to add a BooleanProperty to the Display widget, such as "Use pointer position" and possibly another "Use window dimensions" so that they could e.g. use these behaviors independently (especially as the Widget has default values for these values and somebody might want to use those defaults)? This could also allow a "silent" rollout, e.g. the defaults would be true for both and folks could opt in to using the dimensions and positions specified in the properties if they want to use them?

@cjenkscybercom
Copy link
Contributor Author

cjenkscybercom commented Sep 16, 2022

Aaaah, I take that back...To implement this, those boolean properties need to be hints.

I mean, if we want to generify it and e.g. make it AppInstance<T> where T represents the Hint object or something then I guess that could make hints more universally available to apps...but I think that'd be quite a bit bigger undertaking.

So, can you let me know then @kasemir if there's anything else to do with this, or if this branch is good as-is?

@kasemir kasemir merged commit 4077073 into master Sep 16, 2022
@kasemir kasemir deleted the CSSTUDIO-1121 branch September 16, 2022 12:21
@kasemir
Copy link
Collaborator

kasemir commented Sep 16, 2022

I think this will do. Yes, can't get exact 0, 0 position, need to use 0, 1 or 1, 0 but this is close enough.
It covers the use-size-from-display request, and now also supports converted EDM screens.

I don't think we need much tighter control or more placement hints because the general trend is towards viewing the displays via a web runtime, and then doing that on a laptop, tablet, phone. So in general trying to place something "just so" for a specific monitor will become a futile effort when every user has a different device.

Showing one screen at a time, "full screen", will become the most portable viewing mode. Opening "tabs" may sometimes be an option, but opening additional panels in specific locations really only works when you're in the control room on the exact same display setup.

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