Move search to its own row to prevent it from being compressed
BUG: 393427
ngraham | |
davidedmundson |
Plasma |
Move search to its own row to prevent it from being compressed
BUG: 393427
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
156 | ? Not sure The extra comment adds anything. Instead we should just remove the commented-out code in a new patch IMHO. | |
201 | Can we use some multiple of units.smallSpacing or something here instead of a hardcoded pixel value? | |
207 | Unrelated whitespace change | |
212–213 | Does anything break if we just remove this line entirely? | |
219–220 | No need for this anymore, since True is the default value. | |
280 | This comment, if necessary at all, should be phrased in terms of the current state, not the change that added it. | |
291 | Ditto; let's not use a hardcoded pixel value. | |
389 | Unrelated whitespace change |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
156 | 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. |
A few more code comments that I missed in the first go-around, sorry. Behavior-wise, I think this is a huge improvement!
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
155 | whitespace change :) | |
156 | 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. | |
236 | 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. | |
265 | Is this needed? | |
389 | 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. |
More revising in progress. Units. Bah humbug.
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
201 | 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. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
236 | 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? | |
265 | Self = kicked. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
236 | Plus, gridUnit * 2 doesn't seem excessively large. There's some whitespace, but I don't think it's overkill. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
236 | 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. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
389 | 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. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
236 | 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. |
Well this looks good to me. Let's let some of the Plasma devs have a say in the matter now!
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
236 | Ah, I see what you mean now! |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
276 | 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. |
desktoppackage/contents/explorer/WidgetExplorer.qml | ||
---|---|---|
276 | I think this is the kind of thing you were looking for. Sizes and spacing based off existing elements rather than explicit sizes. |
@davidedmundson (and/or others) - what's the most-correct and most-effective QML method to add a bit of padding around these elements? I know we want to link "tops" to "bottoms" in row layouts, which makes perfect sense. But what's the right way to add a bit of margin? In this case, the descender in the g in Widgets is right up against the search text bar. Likewise, the icons for the available widgets run right up to the bottom of the text field.
Much nicer
what's the most-correct and most-effective QML method to add a bit of padding around these elements?
if you're using anchors, which I think is the case here
TextField
{
anchors.top: topBar.bottom anchors.topMargin: someAmazingValue
}
alternatively if everything is within a layout
ColumnLayout {
spacing : someAwesomeValueHere TheTopHeader {} TextField{}
}