Move Widget search field to its own row so it doesn't get compressed
ClosedPublic

Authored by sharvey on May 13 2018, 10:37 PM.

Details

Summary

Move search to its own row to prevent it from being compressed

BUG: 393427

Test Plan
  • Recompile plasma-desktop
  • Bring up Widget Explorer
  • Press Search to see Search in new row
  • Select widget type category to see Title become elided

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sharvey created this revision.May 13 2018, 10:37 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 13 2018, 10:37 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sharvey requested review of this revision.May 13 2018, 10:37 PM
sharvey edited the summary of this revision. (Show Details)May 13 2018, 10:39 PM

Before:

After:

sharvey updated this revision to Diff 34083.May 13 2018, 11:14 PM

Submitted from wrong local branch

ngraham added inline comments.May 14 2018, 2:43 AM
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

sharvey added inline comments.May 14 2018, 9:14 AM
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.

sharvey updated this revision to Diff 34129.May 14 2018, 11:22 AM
  • Remove hardcoded pixel sizes; misc code cleanup
sharvey marked 8 inline comments as done.May 14 2018, 11:25 AM
sharvey retitled this revision from Move search field to its own row so it doesn't get compressed to Move Widget search field to its own row so it doesn't get compressed.May 14 2018, 1:03 PM

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.

sharvey updated this revision to Diff 34158.May 14 2018, 6:35 PM
sharvey marked an inline comment as done and an inline comment as not done.
  • Units changed to gridUnits instead of smallSpacing; whitespace cleanup
ngraham added inline comments.May 14 2018, 6:46 PM
desktoppackage/contents/explorer/WidgetExplorer.qml
236

units.gridUnit * 2 = 36 pixels. That seems a bit tall, no? Is there any way we can derive this height value programmatically?

265

Delete code, don't comment it out.

sharvey marked 2 inline comments as done and 2 inline comments as done.May 14 2018, 6:48 PM

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.

sharvey updated this revision to Diff 34160.May 14 2018, 7:21 PM
sharvey marked an inline comment as done and an inline comment as not done.
  • Remove unneeded commented-out code
sharvey marked 3 inline comments as not done.May 14 2018, 7:22 PM
sharvey added inline comments.
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

The fundamental unit of space that should be used for sizes, expressed in pixels.

Given the screen has an accurate DPI settings, it corresponds to a width of the capital letter M

So if I understand your question correctly, deriving the row size from a font size would be redundant. Wouldn't it?

265

Self = kicked.

sharvey marked 7 inline comments as done and 5 inline comments as done.May 14 2018, 7:26 PM
sharvey added inline comments.
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.

ngraham added inline comments.May 14 2018, 7:29 PM
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.

sharvey marked an inline comment as done.May 14 2018, 7:30 PM
sharvey added inline comments.
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.

sharvey marked 13 inline comments as done.May 15 2018, 12:39 AM
sharvey added inline comments.
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!

ngraham accepted this revision.May 15 2018, 4:37 AM

(Please don't land this until at least one Plasma developer has also approved it)

This revision is now accepted and ready to land.May 15 2018, 4:37 AM
davidedmundson requested changes to this revision.May 23 2018, 3:36 PM
davidedmundson added inline comments.
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.

This revision now requires changes to proceed.May 23 2018, 3:36 PM
sharvey updated this revision to Diff 34761.May 23 2018, 11:05 PM
sharvey marked an inline comment as done.
  • Merge branch 'master' into new-search-row
  • Revise QML using relative sizing & spacing instead of fixed values
sharvey marked an inline comment as done.May 23 2018, 11:06 PM
sharvey added inline comments.
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.

sharvey marked an inline comment as done.May 23 2018, 11:11 PM

@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{}

}

This comment was removed by sharvey.
sharvey updated this revision to Diff 34767.May 24 2018, 3:38 AM
  • Add units.smallSpacing as margins around search bar row


Two quick injections of units.smallSpacing for margins and we have a nice sleek display with no overlap.

sharvey marked 3 inline comments as done.May 24 2018, 3:42 AM

@davidedmundson does this look okay now?

davidedmundson accepted this revision.Aug 24 2018, 7:45 AM
This revision is now accepted and ready to land.Aug 24 2018, 7:45 AM
This revision was automatically updated to reflect the committed changes.