ngraham added a comment.
A few more code comments that I missed in the first go-around, sorry.
Behavior-wise, I think this is a huge improvement!
INLINE COMMENTS
> WidgetExplorer.qml:155
> }
> +
> /*
whitespace change :)
> WidgetExplorer.qml:236
> + } else {
> + newSearchRow.height = units.smallSpacing * 5
> }
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.
> WidgetExplorer.qml:265
> +
> + height: 0
> +
Is this needed?
> sharvey wrote in WidgetExplorer.qml:157
> 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.
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 wrote in WidgetExplorer.qml:389
> 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.
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D12855
To: sharvey, ngraham, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
