Refactor UI to a single list of Combobox:
- Simplify code
ngraham | |
ervin |
Plasma | |
VDG |
Refactor UI to a single list of Combobox:
Before:
After:
In systemsettings :
No Linters Available |
No Unit Test Coverage |
Buildable 21423 | |
Build 21441: arc lint + arc unit |
Wow, such a huge UI improvement.
This is a pre-existing issue, but I see duplicate entries in the copy that's built from source in my pde prefix location, which looks pretty hilarious in the new UI:
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
80 ↗ | (On Diff #73975) | I know it's older code but since you touched it, what about adding the spaces around == ? |
98 ↗ | (On Diff #73975) | Curly brace on its own line. |
100 ↗ | (On Diff #73975) | Does that make any sense to keep the val parameter on that method if you loop through all the plugins because of defaulted anyway? |
103 ↗ | (On Diff #73975) | Space before = missing |
104 ↗ | (On Diff #73975) | No need for the spaces within the parenthesis |
111 ↗ | (On Diff #73975) | Ditto |
112 ↗ | (On Diff #73975) | Ditto |
127 ↗ | (On Diff #73975) | Space before = |
132 ↗ | (On Diff #73975) | No space after *, no space within parenthesis |
141 ↗ | (On Diff #73975) | Ditto |
146 ↗ | (On Diff #73975) | Ditto |
Code formatting, only one loop in ComponentChooser::emitChanged, indentation tabs vs spaces
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
98 | I still think val is useless. |
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
98 | I kept it as the compiler might be able to do some optimization in the loop when val is true line 105. |
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
98 | I don't think you'd find a compiler doing this, there's nothing ensuring that hasChanged doesn't modify some mutable variable (cache management or such). I don't think one can safely spare the call in the general case. |
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
96 ↗ | (On Diff #73975) | Superfluous space before : |
97–98 ↗ | (On Diff #73975) | Use new connect syntax |
103 ↗ | (On Diff #73975) | isDefaults |
105 ↗ | (On Diff #73975) | You don't actually use the key, so you can just use range-for. |
108 ↗ | (On Diff #73975) | Check if loadedConfigWidget is null |
127–136 ↗ | (On Diff #73975) | normal constBegin() and then it.key() and it.value() below |
147 ↗ | (On Diff #73975) | In some places you check if your dynamic_cast worked and sometimes you don't. Since we only put CfgPlugin instanes in there, I don't think we need to check. See my note on the ominous plug-in architecture above. |
158 ↗ | (On Diff #73975) | The hash contains pointers, so auto * |
161 | It appears you dropped the generic component configuration? If that even was a thing... makes me wonder why this KCM uses desktop files when it effectively only operates on a fixed set of options.
Well, so much for that I guess? | |
kcms/componentchooser/componentchooserbrowser.cpp | ||
104 ↗ | (On Diff #73975) | As a future step I would like those default components not hardcoded in the code |
111–113 ↗ | (On Diff #73975) | Something funky is going on with this one: I configured a custom kdialog --sorry "%f" command line. It only shows up as "kdialog" so I can't tell what the actual command was and not edit it later. More importantly, though, I then changed my browser to be Kate. While it initially showed "Kate" with proper icon, when I re-open the KCM, it shows "kdialog" again as custom command, while still opening Kate when I open http URLs. |
117 ↗ | (On Diff #73975) | Don't use QStringLiteral(), use QString() for a null string. |
kcms/componentchooser/componentchooseremail.cpp | ||
149 ↗ | (On Diff #73975) | So here you do show an entryPath but not in the other components? |
kcms/componentchooser/componentchooserfilemanager.cpp | ||
54 ↗ | (On Diff #73975) | This seems to be the same in all classes. Since now all of them are just QComboBoxes, would it make sense to move any such functionality (and also a defaultIndex) into the base class? |
A lot cleaner and straightforward now, good job. I was wondering though what it would look like if we made all the comoboboxes have equal width (if possible).
I can make that happen, at some point they expanded to the right which wasn't ideal.
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
108 ↗ | (On Diff #73975) | I'd rather have a Q_ASSERT : this isn't supposed to happen |
kcms/componentchooser/componentchooseremail.cpp | ||
---|---|---|
149 ↗ | (On Diff #73975) | For email I have a slightly more advanced logic to read and display the "Added Associations" section of mimeapps.list and add it to the list of choices. I did this because at least for thunderbird, when it set itself as default browser it creates a new entry to "Added Associations" and use this new entry as choice in "Default Applications". I may want to reconsider and simply add whatever service is in Default Application and display it, ignoring entries in "Added Associations", as I do elsewhere. |
kcms/componentchooser/componentchooser.cpp | ||
---|---|---|
89 ↗ | (On Diff #73975) | Missing space after comma |
105 ↗ | (On Diff #73975) | No space after * and missing qAsConst |
132 ↗ | (On Diff #73975) | No space within parenthesis |
141–148 ↗ | (On Diff #73975) | Missing space before { |
146 ↗ | (On Diff #73975) | No space between parenthesis |
160 ↗ | (On Diff #73975) | Space before the * and not after. Also you could have used auto or auto * for all those loops (just that const auto & makes no sense in that context). |
kcms/componentchooser/componentchooserbrowser.cpp | ||
104 ↗ | (On Diff #73975) | Yes, it screams for GUI / config separation (in another commit) |
Code formatting, add qAsConst, only search for one directory kcm_componentchooser to find its components
I may want to reconsider and simply add whatever service is in Default Application and display it, ignoring entries in "Added Associations", as I do elsewhere.
I am thinking about doing this, as it makes the code more similar with other cfgplugin, and the current code in the email cfgpluginexisted mostly because it was the first one I edited.