Detangle the code, implement models and start the Qml
Diff Detail
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- kcm_formats
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 19037 Build 19055: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
| kcms/formats/localemodel.cpp | ||
|---|---|---|
| 69 | for (const auto &localbe: qAsConst(allLocales)) | |
| 83 | Space before & not after | |
| 89 | Ditto | |
| 104 | Ditto | |
| 106 | This sounds like a std::find_if | |
| 121 | You could just have return { here | |
| kcms/formats/package/contents/ui/main.qml | ||
| 34 | Shouldn't controls be deactivated based on the keys mutability here? | |
Comment Actions
- Correctly extract qml
- Update documentation text
- Remove deprecated calls
- Correct placement of pointer and reference symbols
- Rename variables on kconfigxt and use name and key
- Correct constructor
- Use auto
- Use qAsConst
- Return a map directly
- Inheit from the generated settings to use usrSave
| kcms/formats/Messages.sh | ||
|---|---|---|
| 4 | I don't think this is necessary, at least it's the first time I have ever seen this sort of thing | |
| kcms/formats/kcm.cpp | ||
| 38 | You're not importing this anywhere, and this still works? Makes me wonder why Qt went through the trouble of introducing this when it makes no diference over qmlRegisterType<T>() ... Please also use a KCM-specific import name for this, such as org.kde.private.kcms.formats | |
| kcms/formats/kcm.h | ||
| 40 | virtual isn't necessary here | |
| 43 | Name this updateExampleStrings and where is it actually being called from? | |
| 49 | Remove whitespace | |
| kcms/formats/kcm_formats.desktop | ||
| 15 | Yeah, what is it about? :) | |
| kcms/formats/localemodel.cpp | ||
| 86 | Ensure that it's not trying to make you a tree model: if (parent.isValid()) {
return 0;
}
return m_data.count(); | |
| 92 | Use checkIndex(idx) which will also ensure it is within bounds iirc | |
| kcms/formats/localemodel.h | ||
| 2 | Missing copyright header | |
| 9 | Unused | |
| 17 | declare as movable type | |
| 19 | { on new line | |
| 22 | Use Qt::DisplayRole for LocaleName Also, what is this ominous "value"? Please give it a clearer name. | |
| 23 | explicit | |
| kcms/formats/package/contents/ui/main.qml | ||
| 39 | QAbstractItemModel::match is usable from QML, then you don't need to roll your own indexFor method | |
| 42 | Have you tried QQC2.ItemDelegate? | |
| 47 | Use onActivated | |
| 48 | QAbstractItemModel::data is usable from QML: var idx = kcm.model.index(currentIndex, 0); kcm.settings.defaultLanguage = kcm.model.data(idx, YourModel.LocaleValueRole); If we're depending on Qt 5.14 there's now also a valueRole property you can set and then just use currentValue | |
| 52 | Does this have to be section or something? Iirc FormLayout has a checkable section feature | |
| 63 | Use onActivated and everywhere else | |
Comment Actions
- Change how we set locales
- Set the locale
- Add missing files
- Typos
- Set the correct size of the list
Comment Actions
Doesn't build for me:
[ 67%] Building CXX object kcms/spellchecking/CMakeFiles/kcmspellchecking.dir/kcmspellchecking_autogen/mocs_compilation.cpp.o
/home/nate/kde/src/plasma-desktop/kcms/formats/kcm.cpp: In constructor ‘Formats::Formats(QObject*, const QVariantList&)’:
/home/nate/kde/src/plasma-desktop/kcms/formats/kcm.cpp:39:5: error: ‘qmlRegisterAnonymousType’ was not declared in this scope; did you mean ‘qmlRegisterSingletonType’?
39 | qmlRegisterAnonymousType<FormatsSettings>("kcm.kde.org", 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~
| qmlRegisterSingletonTypeAlso it would be nice if you could put a screenshot in the Test Plan section so VDG people can do basic UI review without having to compile it.
Comment Actions
I'm not a huge fan of that language list selection mode, it's unlike anything we use anywhere else.
Most importantly, it makes the KCM take *forever* to load as it creates the entire list at once. Instead, you want to use the ListView properly so only that part scrolls.
I also find it quite unclear which part I'm editing now. Also, keyboard navigation is missing there, i.e. I can't click on the language, start typing or use the arrow keys.
Imho a custom ComboBox with a search field in its popup or something like that would be a better control.
| kcms/formats/formatsettings_impl.cpp | ||
|---|---|---|
| 20 ↗ | (On Diff #73135) | This file is missing |
| kcms/formats/kcm.cpp | ||
| 48 | Just i18n("Formats")? "configuration module" is quite geeky | |
| kcms/formats/package/contents/ui/CountryList.qml | ||
| 32 ↗ | (On Diff #73135) | No word puzzzles: i18n("Setting locale for %1", currentLocale.text) |
| kcms/formats/package/contents/ui/main.qml | ||
| 80 | Why the spaces? | |
| 84 | Copy paste error? | |
| kcms/formats/kcm.cpp | ||
|---|---|---|
| 40 | this is 5.14 only from which we can't depend yet | |
Comment Actions
Imho a custom ComboBox with a search field in its popup or something like that would be a better control.
I tried to use the combobox (but of course phabricator destroys my history so it's hard to even look at it). I had problems with the velocity because of the amount of items that are loaded in the model and the flag pixmaps.
| kcms/formats/kcm.cpp | ||
|---|---|---|
| 40 | ah, ok. I was having deprecated warnings, that's why I removed. | |
Comment Actions
tested it:
- i have al horizontal scroll of the left column that should never happen:
- opening it in kcmshell tries to resize the window to 100% height of the screen
- it tries to resize the list of locales to its full height and put a scrollbar on the whole kcm, rather than doing a scrollable list, which makes the left column unusable (and the right column slower to load)
Comment Actions
design wise i would go this way:
left column:
A FormLayout with labels and buttons aligned in the usual standard centered way
right Column:
Either an overlaysheet that opens over everything like the translations kcm, or if you want to keep the drag and drop possibility, a Drawer that opens on the right (with modal set to false) so it can reasonably both have a normal dialog bahavior (click on button, open it, click on the language, close) or drag and drop over the buttons
| kcms/formats/kcm.cpp | ||
|---|---|---|
| 38 | I also don't know but now I have deprecated warnings on the terminal to change for that. | |
| kcms/formats/localemodel.cpp | ||
| 15 | old, unmodified code. Changing now. | |
| 106 | it does, but the std::find_if has a worse readability than a for loop, at least for simple cases. I'm excited to be able to use ranges tougth. | |
Comment Actions
- Remove code dependant on Qt 5.14
- Mark that we will use some text in the future
- Remove uneeded virtual
- Update role names to something more explicit
- Correctly handle wrong indices
- Update Qml Library
- Handle keyboard navigation
- Use correct method for extracting data from the model
- Nitpicks
- Correctly compare locales
- Fix metadata
Comment Actions
Got this tested.
I'm sorry to say that I really really really really don't like the pop-out drawer-style list at all. It's unlike the way we do pop-up lists anywhere else, its list scrolls much too fast, the list items overlap the search field when scrolling, it can't be closed with the escape key and it just overall looks terrible and really feels cosmically wrong to me. Please use a list in an overlaysheet instead, like how the translations KCM does it.
I think when we redo a KCM, we either need to follow the VDG mockup if there is one, work with them to create one if there isn't one, or else mimic the existing style as closely as possible. Otherwise we just go back and forth with the design in the patch itself which is presumably disheartening and exhausting for poor @tcanabrava. :)
Comment Actions
With OverlaySheet
| kcms/formats/kcm.cpp | ||
|---|---|---|
| 42 | The non obsolete version is in Qt 5.14, we can't use yet. | |
Comment Actions
We should use a more conventional approach of having an actual framed scrollview below a search field, with both vertically aligned to one another, and with margins around all of them.
The preview area from the current KCM is missing.
In the sheet, the scrollview below the search field seems to be on top of the search field, and the sheet's close button is inside it:
In the sheet, the scrollbar scrolls the sheet itself, not the list inside it, and when you scroll in the list, the scrollbar doesn't change:
The scroll speed in the sheet is way too fast when using a touchpad.
The Reset button doesn't do anything when clicked.
The Defaults button doesn't do anything when clicked.
Comment Actions
More generally, I'm not sure the interaction model of the Detailed Settings part of this KCM (both old and new) makes any sense. Once I choose my global locale, if I want to override individual elements, I want to choose the exact format I want, which I currently can't do.
For example, if I want to change to 24-hour time, my expectation is that I could have the option to override the time to 12 hour or 24-hour directly. Instead I see the same old list of all locales, and I have to guess which locale has 24-hour time by default. Does Botswana have 24-hour time? How about Chile? Maybe Australia? Who knows? If I want to change the number format, I want to actually directly manipulate the format itself, not choose a different locale where again I have no idea what format they use in that locale.
I think if we want to make a major UX improvement to this KCM, we should consider allowing users to directly choose the formats they want in the Detailed Settings section.
Comment Actions
Note that the interaction model is that way because that's how the system locale works. The KCM sets a bunch of environment variables (LC_ALL and related) which Qt then uses to determine what format to use. These generally correspond to certain country formats. While in theory it might be possible to create custom locales, I have no idea how hard that is to do.
That said, we could still display an actual format string instead of the country name, as I agree those are pretty useless.