[plasma-framework] make it compiles without foreach
ClosedPublic

Authored by mlaurent on Mar 20 2019, 12:58 PM.

Details

Summary

compile without foreach

Test Plan

autotest ok

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
compile_without_foreach (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10649
Build 10667: arc lint + arc unit
mlaurent created this revision.Mar 20 2019, 12:58 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 20 2019, 12:58 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mlaurent requested review of this revision.Mar 20 2019, 12:58 PM
apol added a subscriber: apol.Mar 26 2019, 2:46 PM

It's bad enough that we need to give things names because Qt keeps detaching on foreach, I'd be giving at least more semantic names to these temporary variables.

autotests/coronatest.cpp
149–150

maybe call it containments? naming variables with a typename is bad (we even are using auto to not mention it's a list!)

src/declarativeimports/core/datamodel.cpp
401

It already is const.

src/plasma/pluginloader.cpp
567

shouldn't it be const?

mlaurent updated this revision to Diff 54920.Mar 27 2019, 7:02 AM

Fix comment reported by Apol

apol added a comment.Mar 27 2019, 11:59 AM

etc.

src/declarativeimports/core/datamodel.cpp
309–310

same?
Also this should clearly be using iterators, no?

src/declarativeimports/core/tooltipdialog.cpp
59–60

same?
If we have to open an incidence for every comment this gets exhausting.

src/plasma/containment.cpp
299–300

just applets?

src/plasma/corona.cpp
132–133

applets?

mlaurent updated this revision to Diff 55796.Apr 9 2019, 6:04 AM

Rename some variables

broulik added a subscriber: broulik.Apr 9 2019, 6:41 AM
broulik added inline comments.
autotests/coronatest.cpp
150

Please always annotate auto with e.g. asterisk or ampersand depending on type

216

why not make the container const?

src/declarativeimports/calendar/daysmodel.cpp
171

While at it const QDate &

src/declarativeimports/core/datamodel.cpp
309–310

Yes, please change it to iterators

mlaurent added inline comments.Apr 9 2019, 7:10 AM
autotests/coronatest.cpp
216

Because this variable is use in code see line 228

dfaure accepted this revision.Apr 14 2020, 8:59 PM

Looks like this got lost/abandoned? It's painful to review (because it's so long), but it shouldn't be lost work...

Can you rebase and see if it still applies?

Maybe in the future better split this up into multiple patches so it can land in chunks instead of getting stuck forever....

src/declarativeimports/calendar/daysmodel.cpp
171

Actually better not, a QDate is just a wrapper for a qint64, with a generated copy constructor (so it's just "copying" a qint64)

src/plasma/corona.cpp
467

(already const, this method is const)

src/plasma/pluginloader.cpp
567

(it is now)

This revision is now accepted and ready to land.Apr 14 2020, 8:59 PM

The title says plasma-desktop, but this is plasma-framework?

mlaurent updated this revision to Diff 80168.Apr 15 2020, 4:53 AM

Rebase against master

mlaurent updated this revision to Diff 80169.Apr 15 2020, 4:56 AM

Port last element to for(...:...)

mlaurent updated this revision to Diff 80170.Apr 15 2020, 4:58 AM

Fix David comment

mlaurent retitled this revision from [plasma-desktop] make it compiles without foreach to [plasma-framework] make it compiles without foreach.Apr 15 2020, 4:59 AM
ahmadsamir added inline comments.
src/declarativeimports/core/iconitem.cpp
628

Coding style &overlay

src/plasma/pluginloader.cpp
328

Coding style, auto &plugin

567

Style, '&md'

645

const, and style '&plugin'.

src/plasma/private/applet_p.cpp
416

Nit-pick: initializer list instead of <<.

src/plasma/private/theme_p.cpp
837

s/QLatin1Literal/QStringLiteral/

src/plasma/private/timetracker.cpp
44

Style "&history".

71

"&ev"

src/scriptengines/qml/plasmoid/containmentinterface.cpp
927

QLatin1String

mlaurent updated this revision to Diff 80182.Apr 15 2020, 9:26 AM

Fix comment

apol accepted this revision.Apr 16 2020, 11:06 PM
apol added inline comments.
src/scriptengines/qml/plasmoid/dropmenu.cpp
80 ↗(On Diff #80182)

m_menu->addActions(m_dropActions);

mlaurent updated this revision to Diff 80344.Apr 17 2020, 4:46 AM

Use m_menu->addActions(m_dropActions);

This revision was automatically updated to reflect the committed changes.