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 #23662) | Please also remove #include <config-baloo.h> from the header file |
| 79–80 | Maybe we can use a QScopedPointer for m_sourceModel? | |
| 137–138 | typo: correct | |
| 167 | Please call it sourceIndex | |
| 424 | Sounds like if should not be in this sentence. | |
| 547–556 | remove one space before + | |
| 567 | sourceIndex | |
| 668 | Please call it group | |
| 853–855 ↗ | (On Diff #23662) | 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 | ||
| 149 | for (int r = 0, c = m_sourceModel->rowCount(); r < c; ++r) { don't call rowCount on every iteration | |
| 405–406 | dito, don't call count on every iteration | |
| 419 | dito | |
| 595 | dito | |
| 723–724 | 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 | |
| 734–735 | simplify this whole method to return m_indexMap.value(row); | |
| 741 | 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 | 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.