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

[Benjamin Port]

bport added a comment.


  Default indicator discussion is now on https://phabricator.kde.org/T13008

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg, mart
Cc: GB_2, mart, ngraham, abetts, filipf, The-Feren-OS-Dev, ndavis, broulik, 
plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, fbampaloukas, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, 
apol, ahiemstra

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

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.


  LGTM

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg, mart
Cc: GB_2, mart, ngraham, abetts, filipf, The-Feren-OS-Dev, ndavis, broulik, 
plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, fbampaloukas, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, 
apol, ahiemstra

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

[Björn Feber]

GB_2 added a comment.


  In D28461#640439 , 
@The-Feren-OS-Dev wrote:
  
  > How about instead of a blue dot there's instead an option in the SySe 
hamburger button menu to use search to filter out any settings pages where 
settings haven't been changed, meaning that only settings with changes made to 
them are then listed in the search results?
  
  
  I personally would like this much more than having a blue dot, otherwise it 
could get very distracting or cluttered if you made changes in many different 
KCMs.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg, mart
Cc: GB_2, mart, ngraham, abetts, filipf, The-Feren-OS-Dev, ndavis, broulik, 
plasma-devel, Orage, LeGast00n, cblack, jraleigh, zachus, fbampaloukas, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, 
apol, ahiemstra

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

[Benjamin Port]

bport updated this revision to Diff 79771.
bport added a comment.


  - Factorize QML code
  - Rename isDefault to showDefaultIndicator

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28461?vs=79499=79771

REVISION DETAIL
  https://phabricator.kde.org/D28461

AFFECTED FILES
  core/MenuItem.cpp
  core/MenuItem.h
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h
  sidebar/package/contents/ui/CategoriesPage.qml
  sidebar/package/contents/ui/CategoryItem.qml
  sidebar/package/contents/ui/SubCategoryPage.qml

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

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 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();
> +// 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

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

[Benjamin Port]

bport updated this revision to Diff 79499.
bport added a comment.


  Change icon with a blue dot

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28461?vs=78970=79499

REVISION DETAIL
  https://phabricator.kde.org/D28461

AFFECTED FILES
  core/MenuItem.cpp
  core/MenuItem.h
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h
  sidebar/package/contents/ui/CategoriesPage.qml
  sidebar/package/contents/ui/SubCategoryPage.qml

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

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

[Benjamin Port]

bport added inline comments.

INLINE COMMENTS

> mart wrote in CategoriesPage.qml:207
> try to keep the item count as small as possible, especially in item delegates.
> I would really prefer if we could do without this wrapper

Indeed this wrapper is not needed

> mart wrote in SubCategoryPage.qml:198
> width: Kirigami.Units.iconSizes.small

Thanks I will fix that

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

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

[Marco Martin]

mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> broulik wrote in CategoriesPage.qml:207
> `Kirigami.Units.iconSizes.small` and elsewhere
> 
> Why do you need this wrapper `Item`, though?

try to keep the item count as small as possible, especially in item delegates.
I would really prefer if we could do without this wrapper

> SubCategoryPage.qml:198
> +
> +width: 16
> +height: 16

width: Kirigami.Units.iconSizes.small

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

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

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  How about instead of a blue dot there's instead an option in the SySe 
hamburger button menu to use search to filter out any settings pages where 
settings haven't been changed, meaning that only settings with changes made to 
them are then listed in the search results?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Benjamin Port]

bport added a comment.


  Yes we will need to have a probe for each KCM, and I currently look to reuse 
this probe in the KCM that will split a bit more data and UI.
  Some KCM like pulseaudio one don't need that for example, because there is no 
default setup

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Noah Davis]

ndavis added a comment.


  In D28461#639470 , @ngraham wrote:
  
  > So every single KCM will need to be patched to add a settings proper in 
support of this? Oof. Is there no easier way?
  
  
  Perhaps we could find a way to turn it to our advantage? Do //all// KCMs 
really need to have this kind of indicator?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Nathaniel Graham]

ngraham added a comment.


  So every single KCM will need to be patched to add a settings proper in 
support of this? Oof. Is there no easier way?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Benjamin Port]

bport added a comment.


  In D28461#639056 , @ngraham wrote:
  
  > Is D28460  the only other patch that's 
needed? I applied that and then this patch compiled fine, but I don't see any 
difference in the sidebar, despite having changed some settings from their 
default values.
  
  
  No we also need to add probe to each KCM there is an example with the color 
one there D28462  (locally I have more 
example but need some code cleanup and not needed for code review so I will do 
code cleanup after probe API cleanup is done i.e. patch D28460 
)

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Nathaniel Graham]

ngraham added a comment.


  Is D28460  the only other patch that's 
needed? I applied that and then this patch compiled fine, but I don't see any 
difference in the sidebar, despite having changed some settings from their 
default values.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Benjamin Port]

bport added a comment.


  In D28461#639022 , @ngraham wrote:
  
  > I get a compile error:
  >
  >   /home/nate/kde/src/systemsettings/core/MenuItem.cpp: In member function 
‘void MenuItem::updateDefault()’:
  >   /home/nate/kde/src/systemsettings/core/MenuItem.cpp:154:36: error: 
‘isDefaults’ is not a member of ‘KCModuleLoader’
  > 154 | d->isDefault = KCModuleLoader::isDefaults(d->item);
  > |^~
  >
  >
  > Does this have un-merged dependencies? If so, you can mark them in Phab by 
adding the text `Depends on D12345` (but the actual patch ID, not D12345 
 :) ) in the Summary section.
  
  
  Ok I will add it I thought setting a parent revision on the right sidebar was 
enough

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Nathaniel Graham]

