Add applet with screen layouts and presentation mode
ClosedPublic

Authored by broulik on Aug 15 2018, 8:13 AM.

Details

Summary

One of LiMux client's requirements is for display configuration to be easily accessible by mouse.
The OSD cannot be accessed by mouse, so this applet offers commonly used screen layouts in an easily accessible place.
To keep the screen on during a presentation (when the application does not do that or the user is actually demonstrating something on the machine itself) one needs to uncheck the non-userfriendly labeled "Enable powermanagement" check box in the battery monitor. Since this is also affects "screen setup", it is placed in this plasmoid as well.
The widget can be placed as an always-visible plasmoid in the panel or in System Tray where it would only show if presentation mode is enabled or more then one screen is connected.

Test Plan


Updated layout:

Diff Detail

Repository
R104 KScreen
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Aug 15 2018, 8:13 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 15 2018, 8:13 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Aug 15 2018, 8:13 AM
broulik updated this revision to Diff 39779.Aug 15 2018, 9:38 AM
  • Use "application enforce presentation mode" to communicate better that the checkbox might be overruled
abetts added a subscriber: abetts.Aug 15 2018, 9:48 AM

On the first string I would change the wording:

"Configurations become enabled when another screen is connected"

The second string could read:

"It will prevent the screen from turning off" (automatically = optional)

This looks generally nice, I like it. Since we're short on people maintaining this stuff, I'd like more of the code to be shared though.

kded/daemon.h
66

Could this be unified in some nice way with the above function (applyOsdAction) which does the same based on the enum...? Maybe just use the enum, or is there a reason for using a string here but not in other places?

plasmoid/kscreenapplet.cpp
52

What is the ownership of the GetConfigOperation? Is it deleted? (I don't know this code very well)

63

This seems completely unused? Remove it maybe.

plasmoid/kscreenapplet.h
44

This enum is a duplicate of OsdAction::Action, and it's off by one. I don't think that's a good idea. Let's share the enum somehow (and move to enum class for new enums).

plasmoid/package/contents/ui/main.qml
52

This is completely duplicated from the OSD, is there no way to share the data instead? I know it doesn't have the "do nothing" action, but I'd rather see improvements in the OSD at the same time (which maybe could get rid of do nothing in favor of just disappearing when the user clicks somewhere else...).

plasmoid/package/metadata.desktop
4

We usually refer to the "monitors" as screen or display, this adds a third term ;) So I'd prefer to have either screen or display here.

broulik added inline comments.Aug 17 2018, 11:23 AM
kded/daemon.h
66

This thing is called by DBus, so I can only send a string over (or a random int, if you prefer that). I use QMetaEnum to map the string to an enum value to avoid having a duplicated list of strings. I don't think this can be optimized further.

plasmoid/kscreenapplet.cpp
52

GetConfigOperation self-deleteLaters when done

mart added a subscriber: mart.Aug 20 2018, 12:11 PM

would this applet be on by default?

broulik updated this revision to Diff 40088.Aug 20 2018, 9:08 PM
  • Share monitor preset code between OSD and applet
ngraham added a subscriber: ngraham.EditedAug 20 2018, 9:17 PM

Instead of:

Presentation mode
This will prevent your screen and computer from turning off automatically
 [] Enable Presentation Mode

I would prefer to get rid of some redundancy and instead do this:

[] Presentation Mode
This will prevent your screen and computer from turning off automatically

Alternatively, we could re-add the word "Enable" if we make it not a big bold header. In that case, we might also want to-rethink the big bold header for the screen layout buttons too.

Instead of:

Presentation mode
This will prevent your screen and computer from turning off automatically
 [] Enable Presentation Mode

I would prefer to get rid of some redundancy and instead do one of these:

[] Enable Presentation Mode
This will prevent your screen and computer from turning off automatically
[] Presentation mode
This will prevent your screen and computer from turning off automatically

+1

ngraham added inline comments.Aug 20 2018, 10:30 PM
plasmoid/package/contents/ui/main.qml
44

Per https://hig.kde.org/style/writing/labels.html#using-ellipses-in-labels, if you want this to have ellipses, it has to start with an action verb (e.g. "Configure blabla..."). If not, it shouldn't have ellipses, because it's not an action, it's for navigation.

The presentation mode doesn't really relate to the layouts that's why I put the headings in there. Just placing a checkbox there will make it drown underneath the layout buttons

Maybe, but right now it's being oppressed to death by huge headers! :)

Then please make a final layout image that I can change my work to, preferably not being *completely* different from what I have now … also, the headings are smaller than e.g. System Tray's "Status and Notifications" heading

@broulik, i'm pretty sure that it has a review that enable accessing OSD by mouse, can you investigate and ship it. I'm not against this but accessing OSD by mouse is pretty good feature after all.

broulik updated this revision to Diff 40245.Aug 22 2018, 6:59 PM
broulik edited the test plan for this revision. (Show Details)
  • Fixup wording
  • Improve layout of Presentation Mode checkbox

+1 on the UI now.

Love! Love! <3

Thanks for working on it.

gladhorn accepted this revision.Aug 24 2018, 1:09 PM

OK, let's do it :)

This revision is now accepted and ready to land.Aug 24 2018, 1:09 PM
This revision was automatically updated to reflect the committed changes.

@broulik Could we clarify the copyright?

plasmoid/kscreenapplet.h
4–5

Hi Kai. Imo this copyright is fuzzy to me: Do you have the copyright here explicitly or is "the LiMuX project" also copyright holder?
It's nice that this was sponsored, but to be honest this should not be part of this here. At the point where we try to relicense this for instance (for whatever reason, e.g. to LGPL), this will popup again.