Adjusted the default window size to conform to the monitor aspect ratio
AbandonedPublic

Authored by anemeth on Feb 5 2018, 3:45 PM.

Details

Reviewers
ngraham
Group Reviewers
Spectacle
Summary

When spectacle opens the fullscreen capture is shown.
I adjusted the window size to perfectly fit the image for the most common aspect ratios when opened.
It is a bit bigger now, but that shouldn't be an issue even on 1366x768 resolution.

Abandoned for this revision: D10377

Test Plan

Before and after

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D10326
Lint
No Linters Available
Unit
No Unit Test Coverage
anemeth requested review of this revision.Feb 5 2018, 3:45 PM
anemeth created this revision.
anemeth edited the summary of this revision. (Show Details)Feb 5 2018, 3:47 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added a reviewer: Spectacle.
anemeth added a project: Spectacle.
januz awarded a token.Feb 5 2018, 5:32 PM
ngraham added a subscriber: ngraham.Feb 5 2018, 5:56 PM

An interesting idea! At this larger size, I like the way the main window looks much more. Everything seems just kind of better laid-out.

However, this may well look worse than before if the user uses that window to take a new screenshot that doesn't have the same aspect ratio. How does it look if you take a new screenshot that's tall and skinny?

How does it look if you take a new screenshot that's tall and skinny?

That looks pretty stupid, but I guess the existing UI looks pretty stupid with it for the same reason.

I'm inclined to accept this, but now have a thought: instead of hard-coding the default window size to look good with 16:9, could we introspect the user's screen aspect ratio and also make the window get sized correctly for 4x3 and other common sizes? 4x3 is still pretty common in businesses. It would require a bit of logic to do something sane for people with multiple or vertical displays, but that would really be a nice touch.

alexeymin added a subscriber: alexeymin.EditedFeb 5 2018, 7:38 PM

introspect the user's screen aspect ratio

Interesting idea, could be done by simply calling QGuiApplication::primaryScreen()->geometry(), but what to do if user has multiple monitors? With different aspect ratios? ;) And in theory (on systems with no monitor connected?) QGuiApplication::primaryScreen() can even return nullptr, and a simple patch becomes more complex ;) (I know it often happens this way...)

In those cases, I say we give up and try our best to present something sensible. It's not like Spectacle does an outstanding job of this right now anyway. But for the purposes of determining an optimal default size, I think we can and should support the 90% common cases (i.e. single screen, 16:9 or 4:3 aspect ratio).

Hm yeah, attempt to use primary screen's geometry is probably fine, reverting to default size if something goes wrong.

anemeth updated this revision to Diff 26616.Feb 5 2018, 10:09 PM

Added the mainstream ratios: 16:9, 16:10, 4:3, 5:4
Window widths per aspect ratios are hardcoded, but the window size was hardcoded previously too.

Nice! Does anybody have a non-16:9 screen available to test on?

Also, could you adjust the title and description to reflect the latest changes? Thanks!

Nice! Does anybody have a non-16:9 screen available to test on?

I tested on my computer with different resolutions and aspect ratios.
works for me every time.

anemeth retitled this revision from Adjusted the default window size to Adjusted the default window size to conform to the monitor aspect ratio.Feb 5 2018, 10:26 PM
anemeth edited the summary of this revision. (Show Details)

Cool. Let me test it out myself later today.

When you take a screenshot of a window or a region the spectacle window gets hidden and is shown again when the image is captured.
While it is hidden we could adjust the window size to the captured image aspect ratio.
What do you think?
Should I implement this?
If yes should I add it to this revision?

That's a very interesting idea! Let's do it in a different patch.

ngraham accepted this revision.Feb 5 2018, 11:50 PM

Works great! This is a nice change, thanks for the contribution.

This revision is now accepted and ready to land.Feb 5 2018, 11:50 PM
zzag added a subscriber: zzag.Feb 6 2018, 12:55 AM
zzag added inline comments.
src/Gui/KSMainWindow.cpp
194

No-no-no. qFuzzyCompare

zzag added a comment.Feb 6 2018, 12:59 AM

Also, you could calculate width from given aspect ratio and desired height.

ngraham requested changes to this revision.Feb 6 2018, 2:07 AM
This revision now requires changes to proceed.Feb 6 2018, 2:07 AM

While it is hidden we could adjust the window size to the captured image aspect ratio.
What do you think?

User can choose to capture rectangular selection, not the whole screen. So aspect ratio can be very different, resulting window size needs to stay in sane bounds, not to become very distorted. Some kind of limits checking is needed, then

src/Gui/KSMainWindow.cpp
187

nullptr check?

194

Indeed, you should not use == to compare floats

rkflx added a subscriber: rkflx.EditedFeb 6 2018, 9:14 AM

Did you test with other widget styles, in particular those that have different spacing than Breeze? I fear your pile of hardcoded sizes will break down at some point. This can also happen once we do more changes to the UI layout, e.g. in D10301, or when Breeze changes in the future, or for different font sizes etc.

Could you try to figure out the layout with calls like minimumSize() and then dynamically adapt in relation to the aspect ratio of the screenshot? This way Qt would do most of the work for you.

rkflx added a comment.Feb 6 2018, 10:52 AM

When you take a screenshot of a window or a region the spectacle window gets hidden and is shown again when the image is captured.
While it is hidden we could adjust the window size to the captured image aspect ratio.

Not only in this case, but also on startup you should look at what is being captured instead of using the screen's geometry. So far you only considered the default case, but note that Spectacle can also be started with --activewindow.

Doing it dynamically you'll avoid all those 'ifs`.

anemeth abandoned this revision.Feb 8 2018, 12:05 AM

I created a patch that fixes the concerns here but (in my opinion) the change is too different from this scope so I created a different revision: D10377

anemeth edited the summary of this revision. (Show Details)Feb 8 2018, 12:06 AM