Through the ui the user can now right click on a place group
and request to hide it.
Note: It is not the collapsing solution, which would need a way bigger
effort on the model to be achieve but an intermediary solution that could
be useful for users.
Depends on D8367
Depends on D8948
Depends on D8947
Depends on D8946
Depends on D8945
Depends on D8944
Depends on D8943
BUG: 300247
Manual testing with the ui.
| Lint Skipped |
| Unit Tests Skipped |
At least the unit test would be welcome, I let you decide on the other comment.
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 329 ↗ | (On Diff #21612) | Would be nice to unit test that guy |
| src/filewidgets/kfileplacesview.cpp | ||
| 863–866 ↗ | (On Diff #21612) | This duplicates code from the next if branch below... I wonder if before or after that commit we shouldn't try to refactor that and remove some of the code duplication around the delegate animation use? |
please use arc, or add more context to your patches in the future
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 298 | the reserve + loop should be the same as doing m_disappearingItems += indexesGroup; | |
| 431 | i18n puzzle, do something like this: const QString groupLabel = index.data(KFilePlacesModel::GroupRole).toString();
const bool groupHidden = placesModel->isPlaceGroupHidden(index);
const QString category = groupHidden ? i18n("%1 (hidden)", groupLabel) : groupLabel; | |
| 861 | shouldn't we disable the hideSection when d->showAll is in effect? makes no sense to hide something when it has no visual effect, no? | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 298 | I'm filling a list of persistant indexes with regular indexes that is why I use the loop, if you have any other way comment it to me please :) | |
| 861 | There is a visual effect : the label holds "hidden" or not depending of the state. Plus to be consistent with the place entries we should stick with their behavior I think. | |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 863–866 ↗ | (On Diff #21612) | Cool, I'll wait for that extra review to appear before accepting that one. So that we don't forget it. :-) |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 863–866 ↗ | (On Diff #21612) | Was that the thing you had in mind ? |
update, have to wait to be pushed, since it contains i18n translation, next week:
in the first two weeks after the first saturday of the month
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 863–866 ↗ | (On Diff #21612) | Ok for removing duplicate code after this patch. Too hard to add patch before it. |
one minor nit, otherwise lgtm
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 431 ↗ | (On Diff #21612) | only call rowCount once: for (int row = 0, c = rowCount(); row < c; ++row) { |
Looks good and work as expected
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 766 | I think that you can replace if (!clickOverHeader && index.isValid()) { with else if (index.isValid()) { | |