Differential D22468 Diff 65451 kcm/package/contents/ui/OutputPanel.qml

Changeset View

Standalone View

View Options

kcm/package/contents/ui/OutputPanel.qml

  • This file was added.
1​/********************************************************************
2​Copyright © 2019 Roman Gilg <subdiff@gmail.com>
3​
4​This program is free software; you can redistribute it and/or modify
5​it under the terms of the GNU General Public License as published by
6​the Free Software Foundation; either version 2 of the License, or
7​(at your option) any later version.
8​
9​This program is distributed in the hope that it will be useful,
10​but WITHOUT ANY WARRANTY; without even the implied warranty of
11​MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12​GNU General Public License for more details.
13​
14​You should have received a copy of the GNU General Public License
15​along with this program. If not, see <http://www.gnu.org/licenses/>.
16​*********************************************************************/
17​import QtQuick 2.9
18​import QtQuick.Layouts 1.1
19​import QtQuick.Controls 2.3 as Controls
20​import org.kde.kirigami 2.4 as Kirigami
21​
22​ColumnLayout {
23​ id: outputPanel
24​ property var element: model
25​
26​ Kirigami.FormLayout {
27​ Controls.Label {
28​// Kirigami.FormData.label: i18n("Name")
29​ text: element.display
30​ }
31​
32​ Controls.CheckBox {
33​ text: i18n("Enabled")
34​ checked: element.enabled
35​ onClicked: element.enabled = checked
36​ }
37​
38​ Controls.CheckBox {
39​ text: i18n("Primary")
40​ checked: element.primary
41​ onClicked: element.primary = checked
42​ visible: kcm.primaryOutputSupported
broulikUnsubmitted
Done

i18n and everywhere else

broulik: `i18n` and everywhere else
romanggAuthorUnsubmitted
Done

i18n and everywhere else

I saw in other KCMs the Kirigami label being set without the i18n macro and I hoped Kirigami would do this internally. But apparently not, so I will add them.

romangg: > i18n and everywhere else I saw in other KCMs the Kirigami label being set without the i18n…
broulikUnsubmitted
Done

Try Kirigami.FormData.checkable: true to make the checkbox to the left instead of having a lonely tiny checkbox floating there

broulik: Try `Kirigami.FormData.checkable: true` to make the checkbox to the left instead of having a…
romanggAuthorUnsubmitted
Not Done

Then there are two checkboxes. Looked at Night Color KCM, which does it by just setting the text instead of the FormData.label. Changed it to that for now

romangg: Then there are two checkboxes. Looked at Night Color KCM, which does it by just setting the…
43​ }
44​
45​ Controls.ComboBox {
46​ Kirigami.FormData.label: i18n("Resolution:")
47​ model: element.resolutions
48​ currentIndex: element.resolutionIndex !== undefined ?
49​ element.resolutionIndex : -1
50​ onActivated: element.resolutionIndex = currentIndex
broulikUnsubmitted
Done

Use onClicked which only fires when the user explicitly clicks it, not in case a property/model role changes on its own, also everywhere else

broulik: Use `onClicked` which only fires when the user explicitly clicks it, not in case a…
51​ }
52​
53​ ColumnLayout {
54​ Layout.fillWidth: true
55​
56​ visible: kcm.perOutputScaling
57​ Kirigami.FormData.label: i18n("Scale:")
broulikUnsubmitted
Done

When you have loads of resolutions, we change the Slider for a ComboBox. I don't really like that but a slider could be fiddly to operate with many steps

broulik: When you have loads of resolutions, we change the `Slider` for a `ComboBox`. I don't really…
58​ Controls.Slider {
59​ id: scaleSlider
60​
61​ Layout.fillWidth: true
62​ from: 0.5
63​ to: 3
64​ stepSize: 0.1
65​ live: true
66​ value: element.scale
67​ onMoved: element.scale = value
68​ }
69​ Controls.Label {
70​ Layout.alignment: Qt.AlignHCenter
71​ text: scaleSlider.value.toLocaleString(Qt.locale(), "f", 1)
72​ }
73​ }
74​
75​ Controls.ButtonGroup {
76​ buttons: orientation.children
77​ }
78​
79​ RowLayout {
80​ id: orientation
81​ Kirigami.FormData.label: i18n("Orientation:")
82​ Layout.fillWidth: true
83​
84​ RotationButton {
85​ value: 0
86​ }
davidedmundsonUnsubmitted
Not Done

These need to be exclusive buttons. Currently you can select N at once.

davidedmundson: These need to be exclusive buttons. Currently you can select N at once.
romanggAuthorUnsubmitted
Not Done

Thanks, the issue wasn't them being not exclusive buttons (this is automatically with ButtonGroup above), but that the RotationButtons were encapsulated into another item. Have removed the encapsulation now.

romangg: Thanks, the issue wasn't them being not exclusive buttons (this is automatically with…
87​ RotationButton {
88​ value: 90
89​ }
90​ RotationButton {
91​ value: 180
92​ }
93​ RotationButton {
94​ value: 270
95​ }
96​ }
97​
gvarsanyiUnsubmitted
Done

stepSize settings confuse me a bit. For perOutput it's 0.2, for global, it's 0.1.

I recommend setting both to 0.25 increments.

Both Windows and Gnome devs decided to do 0.25 increments for fractional scaling, and OSX also only has 3 fractional scales for every integer (albeit in logarithmic increments).
Although KDE doesn't have to follow suit, they all might have been onto something.
Advantages of 0.25 steps:

  • less prone to weird pixel-problems and artifacts that would come with scales like 1.6 or 1.1
  • does not skip the important 1.5 factor (like 0.2 steps would)
  • users probably want the 1.25 and 1.75 scale options more than 1.1, 1.2, 1.3, 1.4, 1.6, 1.7, 1.8, 1.9 - but they are unavailable with the 0.1 steps
  • much easier to run test apps against less (4 vs 10) variants

On the other hand, 0.25 should be good enough granularity for most.

gvarsanyi: `stepSize` settings confuse me a bit. For perOutput it's 0.2, for global, it's 0.1. I…
98​ Controls.ComboBox {
99​ Kirigami.FormData.label: i18n("Refresh rate:")
100​ model: element.refreshRates
101​ currentIndex: element.refreshRateIndex ?
102​ element.refreshRateIndex : 0
103​ onActivated: element.refreshRateIndex = currentIndex
104​ }
105​ }
106​}
broulikUnsubmitted
Done

Why not just bind those properties directly?

broulik: Why not just bind those properties directly?
romanggAuthorUnsubmitted
Done

When I try it with model.refreshRateIndex directly it takes the ComboBox model and not the output model. Is there another solution? Thinking of it I could add a outputModel property to the whole OutputPanel component and bind to that here then instead. Does this sound better?

romangg: When I try it with model.refreshRateIndex directly it takes the ComboBox model and not the…
broulikUnsubmitted
Done

Use onActivated which only fires when the user explicitly changes it

broulik: Use `onActivated` which only fires when the user explicitly changes it
broulikUnsubmitted
Done

I think you might want to use a ButtonGroup here to make them semantically mutally exclusive (just a suggestion)

broulik: I think you might want to use a `ButtonGroup` here to make them semantically mutally exclusive…
broulikUnsubmitted
Done

Please use enum names here instead of magic numbers

broulik: Please use enum names here instead of magic numbers
broulikUnsubmitted
Done

You sure this combobox should be full width?

broulik: You sure this combobox should be full width?
broulikUnsubmitted
Done

They lack formatting (decimal separator and "Hz" suffix)

broulik: They lack formatting (decimal separator and "Hz" suffix)