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

Projector Screen Margins #190

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

Conversation

dallonf
Copy link
Contributor

@dallonf dallonf commented Sep 7, 2019

Adds an option to set margins for the projector in Display Settings:

image

As described briefly in #145, this is useful in cases where your projector projects beyond the physical boundaries of the screen and you need to change the shape so that the projection fits better onscreen. My church has a very extreme example where we can only lower the screen halfway without obscuring the band! (Margin Bottom 50%) After singing, we lower the screen the rest of the way. (Margin Bottom 0%)

You can already do this by tweaking the "Custom Position", but margins are a more convenient way to solve the problem. It's easier to remember 50% and 0% than 384 and 768 :)

There are some pretty serious problems I observed on Windows when your screens have different display densities (that is, DPI) - screens with margins often won't be sized correctly to the screen. I really don't know how to solve this and I think it would take a good deal more investigation to figure it out. Maybe for now, it'd be better to alert users somehow that there may be unexpected behavior, whether by documentation or an inline warning on the config screen. (maybe it can detect screens are different densities and show a message if the margins are non-zero?)

@ArvidNy
Copy link
Member

ArvidNy commented Sep 8, 2019

In general this looks great to me! A few small notes however:

  • I personally would suggest using a slider instead of an integer field, I feel that would be easier to handle both from a developer's and user's perspective. If you look at the notice options, I created a custom percentage slider that might be useful in this scenario.
  • Codacy reports a few unused imports, could you perhaps delete them?
  • Regarding DPI, I've tried experimenting with it quite recently and to my knowledge you can get the value for a screen using double DPI = Screen.getScreens().get(PROJECTORSCREEN).getDpi(); In my case it returns 96 if no scaling is being used and 192 if it is scaled. I think you should be able to use that number to know if you should multiply the margin value or not. (Something like DPI > 100 ? marginTop * (DPI / 100) : marginTop;) Do you think that would work?

@dallonf
Copy link
Contributor Author

dallonf commented Sep 10, 2019

I can make most of those changes you suggested! But I've done quite a bit of experimentation with DPI and whatever the fix is, it's not going to be simple or straightforward. It's not a problem specifically with margins, I think; even Custom Position behaves strangely in this situation. It seems to happen whenever the position and size of the projection window doesn't exactly match the full resolution of the screen (I suspect something, somewhere, is special-casing this specific use case of setting a window to cover an entire display, but even 1 pixel off confuses it).

I could probably investigate at some point in the future, but this has already been a pretty long project and I really need a break soon.

@dallonf
Copy link
Contributor Author

dallonf commented Sep 10, 2019

Disregard the current commit for sliders - I just discovered PercentSliderControl which is closer to what I need, but is gonna need a little bit of refactoring to hook it up. I'll get to that in a few days.

@ArvidNy
Copy link
Member

ArvidNy commented Sep 10, 2019

Sure, if that's the case for Custom Position too, I'd say a DPI solution could be an issue for a new PR at some point.

I still haven't tried running the code, but the changes look good to me from a code perspective anyways. No worries, we'll hold off merging this until you've switched the slider for a PercentSliderControl.

@ArvidNy
Copy link
Member

ArvidNy commented Sep 30, 2019

Sorry for taking a long time to check this out! I've just tested the code and the custom position works just as expected. However, nothing happens if I use one of the Output options. I'm using a Linux system and if I remember correctly, Quelea for Linux has an extra full screen option that you might have to override as well. Would you mind checking that out as well?

@dallonf
Copy link
Contributor Author

dallonf commented Sep 30, 2019

Hmm, good catch! Unfortunately I don't have a Linux development environment handy... not sure I'll be able to fix this anytime soon

@ArvidNy
Copy link
Member

ArvidNy commented Sep 30, 2019

This is the method you need to take a look at. I did a quick test by returning false there and then it seems to work then. So if you only add an option to easily check if margins are used and then return false, it will work on Linux systems as well. Although perhaps you should enable setAlwaysOnTop anyways.

@dallonf
Copy link
Contributor Author

dallonf commented Sep 30, 2019

Ok, when I'll get a free moment I'll make that change! I'll just need someone to test on Linux.

@ArvidNy
Copy link
Member

ArvidNy commented Sep 30, 2019

If you make the change I could double-check that it works on Linux!

@dallonf
Copy link
Contributor Author

dallonf commented Oct 18, 2019

@ArvidNy Ok, I did as much as I could without actually being able to test it!

I also notice that setFullScreenAlwaysOnTop function is called from VLCWindowDirect - does this need to be aware of margins as well? I don't have a lot of context on what this does.

Copy link
Member

@ArvidNy ArvidNy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, there's just one small thing I'd want you to change before I'd be willing to merge it (see separate comment).

There also are some issues with the video window not always getting resized properly, but I noticed that we have that issue with custom position screens as well, so I'd say we could live with that for now and sort it out later.

The only other thing that might be an issue is that the projection window isn't set to be always on top with your code. @berry120, do you think that's needed before we can merge it or should we leave it out for now?

}
fiLyricWindow.setAreaImmediate(bounds);
if (!QueleaApp.get().getMainWindow().getMainPanel().getLivePanel().getHide().isSelected()) {
fiLyricWindow.show();
}

// non-custom positioned windows are fullscreen
if (!QueleaProperties.get().isProjectorModeCoords()) {
if (!QueleaProperties.get().isProjectorModeCoords() && !QueleaProperties.get().hasProjectorMargin()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line keeps the projection window from closing if neither a custom position is set nor an output, whereas margins are set. I would suggest moving it to the line below in the else statement where it's actually needed:

} else {
    if (!QueleaProperties.get().hasProjectorMargin()) {
        fiLyricWindow.setFullScreenAlwaysOnTop(true);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated

@ArvidNy
Copy link
Member

ArvidNy commented Oct 18, 2019

I'm not sure about VLCWindowDirect. I haven't noticed any issues while testing it anyways. @berry120, do you know if it needs to be aware of margins?

@berry120
Copy link
Member

Hey,

Sorry, just to say that we're aware this still needs looking at & approving, but we're currently a bit snowed under so it's likely to be after the 2020.0 release is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants