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

Add Fixed Windows Size Settings #394

Merged

Conversation

drothmaler
Copy link
Contributor

@drothmaler drothmaler commented Aug 3, 2021

What does this implement/fix? Explain your changes

As discussed in the issue linked below, for Zoom (or other video) meetings, it would be very handy to be able to preconfigure the size of the media window to ensure a consistent media presentation. So this PR adds the possibility to specify a fixed window resolution in the settings. There are also preset buttons for the most common resolutions (360p, 480p, 720p, 1080p).

Settings Window

Does this close any currently open issues?

#339

Any other comments?

Nope...

@drothmaler
Copy link
Contributor Author

I Have also tried to integrate the auto-adjustment feature #383 from @zokradonh in another feature branch.

The settings Window then would look like that:
Settings windows with auto-size option

But there are a few problems I see (besides that I personally don't like the window jumping at all):

  1. When switching between two differently sliced slides it sets the new size first and starts the fading animation after that. I think the resizing should be part of the animation instead.
  2. When switching to the blank screen, the window goes to min. size, when it probably should just stay the same (or maybe should go to the reference size)

I probably could fix the blank screen issue relatively easy, but the animation part is a bit trickier. Not sure if I want to spend much more time with it - as I don't find the auto-size feature that useful, but maybe other people like/need it much more, than I do.

@AntonyCorbett
Copy link
Owner

@drothmaler Thanks for your work on this feature. I will review shortly and if all is well try to include it in the net-5-port branch. Per the project's contributing guidelines, no localisation files should be included in the PR as this doesn't work well with the existing Crowdin translation mechanism. I can sort that out during the merge. Thanks again! Regards, Antony

@AntonyCorbett
Copy link
Owner

@drothmaler Thanks for your work on this feature. I will review shortly and if all is well try to include it in the net-5-port branch....

I'd forgotten that the net-5-port branch had already been merged into master :)

@drothmaler drothmaler force-pushed the feature/fixed-window-size branch from 225c868 to 2389d02 Compare August 4, 2021 10:30
@drothmaler
Copy link
Contributor Author

No problem... just reverted the German translation file...

@AntonyCorbett
Copy link
Owner

No problem... just reverted the German translation file...

Thanks - it should get translated by the German translator as soon as I push the changes, but if not I can add your translated terms.

@AntonyCorbett AntonyCorbett merged commit 458ec7a into AntonyCorbett:master Aug 4, 2021
@AntonyCorbett
Copy link
Owner

@drothmaler Super work! Thanks again.

A couple of minor changes I would like to make:

  1. In Settings, change the 720p button to the same style as the others. Currently it looks like the 720p button is the currently selected size just because it is rendered differently.
  2. Remove the tooltip on the 720p button since the note about an internet connection may confuse some who are using it outside of a video conference context.

What do you think?

@drothmaler
Copy link
Contributor Author

drothmaler commented Aug 4, 2021

@AntonyCorbett Regarding your comments:

  1. I chose that style to "promote" the option that (probably) most people want - but if you think it confuses more than it helps, I can remove it - no problem.
  2. But outside a video conferencing you would probably not use windowed mode, I think... Again I just tried to make it helpful for users with less technical knowledge - if it isn't, lets get rid of it...

@AntonyCorbett
Copy link
Owner

@drothmaler I've made these minor changes and accepted the PR

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