D14855: Add applet with screen layouts and presentation mode

[Dominik Haumann]

dhaumann added a comment.


  @broulik Could we clarify the copyright?

INLINE COMMENTS

> kscreenapplet.h:3-4
> + * Copyright (c) 2018 Kai Uwe Broulik 
> + *Work sponsored by the LiMux project of
> + *the city of Munich.
> + *

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.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy, gladhorn
Cc: dhaumann, anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R104:b4baa322c361: Add applet with screen layouts and 
presentation mode (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D14855?vs=40245=40434#toc

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14855?vs=40245=40434

REVISION DETAIL
  https://phabricator.kde.org/D14855

AFFECTED FILES
  CMakeLists.txt
  kded/CMakeLists.txt
  kded/daemon.cpp
  kded/daemon.h
  kded/org.kde.KScreen.xml
  kded/osdaction.cpp
  kded/osdaction.h
  kded/osdmanager.cpp
  kded/osdmanager.h
  kded/qml/OsdSelector.qml
  plasmoid/CMakeLists.txt
  plasmoid/Messages.sh
  plasmoid/kscreenapplet.cpp
  plasmoid/kscreenapplet.h
  plasmoid/package/contents/ui/PresentationModeItem.qml
  plasmoid/package/contents/ui/ScreenLayoutSelection.qml
  plasmoid/package/contents/ui/main.qml
  plasmoid/package/metadata.desktop
  tests/osd/CMakeLists.txt

To: broulik, #plasma, #vdg, fischbach, harmathy, gladhorn
Cc: anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Frederik Gladhorn]

gladhorn accepted this revision.
gladhorn added a comment.
This revision is now accepted and ready to land.


  OK, let's do it :)

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy, gladhorn
Cc: anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Andres Betts]

abetts added a comment.


  Love! Love! <3
  
  Thanks for working on it.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Nathaniel Graham]

ngraham added a comment.


  +1 on the UI now.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik updated this revision to Diff 40245.
broulik edited the test plan for this revision.
broulik added a comment.


  - Fixup wording
  - Improve layout of Presentation Mode checkbox

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14855?vs=40088=40245

REVISION DETAIL
  https://phabricator.kde.org/D14855

AFFECTED FILES
  CMakeLists.txt
  kded/CMakeLists.txt
  kded/daemon.cpp
  kded/daemon.h
  kded/org.kde.KScreen.xml
  kded/osdaction.cpp
  kded/osdaction.h
  kded/osdmanager.cpp
  kded/osdmanager.h
  kded/qml/OsdSelector.qml
  plasmoid/CMakeLists.txt
  plasmoid/Messages.sh
  plasmoid/kscreenapplet.cpp
  plasmoid/kscreenapplet.h
  plasmoid/package/contents/ui/PresentationModeItem.qml
  plasmoid/package/contents/ui/ScreenLayoutSelection.qml
  plasmoid/package/contents/ui/main.qml
  plasmoid/package/metadata.desktop
  tests/osd/CMakeLists.txt

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Anthony Fieroni]

anthonyfieroni added a comment.


  @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.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: anthonyfieroni, ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik added a comment.


  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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik added a comment.


  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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> main.qml:44
> +// does this need an ellipsis?
> +readonly property string kcmLabel: i18nc("Open the full display settings 
> module", "Advanced Display Settings...")
> +readonly property string kcmIconName: "preferences-desktop-display-randr"

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.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Andres Betts]

abetts added a comment.


  In D14855#312321 , @ngraham wrote:
  
  > 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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Nathaniel Graham]

ngraham added a comment.


  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

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: ngraham, mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik updated this revision to Diff 40088.
broulik added a comment.


  - Share monitor preset code between OSD and applet

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14855?vs=39779=40088

REVISION DETAIL
  https://phabricator.kde.org/D14855

