Diff Detail
- Repository
- R124 System Settings
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 25019 Build 25037: arc lint + arc unit
Please include screenshots when you do UI changes.
| sidebar/package/contents/ui/CategoriesPage.qml | ||
|---|---|---|
| 191 | Kirigami.Units.iconSizes.small and elsewhere Why do you need this wrapper Item, though? | |
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?
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.
Can you explain why we want this? It's going to be visually prominent so we need a good reason.
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.
maybe it would make more sense to add a different way to view non-default settings? like maybe a summary of custom settings. It might have issues with organization, but it would require less navigation.
Test with a blue dot, took less space than an icon
Wa can imagine a tooltip when you go hover the dot explaining it
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.
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
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
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.
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?
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
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?
| core/MenuItem.h | ||
|---|---|---|
| 160 | 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... :-/ | |
| sidebar/SidebarMode.cpp | ||
| 117 | 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; | |
| 586 | I'd feel better with a Q_ASSERT(item) | |
| 588 | Technically correct, we tend to favor the use of isEmpty() though. | |
| sidebar/package/contents/ui/CategoriesPage.qml | ||
| 190 | Missing spaces around * | |
| sidebar/package/contents/ui/SubCategoryPage.qml | ||
| 195 | This is twice the same Rectangle item, what about we make a reusable element and use it at both places to reduce code duplication? | |
| 197 | Spaces missing around * | |
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.