ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > MenuItem.h:160 > + > + void updateDefault(); > + I don't like the name much, I think it could be confused with updating actual default value or such... updateIsDefault() is not great either though... :-/ > SidebarMode.cpp:117 > +{ > + QHash<int, QByteArray> roleNames; > + roleNames.insert(Qt::DisplayRole, "display"); This is generally implemented by calling the roleNames() of the parent class and then tune the returned hash. This way you ensure you keep the support for the standard roles. In your case that'd give something like: auto roleNames = QStandardItemModel::roleNames(); roleNames.insert(MenuModel::IsDefaultRole, "default"); // yeah... QML doesn't have the isFoo convention, go figure return roleNames; > SidebarMode.cpp:588 > + QModelIndex categoryIdx = > d->categorizedModel->index(d->activeCategoryRow, 0); > + auto item = categoryIdx.data(Qt::UserRole).value<MenuItem*>(); > + // If subcategory exist update from subcategory I'd feel better with a Q_ASSERT(item) > SidebarMode.cpp:590 > + // If subcategory exist update from subcategory > + if (!item->children().empty()) { > + item = item->child(d->activeSubCategoryRow); Technically correct, we tend to favor the use of `isEmpty()` though. > CategoriesPage.qml:206 > + id: defaultMarker > + radius: width*0.5 > + width: Kirigami.Units.largeSpacing Missing spaces around * > SubCategoryPage.qml:195 > + > + Rectangle { > + id: defaultMarker This is twice the same Rectangle item, what about we make a reusable element and use it at both places to reduce code duplication? > SubCategoryPage.qml:197 > + id: defaultMarker > + radius: width*0.5 > + width: Kirigami.Units.largeSpacing Spaces missing around * REPOSITORY R124 System Settings REVISION DETAIL https://phabricator.kde.org/D28461 To: bport, #plasma, ervin, meven, crossi, hchain, #vdg, mart Cc: mart, ngraham, abetts, filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, ahiemstra