AFFECTED FILES
  CMakeLists.txt
  kded/CMakeLists.txt
  kded/daemon.cpp
  kded/daemon.h
  kded/org.kde.KScreen.xml
  kded/osdaction.cpp
  kded/osdaction.h
  kded/osdmanager.cpp
  kded/osdmanager.h
  kded/qml/OsdSelector.qml
  plasmoid/CMakeLists.txt
  plasmoid/Messages.sh
  plasmoid/kscreenapplet.cpp
  plasmoid/kscreenapplet.h
  plasmoid/package/contents/ui/PresentationModeItem.qml
  plasmoid/package/contents/ui/ScreenLayoutSelection.qml
  plasmoid/package/contents/ui/main.qml
  plasmoid/package/metadata.desktop
  tests/osd/CMakeLists.txt

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Marco Martin]

mart added a comment.


  would this applet be on by default?

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: mart, gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> gladhorn wrote in 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?

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.

> gladhorn wrote in kscreenapplet.cpp:51
> What is the ownership of the GetConfigOperation? Is it deleted? (I don't know 
> this code very well)

`GetConfigOperation` self-`deleteLater`s when done

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D14855: Add applet with screen layouts and presentation mode

[Frederik Gladhorn]

gladhorn added a comment.


  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.

INLINE COMMENTS

> daemon.h:66
> +// DBus
> +void applyLayoutPreset(const QString );
>  Q_SIGNALS:

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?

> kscreenapplet.cpp:51
> +
> +connect(new 
> KScreen::GetConfigOperation(KScreen::GetConfigOperation::NoEDID), 
> ::ConfigOperation::finished,
> +this, [this](KScreen::ConfigOperation *op) {

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

> kscreenapplet.cpp:62
> +
> +void KScreenApplet::configChanged()
> +{

This seems completely unused? Remove it maybe.

> kscreenapplet.h:43
> +
> +enum Action {
> +SwitchToExternal,

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).

> main.qml:51
> +
> +readonly property var screenLayouts: [
> +{

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...).

> metadata.desktop:3
> +Name=Display Configuration
> +Comment=Quickly switch between monitor layouts and presentation mode
> +Icon=preferences-desktop-display

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.

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach, harmathy
Cc: gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D14855: Add applet with screen layouts and presentation mode

[Andres Betts]

abetts added a comment.


  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)

REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

To: broulik, #plasma, #vdg, fischbach
Cc: abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik updated this revision to Diff 39779.
broulik added a comment.


  - Use "application enforce presentation mode" to communicate better that the 
checkbox might be overruled

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14855?vs=39763=39779

REVISION DETAIL
  https://phabricator.kde.org/D14855

AFFECTED FILES
  CMakeLists.txt
  kded/daemon.cpp
  kded/daemon.h
  kded/org.kde.KScreen.xml
  plasmoid/CMakeLists.txt
  plasmoid/Messages.sh
  plasmoid/kscreenapplet.cpp
  plasmoid/kscreenapplet.h
  plasmoid/package/contents/ui/PresentationModeItem.qml
  plasmoid/package/contents/ui/ScreenLayoutSelection.qml
  plasmoid/package/contents/ui/main.qml
  plasmoid/package/metadata.desktop

To: broulik, #plasma, #vdg, fischbach
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14855: Add applet with screen layouts and presentation mode

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, VDG, fischbach.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION 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
  F6196712: Screenshot_20180815_095617.png 

  F6196713: Screenshot_20180815_095827.png 


REPOSITORY
  R104 KScreen

REVISION DETAIL
  https://phabricator.kde.org/D14855

AFFECTED FILES
  CMakeLists.txt
  kded/daemon.cpp
  kded/daemon.h
  kded/org.kde.KScreen.xml
  plasmoid/CMakeLists.txt
  plasmoid/Messages.sh
  plasmoid/kscreenapplet.cpp
  plasmoid/kscreenapplet.h
  plasmoid/package/contents/ui/PresentationModeItem.qml
  plasmoid/package/contents/ui/ScreenLayoutSelection.qml
  plasmoid/package/contents/ui/main.qml
  plasmoid/package/metadata.desktop

To: broulik, #plasma, #vdg, fischbach
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart