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 &presetName);
> 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),
> &KScreen::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
