Remove "Full Screen(All Monitors)" when only one monitor exists
ClosedPublic

Authored by emateli on Jan 29 2019, 10:33 PM.

Details

Summary

When there is only one screen available the "Current Screen" and "Full Screen (All monitors)" options are identical, so this patch removes the latter.

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
emateli created this revision.Jan 29 2019, 10:33 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptJan 29 2019, 10:33 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
emateli requested review of this revision.Jan 29 2019, 10:33 PM
emateli updated this revision to Diff 50534.Jan 29 2019, 11:00 PM
  • Code style fix

+1 on the idea.

I think it's a bit awkward that when having one screen the remaining option is "Current Screen". I think it should be "Full Screen" instead

+1 on the idea.

I think it's a bit awkward that when having one screen the remaining option is "Current Screen". I think it should be "Full Screen" instead

Agreed.

Thanks for the fix, @emateli, this has mildly bugged me for ages!

ngraham requested changes to this revision.Jan 30 2019, 12:48 AM

Please make that change and then this will be fit to land.

This revision now requires changes to proceed.Jan 30 2019, 12:48 AM
emateli updated this revision to Diff 50562.Jan 30 2019, 4:15 PM
  • Rename action

Hmm, that's not quite what I meant. When there's only one screen, return {FullScreen, ActiveWindow, WindowUnderCursor, TransientWithParent, RectangularRegion} on X11 and {FullScreen, WindowUnderCursor, TransientWithParent} on Wayland.

In other words, CurrentScreen is the thing that should only appear when there's more than one screen.

Ah, thought you and @nicolasfella meant to change the wording on the "Current Screen" option. Isn't it a bit awkward to have "Full Screen (All monitors)" when there is just one active though? Ideally, "Current Screen" could be changed to "Full Screen" when there is only 1 monitor and left as is when there's more than one.

Just because "All monitors" looks a bit awkward when there's only 1, however its fine with me either way, I have no strong feelings on the matter.

Oh I see what you mean.

Could we instead change it so that " (All Monitors)" is only appended to the string "Full Screen" when there are are fact more then one monitor?

emateli updated this revision to Diff 50576.Jan 30 2019, 8:33 PM
  • change option label when on a single screen
ngraham accepted this revision.Jan 31 2019, 10:56 PM

+1 shipit!

This revision is now accepted and ready to land.Jan 31 2019, 10:56 PM
This revision was automatically updated to reflect the committed changes.
mdrost added a subscriber: mdrost.Feb 3 2019, 6:48 PM
mdrost added inline comments.
src/Gui/KSWidget.cpp
57

Everywhere else it is QApplication::screens().count(). length() looks like we are summing lengths of all screens :P

ngraham added inline comments.Feb 4 2019, 2:13 PM
src/Gui/KSWidget.cpp
57