Changeset View
Standalone View
desktoppackage/contents/explorer/WidgetExplorer.qml
| Show All 11 Lines | |||||
| 12 | * GNU General Public License for more details | 12 | * GNU General Public License for more details | ||
| 13 | * | 13 | * | ||
| 14 | * You should have received a copy of the GNU Library General Public | 14 | * You should have received a copy of the GNU Library General Public | ||
| 15 | * License along with this program; if not, write to the | 15 | * License along with this program; if not, write to the | ||
| 16 | * Free Software Foundation, Inc., | 16 | * Free Software Foundation, Inc., | ||
| 17 | * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | 17 | * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| 18 | */ | 18 | */ | ||
| 19 | 19 | | |||
| 20 | import QtQuick 2.2 | 20 | import QtQuick 2.7 | ||
| 21 | import QtQuick.Controls 1.1 | 21 | import QtQuick.Controls 1.1 | ||
| 22 | 22 | | |||
| 23 | import org.kde.plasma.components 2.0 as PlasmaComponents | 23 | import org.kde.plasma.components 2.0 as PlasmaComponents | ||
| 24 | import org.kde.plasma.core 2.0 as PlasmaCore | 24 | import org.kde.plasma.core 2.0 as PlasmaCore | ||
| 25 | import org.kde.plasma.extras 2.0 as PlasmaExtras | 25 | import org.kde.plasma.extras 2.0 as PlasmaExtras | ||
| 26 | import org.kde.kquickcontrolsaddons 2.0 | 26 | import org.kde.kquickcontrolsaddons 2.0 | ||
| 27 | import org.kde.kwindowsystem 1.0 | 27 | import org.kde.kwindowsystem 1.0 | ||
| 28 | 28 | | |||
| ▲ Show 20 Lines • Show All 118 Lines • ▼ Show 20 Line(s) | 34 | Item { | |||
| 147 | 147 | | |||
| 148 | PlasmaComponents.ModelContextMenu { | 148 | PlasmaComponents.ModelContextMenu { | ||
| 149 | id: getWidgetsDialog | 149 | id: getWidgetsDialog | ||
| 150 | visualParent: getWidgetsButton | 150 | visualParent: getWidgetsButton | ||
| 151 | placement: PlasmaCore.Types.TopPosedLeftAlignedPopup | 151 | placement: PlasmaCore.Types.TopPosedLeftAlignedPopup | ||
| 152 | // model set on first invocation | 152 | // model set on first invocation | ||
| 153 | onClicked: model.trigger() | 153 | onClicked: model.trigger() | ||
| 154 | } | 154 | } | ||
| 155 | /* | 155 | /* | ||
ngraham: whitespace change :) | |||||
| 156 | PlasmaCore.Dialog { | 156 | PlasmaCore.Dialog { | ||
? Not sure The extra comment adds anything. Instead we should just remove the commented-out code in a new patch IMHO. ngraham: ? Not sure The extra comment adds anything. Instead we should just remove the commented-out… | |||||
I'm always unclear what to do when I find blocks of other people's comments. Some reviewers absolutely loathe comments being left in; others don't seem to object as much. I've asked for a comment policy to be put together as part of the new-onboarding initiative. I was taught to comment my code for clarity, but not to leave blocks of work-in-progress code like this behind. I hesitated to take it out in case someone was planning on returning to it, but I probably shouldn't have added the graffiti. I'll remove my unnecessary side comment. sharvey: I'm always unclear what to do when I find blocks of other people's comments. Some reviewers… | |||||
In general, KDE policy is to remove code rather than commenting it out. But we also try to encourage patches to be as focused as possible. Hence, what I think makes sense is to ignore the commented-out block in this patch, and remove it entirely in another one. ngraham: In general, KDE policy is to remove code rather than commenting it out. But we also try to… | |||||
| 157 | id: tooltipDialog | 157 | id: tooltipDialog | ||
| 158 | property Item appletDelegate | 158 | property Item appletDelegate | ||
| 159 | location: PlasmaCore.Types.RightEdge //actually we want this to be the opposite location of the explorer itself | 159 | location: PlasmaCore.Types.RightEdge //actually we want this to be the opposite location of the explorer itself | ||
| 160 | 160 | | |||
| 161 | type: PlasmaCore.Dialog.Tooltip | 161 | type: PlasmaCore.Dialog.Tooltip | ||
| 162 | flags:Qt.Window|Qt.WindowStaysOnTopHint|Qt.X11BypassWindowManagerHint | 162 | flags:Qt.Window|Qt.WindowStaysOnTopHint|Qt.X11BypassWindowManagerHint | ||
| 163 | 163 | | |||
| 164 | onAppletDelegateChanged: { | 164 | onAppletDelegateChanged: { | ||
| Show All 28 Lines | 190 | Timer { | |||
| 193 | repeat: false | 193 | repeat: false | ||
| 194 | onTriggered: tooltipDialog.visible = false | 194 | onTriggered: tooltipDialog.visible = false | ||
| 195 | } | 195 | } | ||
| 196 | */ | 196 | */ | ||
| 197 | 197 | | |||
| 198 | 198 | | |||
| 199 | RowLayout { | 199 | RowLayout { | ||
| 200 | id: topBar | 200 | id: topBar | ||
| 201 | anchors { | 201 | anchors { | ||
Can we use some multiple of units.smallSpacing or something here instead of a hardcoded pixel value? ngraham: Can we use some multiple of `units.smallSpacing` or something here instead of a hardcoded pixel… | |||||
Switched to regular gridUnit sizing. Units would appear to be the de facto measurement in Plasmaworld. They're based on DPI and are supposed to scale independently, so, yes - better than pixels. Whether units.gridUnits or units.smallSpacing is used seems to depend solely on the scale needed and your willingness to do multiplication. Both variations are used in the (unmodified) scroll area of this Explorer. sharvey: Switched to regular `gridUnit` sizing. `Units` would appear to be the de facto measurement in… | |||||
| 202 | top: parent.top | 202 | top: parent.top | ||
| 203 | left: parent.left | 203 | left: parent.left | ||
| 204 | right: parent.right | 204 | right: parent.right | ||
| 205 | } | 205 | } | ||
| 206 | 206 | | |||
| 207 | | ||||
ngraham: Unrelated whitespace change | |||||
| 208 | | ||||
| 207 | Item { | 209 | Item { | ||
| 208 | id: header | 210 | id: header | ||
| 209 | property bool showingSearch: false | 211 | property bool showingSearch: false | ||
| 210 | Layout.fillWidth: true | 212 | Layout.fillWidth: true | ||
| 211 | Layout.minimumHeight: Math.max(heading.height, searchInput.height) | | |||
| 212 | Layout.alignment: Qt.AlignVCenter | 213 | Layout.alignment: Qt.AlignVCenter | ||
ngraham: Does anything break if we just remove this line entirely? | |||||
| 213 | PlasmaExtras.Title { | 214 | PlasmaExtras.Title { | ||
| 214 | id: heading | 215 | id: heading | ||
| 215 | anchors.verticalCenter: parent.verticalCenter | 216 | anchors.verticalCenter: parent.verticalCenter | ||
| 216 | text: i18nd("plasma_shell_org.kde.plasma.desktop", "Widgets") | 217 | text: i18nd("plasma_shell_org.kde.plasma.desktop", "Widgets") | ||
| 217 | width: parent.width | 218 | width: parent.width | ||
| 218 | elide: Text.ElideRight | 219 | elide: Text.ElideRight | ||
| 219 | visible: !header.showingSearch | | |||
| 220 | } | | |||
| 221 | PlasmaComponents.TextField { | | |||
| 222 | id: searchInput | | |||
| 223 | width: parent.width | | |||
| 224 | clearButtonShown: true | | |||
| 225 | anchors.verticalCenter: parent.verticalCenter | | |||
| 226 | placeholderText: i18nd("plasma_shell_org.kde.plasma.desktop", "Search...") | | |||
| 227 | onTextChanged: { | | |||
| 228 | list.positionViewAtBeginning() | | |||
| 229 | list.currentIndex = -1 | | |||
| 230 | widgetExplorer.widgetsModel.searchTerm = text | | |||
| 231 | header.showingSearch = (text != ""); | | |||
| 232 | } | | |||
| 233 | | ||||
| 234 | Component.onCompleted: forceActiveFocus() | | |||
| 235 | visible: header.showingSearch | | |||
| 236 | } | 220 | } | ||
ngraham: No need for this anymore, since `True` is the default value. | |||||
| 237 | } | 221 | } | ||
| 238 | 222 | | |||
| 239 | PlasmaComponents.ToolButton { | 223 | PlasmaComponents.ToolButton { | ||
| 240 | id: searchButton | 224 | id: searchButton | ||
| 241 | iconSource: "edit-find" | 225 | iconSource: "edit-find" | ||
| 242 | 226 | | |||
| 243 | checkable: true | 227 | checkable: true | ||
| 244 | onClicked: header.showingSearch = !header.showingSearch | 228 | onClicked: header.showingSearch = !header.showingSearch | ||
| 245 | checked: header.showingSearch | 229 | checked: header.showingSearch | ||
| 246 | onCheckedChanged: { | 230 | onCheckedChanged: { | ||
| 247 | if (!checked) { | 231 | if (!checked) { | ||
| 248 | searchInput.text = ""; | 232 | searchInput.text = ""; | ||
| 233 | newSearchRow.height = 0; | ||||
| 234 | widgetExplorer.widgetsModel.searchTerm = ""; | ||||
| 235 | } else { | ||||
| 236 | newSearchRow.height = parent.height; | ||||
Could this be expressed in terms of the search field's actual height? That way we wouldn't be vulnerable to height issues if the units.smallSpacing value changed at some point. ngraham: Could this be expressed in terms of the search field's actual height? That way we wouldn't be… | |||||
units.gridUnit * 2 = 36 pixels. That seems a bit tall, no? Is there any way we can derive this height value programmatically? ngraham: `units.gridUnit * 2` = 36 pixels. That seems a bit tall, no? Is there any way we can derive… | |||||
Wellll.... from my (albeit limited) comprehension of units, they're all derived programatically somehow, either from an icon size, a font size, or at the lowest level, DPI. The API page for Units::gridUnits reads
So if I understand your question correctly, deriving the row size from a font size would be redundant. Wouldn't it? sharvey: Wellll.... from my (albeit limited) comprehension of `units`, they're all derived… | |||||
Plus, gridUnit * 2 doesn't seem excessively large. There's some whitespace, but I don't think it's overkill. sharvey: Plus, `gridUnit * 2` doesn't //seem// excessively large. There's some whitespace, but I don't… | |||||
I mean, could we do something like newSearchRow.height = searchInput.height + (units.smallSpacing * 2) or something like that? Would that work? Now that I think about it, instead of conditionally adjusting the height in this if/else block, could we just have this toggle the showingSearch property? Does that not work? In fact, I wonder why we need the onClicked as well as onCheckedChanged lines at all. Perhaps those could be simplified into just one. ngraham: I mean, could we do something like `newSearchRow.height = searchInput.height + (units. | |||||
A method to my madness! The size is there so the search bar announces its arrival by expanding the row, pushing the widget grid downward, and appearing. And then again in reverse. Otherwise, there's just an empty space there, waiting for the search box to appear. It's not much eye candy, but just a little bit. sharvey: A method to my madness! The size is there so the search bar announces its arrival by expanding… | |||||
ngraham: Ah, I see what you mean now! | |||||
| 249 | } | 237 | } | ||
| 250 | } | 238 | } | ||
| 251 | } | 239 | } | ||
| 252 | 240 | | |||
| 253 | PlasmaComponents.ToolButton { | 241 | PlasmaComponents.ToolButton { | ||
| 254 | id: categoryButton | 242 | id: categoryButton | ||
| 255 | tooltip: i18nd("plasma_shell_org.kde.plasma.desktop", "Categories") | 243 | tooltip: i18nd("plasma_shell_org.kde.plasma.desktop", "Categories") | ||
| 256 | iconSource: "view-filter" | 244 | iconSource: "view-filter" | ||
| 257 | onClicked: { | 245 | onClicked: { | ||
| 258 | categoriesDialog.model = widgetExplorer.filterModel | 246 | categoriesDialog.model = widgetExplorer.filterModel | ||
| 259 | categoriesDialog.open(0, categoryButton.height) | 247 | categoriesDialog.open(0, categoryButton.height) | ||
| 260 | } | 248 | } | ||
| 261 | } | 249 | } | ||
| 262 | 250 | | |||
| 263 | PlasmaComponents.ToolButton { | 251 | PlasmaComponents.ToolButton { | ||
| 264 | id: closeButton | 252 | id: closeButton | ||
| 265 | iconSource: "window-close" | 253 | iconSource: "window-close" | ||
| 266 | onClicked: main.closed() | 254 | onClicked: main.closed() | ||
| 267 | } | 255 | } | ||
| 268 | } | 256 | } | ||
| 269 | 257 | | |||
| 258 | RowLayout { | ||||
| 259 | id: newSearchRow | ||||
| 260 | anchors.top: topBar.bottom | ||||
| 261 | anchors.topMargin: units.smallSpacing | ||||
| 262 | width: topBar.width | ||||
| 263 | | ||||
| 264 | PlasmaComponents.TextField { | ||||
| 265 | id: searchInput | ||||
ngraham: Is this needed? | |||||
ngraham: Delete code, don't comment it out. | |||||
sharvey: Self = kicked. | |||||
| 266 | visible: header.showingSearch | ||||
| 267 | Layout.fillWidth: true | ||||
| 268 | clearButtonShown: true | ||||
| 269 | placeholderText: i18nd("plasma_shell_org.kde.plasma.desktop", "Search...") | ||||
| 270 | onTextChanged: { | ||||
| 271 | list.positionViewAtBeginning() | ||||
| 272 | list.currentIndex = -1 | ||||
| 273 | widgetExplorer.widgetsModel.searchTerm = text | ||||
| 274 | } | ||||
| 275 | | ||||
| 276 | Component.onCompleted: forceActiveFocus() | ||||
This is all a bit weird. This searchBarContainer is 0 px tall as it's not set to be the size of the children. which is why you need to do that topMargin: units.largeSpacing * 1.5 bodge later instead of just anchoring to the bottom of this container. Also this shouldn't have a fixed size, if it's intended to fill the width of the parent, tell it to fill the width of the parent. You can solve all 3 in one by merging searchBarContainer and searchInput into just being the one item. davidedmundson: This is all a bit weird.
This searchBarContainer is 0 px tall as it's not set to be the size… | |||||
I think this is the kind of thing you were looking for. Sizes and spacing based off existing elements rather than explicit sizes. sharvey: I think this is the kind of thing you were looking for. Sizes and spacing based off existing… | |||||
| 277 | } | ||||
| 278 | } | ||||
| 279 | | ||||
| 270 | Timer { | 280 | Timer { | ||
This comment, if necessary at all, should be phrased in terms of the current state, not the change that added it. ngraham: This comment, if necessary at all, should be phrased in terms of the current state, not the… | |||||
| 271 | id: setModelTimer | 281 | id: setModelTimer | ||
| 272 | interval: 20 | 282 | interval: 20 | ||
| 273 | running: true | 283 | running: true | ||
| 274 | onTriggered: list.model = widgetExplorer.widgetsModel | 284 | onTriggered: list.model = widgetExplorer.widgetsModel | ||
| 275 | } | 285 | } | ||
| 276 | 286 | | |||
| 277 | PlasmaExtras.ScrollArea { | 287 | PlasmaExtras.ScrollArea { | ||
| 278 | anchors { | 288 | anchors { | ||
| 279 | top: topBar.bottom | 289 | top: newSearchRow.bottom | ||
| 280 | left: parent.left | 290 | left: parent.left | ||
| 281 | right: parent.right | 291 | right: parent.right | ||
ngraham: Ditto; let's not use a hardcoded pixel value. | |||||
| 282 | bottom: bottomBar.top | 292 | bottom: bottomBar.top | ||
| 283 | topMargin: units.smallSpacing | | |||
| 284 | bottomMargin: units.smallSpacing | 293 | bottomMargin: units.smallSpacing | ||
| 294 | topMargin: units.smallSpacing | ||||
| 285 | } | 295 | } | ||
| 286 | 296 | | |||
| 287 | verticalScrollBarPolicy: Qt.ScrollBarAlwaysOn | 297 | verticalScrollBarPolicy: Qt.ScrollBarAlwaysOn | ||
| 288 | 298 | | |||
| 289 | // hide the flickering by fading in nicely | 299 | // hide the flickering by fading in nicely | ||
| 290 | opacity: setModelTimer.running ? 0 : 1 | 300 | opacity: setModelTimer.running ? 0 : 1 | ||
| 291 | Behavior on opacity { | 301 | Behavior on opacity { | ||
| 292 | OpacityAnimator { | 302 | OpacityAnimator { | ||
| ▲ Show 20 Lines • Show All 88 Lines • ▼ Show 20 Line(s) | 386 | PlasmaComponents.Button { | |||
| 381 | iconSource: widgetExplorer.extraActions[modelData].icon | 391 | iconSource: widgetExplorer.extraActions[modelData].icon | ||
| 382 | text: widgetExplorer.extraActions[modelData].text | 392 | text: widgetExplorer.extraActions[modelData].text | ||
| 383 | onClicked: widgetExplorer.extraActions[modelData].trigger() | 393 | onClicked: widgetExplorer.extraActions[modelData].trigger() | ||
| 384 | } | 394 | } | ||
| 385 | } | 395 | } | ||
| 386 | */ | 396 | */ | ||
| 387 | } | 397 | } | ||
| 388 | } | 398 | } | ||
| 389 | | ||||
ngraham: Unrelated whitespace change | |||||
Looks like it's still there? Not a huge deal, but it would be good to figure out why your changes keep adding or removing whitespace. ngraham: Looks like it's still there? Not a huge deal, but it would be good to figure out why your… | |||||
 Look! An empty line at the bottom of the file. Not sure what's trimming them off. I'm editing in Sublime (since QtCreator doesn't "see" this QML file), but I get a handy red + from an instant git diff. sharvey: {F5848538}
Look! An empty line at the bottom of the file. Not sure what's trimming them off. | |||||
whitespace change :)