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 21432 | |
Build 21450: 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 | ||
---|---|---|
62 | I know it's older code but since you touched it, what about adding the spaces around == ? | |
96 | Curly brace on its own line. | |
98 | Does that make any sense to keep the val parameter on that method if you loop through all the plugins because of defaulted anyway? | |
101 | Space before = missing | |
102 | No need for the spaces within the parenthesis | |
109 | Ditto | |
110 | Ditto | |
121–130 | Space before = | |
126 | No space after *, no space within parenthesis | |
135–142 | Ditto | |
140 | 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 | ||
---|---|---|
65 | Superfluous space before : | |
70 | Use new connect syntax | |
93 | Check if loadedConfigWidget is null | |
101 | isDefaults | |
103 | You don't actually use the key, so you can just use range-for. | |
121 | normal constBegin() and then it.key() and it.value() below | |
141 | 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. | |
152 | 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 | As a future step I would like those default components not hardcoded in the code | |
111 | 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 | Don't use QStringLiteral(), use QString() for a null string. | |
kcms/componentchooser/componentchooseremail.cpp | ||
149 | So here you do show an entryPath but not in the other components? | |
kcms/componentchooser/componentchooserfilemanager.cpp | ||
54 | 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 | ||
---|---|---|
93 | I'd rather have a Q_ASSERT : this isn't supposed to happen |
kcms/componentchooser/componentchooseremail.cpp | ||
---|---|---|
149 | 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 | ||
---|---|---|
62 | Missing space after comma | |
103 | No space after * and missing qAsConst | |
126 | No space within parenthesis | |
135–142 | Missing space before { | |
140 | No space between parenthesis | |
154 | 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 | 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.