D28461: In sidebar mode show if a module is in default state or not

[Kevin Ottens]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200407/25e61b36/attachment-0001.html>