ngraham added a comment.


  I get a compile error:
  
/home/nate/kde/src/systemsettings/core/MenuItem.cpp: In member function 
‘void MenuItem::updateDefault()’:
/home/nate/kde/src/systemsettings/core/MenuItem.cpp:154:36: error: 
‘isDefaults’ is not a member of ‘KCModuleLoader’
  154 | d->isDefault = KCModuleLoader::isDefaults(d->item);
  |^~
  
  Does this have un-merged dependencies? If so, you can mark them in Phab by 
adding the text `Depends on D12345` (but the actual patch ID, not D12345 
 :) ) in the Summary section.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: 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, mart

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

[Benjamin Port]

bport added a comment.


  In D28461#639016 , @abetts wrote:
  
  > If I am understanding correctly, this patch adds icons in the kcm list to 
indicate changes you can revert by going to the kcm. Basically, default 
settings have changed and now you have the possibility to see that changes have 
been made. I am not sure that I like the implementation. I have some questions.
  >
  > What will you do when all of them are changed in some way? Will the entire 
kcm list show an icon indicating change? If you think about it, KDE is the DE 
of changes and options. I feel all of these are going to show some kind of 
change, that could be overwhelming. At the end of the day, is it necessary? 
Wouldn't this be something that probably works best by having it in the 
reset/defaults button?
  
  
  Being the DE of "changes and options" don't mean all people are changing 
everything, but that also mean some people will do some change and then don't 
remember what they changed, and will never find what differ from the default 
values. Reset / Default button have totally different purpose, default button 
will set back default value and will be indeed active if module don't have 
default value. However, user will know that only if he goes to the good module. 
Problem for a "lambda" user is to remember what change.
  
  > Also, these icons indicate change, if you click on the item and then land 
in a kcm that doesn't present more clues as to what changed, then you are left 
with the default action to click defaults or reset on the kcm anyway. There 
doesn't seem to be much more value there.
  
  There is another patch for this purpose
  https://phabricator.kde.org/D27540

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: abetts, filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, 
LeGast00n, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
ahiemstra, mart

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

[Andres Betts]

abetts added a comment.


  If I am understanding correctly, this patch adds icons in the kcm list to 
indicate changes you can revert by going to the kcm. Basically, default 
settings have changed and now you have the possibility to see that changes have 
been made. I am not sure that I like the implementation. I have some questions.
  
  What will you do when all of them are changed in some way? Will the entire 
kcm list show an icon indicating change? If you think about it, KDE is the DE 
of changes and options. I feel all of these are going to show some kind of 
change, that could be overwhelming. At the end of the day, is it necessary? 
Wouldn't this be something that probably works best by having it in the 
reset/defaults button?
  
  Also, these icons indicate change, if you click on the item and then land in 
a kcm that doesn't present more clues as to what changed, then you are left 
with the default action to click defaults or reset on the kcm anyway. There 
doesn't seem to be much more value there.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: abetts, filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, 
LeGast00n, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
ahiemstra, mart

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

[Benjamin Port]

bport added a comment.


  F8208462: image.png 
  
  Test with a blue dot, took less space than an icon
  Wa can imagine a tooltip when you go hover the  dot explaining it

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, LeGast00n, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

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

[Noah Davis]

ndavis added a comment.


  maybe it would make more sense to add a different way to view non-default 
settings? like maybe a summary of custom settings.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, LeGast00n, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

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

[Benjamin Port]

bport added a comment.


  Goal of this feature is to improve user discoverability and allow them to 
find more easily which settings diverged from defaults one, so they can more 
easily revert to the default value when needed
  Indeed the current indicator is not the good one, and I will be happy to have 
some help to find a nicest one.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, LeGast00n, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

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

[Filip Fila]

filipf added a comment.


  Can you explain why we want this? It's going to be visually prominent so we 
need a good reason.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: filipf, The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, LeGast00n, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

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

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  IMHO this is wholly pointless a change, currently. Most people won't care 
about if they've made changes or not in SySe from the defaults, and quite 
frankly seeing that indicator on the sidebar would potentially even deter user 
customisation if not utterly confuse people. If it was an optional setting in 
settings to show these changed settings indicators on the sidebar (such as a 
checkbox item in the SySe menu) that you could turn off by default, I wouldn't 
mind this at all.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: The-Feren-OS-Dev, ndavis, broulik, plasma-devel, Orage, LeGast00n, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Noah Davis]

ndavis added a comment.


  This is merely an indicator and not a button, right? If so, you should not 
use `edit-reset` since you cannot actually reset the settings there. I'll try 
to come up with another indicator.
  
  I'm concerned that this indicator will make system settings visually messy. 
Is it really needed?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain, #vdg
Cc: ndavis, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added a comment.


  Please include screenshots when you do UI changes.

INLINE COMMENTS

> CategoriesPage.qml:207
> +
> +width: 16
> +height: 16

`Kirigami.Units.iconSizes.small` and elsewhere

Why do you need this wrapper `Item`, though?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

To: bport, #plasma, ervin, meven, crossi, hchain
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Benjamin Port]

bport created this revision.
bport added reviewers: Plasma, ervin, meven, crossi, hchain.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bport requested review of this revision.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D28461

AFFECTED FILES
  core/MenuItem.cpp
  core/MenuItem.h
  core/MenuModel.cpp
  core/MenuModel.h
  core/ModuleView.cpp
  core/ModuleView.h
  sidebar/SidebarMode.cpp
  sidebar/SidebarMode.h
  sidebar/package/contents/ui/CategoriesPage.qml
  sidebar/package/contents/ui/SubCategoryPage.qml

To: bport, #plasma, ervin, meven, crossi, hchain
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart