Differential D28666

Refactor for loops
ClosedPublic
Actions

Authored by cblack on Apr 7 2020, 8:59 PM.

Details

Reviewers
mart
Group Reviewers
Kirigami
Commits
R169:6a0535050b8c: Refactor for loops
Summary

For loops have been refactored to use more idiomatic JavaScript functions (filter(), some(), forEach()) where appropriate.

Test Plan

Ensure no regressions in modified components

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cblack created this revision.Apr 7 2020, 8:59 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptApr 7 2020, 8:59 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Apr 7 2020, 8:59 PM
cblack updated this revision to Diff 79621.Apr 8 2020, 12:14 AM

Use Array.from

cblack updated this revision to Diff 79623.Apr 8 2020, 12:25 AM

Use Array.prototype.*.call() instead of Array.from to reduce copying

ngraham added a subscriber: ngraham.Apr 8 2020, 4:04 AM

I'm not a Javascript expert, but the old/current versions seem much more readable to me. Is there a performance advantage to your proposed new versions?

broulik added a subscriber: broulik.Apr 8 2020, 7:06 AM

+1 for the .some() use
-1 on the convoluted Array.prototype things, especially when chained.

src/controls/Action.qml
156

Why are you going through the prototype? Or is children not a "proper" Array?
Also, can we use spread operator ... here?
And yes, please make those a bit more readable by using useful line breaks and using parentheses and braces.

src/controls/FormLayout.qml
83–84

Looks like a usecas for Array.reduce?

src/controls/templates/private/PassiveNotification.qml
62–63

I would like a comment here. Note veryone is faimilar with slice in that fashion.

73–75

Why no arrow function?

cblack updated this revision to Diff 79642.Apr 8 2020, 1:48 PM
cblack marked 3 inline comments as done.

Address feedback

cblack added inline comments.Apr 8 2020, 1:48 PM
src/controls/Action.qml
156

It's a QML list, not an ECMAScript Array. Also, I don't see why you would need to use the spread operator here.

cblack added a comment.Apr 8 2020, 1:49 PM
In D28666#644079, @ngraham wrote:

I'm not a Javascript expert, but the old/current versions seem much more readable to me. Is there a performance advantage to your proposed new versions?

No, the main point is writing idiomatic JavaScript. To people familiar with idiomatic JavaScript, these constructs will immediately let them know what's going on instead of them having to parse a multiline structure.

mart accepted this revision.Apr 20 2020, 3:52 PM
This revision is now accepted and ready to land.Apr 20 2020, 3:52 PM
Closed by commit R169:6a0535050b8c: Refactor for loops (authored by cblack). · Explain WhyApr 20 2020, 9:38 PM
This revision was automatically updated to reflect the committed changes.
ngraham added a comment.Apr 21 2020, 9:24 PM

This appears to have broken SwipeListItem. Now the actions don't show up anymore and there's loads of console spam:

file:///home/nate/kde/usr/lib64/qml/org/kde/kirigami.2/templates/SwipeListItem.qml:421: TypeError: Type error
file:///home/nate/kde/usr/lib64/qml/org/kde/kirigami.2/templates/SwipeListItem.qml:421: TypeError: Type error
file:///home/nate/kde/usr/lib64/qml/org/kde/kirigami.2/templates/SwipeListItem.qml:421: TypeError: Type error

Might want to check everything else too.

ngraham added a comment.Apr 21 2020, 11:11 PM

That was fixed, thanks.

Revision Contents
Changeset List

Diff 80710

View Options

src/controls/Action.qml

Loading...
View Options

src/controls/CardsLayout.qml

Loading...
View Options

src/controls/ContextDrawer.qml

Loading...
View Options

src/controls/FormLayout.qml

Loading...
View Options

src/controls/templates/SwipeListItem.qml

Loading...
View Options

src/controls/templates/private/PassiveNotification.qml

Loading...