Details
Unit test created
Diff Detail
- Repository
- R318 Dolphin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
I don't know if I can change it to accepted as I am not the maintainer and we depend against not release kf5
> so maintainer must accept it not me no ?
@mlaurent Meaning, now that you've requested changes, you need to either accept the revision at some point, or resign from it.
src/panels/places/placesitem.cpp | ||
---|---|---|
159 | contains() has a QLatin1String overload which should be preferred. | |
src/panels/places/placesitemmodel.cpp | ||
54–57 ↗ | (On Diff #23422) | Please also remove #include <config-baloo.h> from the header file |
79–80 | Maybe we can use a QScopedPointer for m_sourceModel? | |
141–156 | typo: correct | |
191 | Please call it sourceIndex | |
432 | Sounds like if should not be in this sentence. | |
544 | remove one space before + | |
564 | sourceIndex | |
665 | Please call it group | |
853–855 ↗ | (On Diff #23422) | There is a similar comment where we define AppNamePrefix, should that be also removed? |
src/panels/places/placesitemmodel.h | ||
66–67 | typo: hidden | |
120 | override | |
src/tests/placesitemmodeltest.cpp | ||
152–171 | !model ? | |
193 | typo: personal |
some small nits and cleanup suggestions from my side, otherwise lgtm.
src/panels/places/placesitem.cpp | ||
---|---|---|
159 | shouldn't this be equality comparisons instead of contains checks? are there schemas like "foobar-search-asdf" that need to be matched too? | |
src/panels/places/placesitemmodel.cpp | ||
153 | for (int r = 0, c = m_sourceModel->rowCount(); r < c; ++r) { don't call rowCount on every iteration | |
408 | dito, don't call count on every iteration | |
427 | dito | |
592 | dito | |
721 | dito, note that you could also just replace this all with a simple return m_indexMap.indexOf(index);, that works too, no? Otherwise try std::find_if + std::distance | |
738 | simplify this whole method to return m_indexMap.value(row); | |
748 | dito | |
src/panels/places/placesitemmodel.h | ||
226 | QVector, this QList would allocate every item on the heap |
src/panels/places/placesitem.cpp | ||
---|---|---|
159 | well the search can be "search://" or "baloonsearch://" not sure if anything else. "timeline" I think is fixed there is no variation. |
src/panels/places/placesitem.cpp | ||
---|---|---|
159 | Then check for the three variations, instead of these wildcard searched I propose |
More fixes about rowCount usage
src/panels/places/placesitem.cpp | ||
---|---|---|
159 | There is several other places that check with contains. Lets keep this way to avoid regressions. |
Remove this extra "virtual" and +2
src/panels/places/placesitemmodel.h | ||
---|---|---|
120 ↗ | (On Diff #23421) | Remove virtual here as you already use override |
Hi,
I think this commit broke a test.
https://build.kde.org/job/Applications%20dolphin%20kf5-qt5%20SUSEQt5.9/17/testReport/(root)/TestSuite/placesitemmodeltest/
Could you take a look please?
See previous message (i don't know why phabricator immediately marked it as accepted when re-opening).
I want to make clear that @renatoo was the original author, and this work was committed in da6f8fe0862585287153f0d90e19eab0b34bfbef.