KCM/Component Refactor UI to a single list of combobox
ClosedPublic

Authored by meven on Jan 20 2020, 6:12 PM.

Details

Summary

Refactor UI to a single list of Combobox:

  • Simplify code
Test Plan

Before:

After:

In systemsettings :

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Jan 20 2020, 6:12 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 20 2020, 6:12 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Jan 20 2020, 6:12 PM
meven edited the test plan for this revision. (Show Details)
meven edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Jan 20 2020, 6:45 PM

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:

This revision is now accepted and ready to land.Jan 20 2020, 6:45 PM
ervin requested changes to this revision.Jan 21 2020, 7:32 AM
ervin added inline comments.
kcms/componentchooser/componentchooser.cpp
81

I know it's older code but since you touched it, what about adding the spaces around == ?
Likewise for the few ifs below.

98

Curly brace on its own line.

100

Does that make any sense to keep the val parameter on that method if you loop through all the plugins because of defaulted anyway?
If it goes away the two loops could thus be merged.

103

Space before = missing

104

No need for the spaces within the parenthesis

110

Ditto

111

Ditto

122–131

Space before =

127

No space after *, no space within parenthesis

136–143

Ditto

141

Ditto

This revision now requires changes to proceed.Jan 21 2020, 7:32 AM
meven updated this revision to Diff 73993.Jan 21 2020, 8:32 AM
meven marked 11 inline comments as done.

Code formatting, only one loop in ComponentChooser::emitChanged, indentation tabs vs spaces

ervin added inline comments.Jan 21 2020, 10:31 AM
kcms/componentchooser/componentchooser.cpp
100

I still think val is useless.

meven added inline comments.Jan 21 2020, 10:38 AM
kcms/componentchooser/componentchooser.cpp
100

I kept it as the compiler might be able to do some optimization in the loop when val is true line 105.
It may skip the plugin->hasChanged(); calls in that case, but I am not sure it gcc is clever enough to accomplish this.

ervin added inline comments.Jan 21 2020, 11:00 AM
kcms/componentchooser/componentchooser.cpp
100

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.

meven updated this revision to Diff 74008.Jan 21 2020, 12:32 PM

change emitChanged(bool) to emitChanged()

meven marked 3 inline comments as done.Jan 21 2020, 12:32 PM
meven updated this revision to Diff 74011.Jan 21 2020, 12:33 PM

Add copyright mentions

ervin accepted this revision.Jan 21 2020, 12:36 PM
This revision is now accepted and ready to land.Jan 21 2020, 12:36 PM

@ngraham I think I should wait for some feedback/review from VDG or Plasma to iron out issues if any.

meven updated this revision to Diff 74012.Jan 21 2020, 12:39 PM

Add missing line change

meven edited the test plan for this revision. (Show Details)Jan 21 2020, 12:40 PM

@ngraham I think I should wait for some feedback/review from VDG or Plasma to iron out issues if any.

For sure.

meven added a comment.Jan 23 2020, 2:14 PM

@ngraham I think I should wait for some feedback/review from VDG or Plasma to iron out issues if any.

For sure.

Would love to find a reviewer from VDG or Plasma

broulik added inline comments.
kcms/componentchooser/componentchooser.cpp
86–87

Superfluous space before :

86–87

Use new connect syntax

98

Check if loadedConfigWidget is null

103

isDefaults

105

You don't actually use the key, so you can just use range-for.

122–131

normal constBegin() and then it.key() and it.value() below

142

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–153

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.

It will be parted of the plugin interface which I plan for KDE 3.2.

Well, so much for that I guess?

kcms/componentchooser/componentchooserbrowser.cpp
87

As a future step I would like those default components not hardcoded in the code

94–96

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.
Now it keeps showing "kdialog" no matter what default browser I use.

100

Don't use QStringLiteral(), use QString() for a null string.
But in this case you can just omit the last argument altogether.

kcms/componentchooser/componentchooseremail.cpp
126

So here you do show an entryPath but not in the other components?

kcms/componentchooser/componentchooserfilemanager.cpp
44

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?

filipf added a subscriber: filipf.Jan 23 2020, 3:09 PM

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).

meven added a comment.Jan 23 2020, 4:57 PM

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
98

I'd rather have a Q_ASSERT : this isn't supposed to happen

meven updated this revision to Diff 74304.Jan 24 2020, 11:34 AM
meven marked 11 inline comments as done.
  • Make CfgPlugin a QComboBox with default and currently saved index handling
  • General code improvements : in loops, naming...
  • set the minimal width of combobox 18 making them most of the time of same lenght
  • Correct typo in license declaration
meven edited the test plan for this revision. (Show Details)Jan 24 2020, 11:35 AM
meven edited the test plan for this revision. (Show Details)
meven added inline comments.
kcms/componentchooser/componentchooseremail.cpp
126

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".
But this new application service has no icon defined, so a user would be confused to see two entries for Thunderbird in the list, one with the icon which is not selected.
I meant here to display something to the user to help distinguish between the two.

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.

ervin requested changes to this revision.Jan 28 2020, 5:36 PM
ervin added inline comments.
kcms/componentchooser/componentchooser.cpp
87

Missing space after comma

105

No space after * and missing qAsConst

127

No space within parenthesis

136–143

Missing space before {

141

No space between parenthesis

153–155

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
87

Yes, it screams for GUI / config separation (in another commit)

This revision now requires changes to proceed.Jan 28 2020, 5:36 PM
meven updated this revision to Diff 74541.Jan 29 2020, 5:07 AM
meven marked 7 inline comments as done.

Code formatting, add qAsConst, only search for one directory kcm_componentchooser to find its components

ervin accepted this revision.Jan 29 2020, 8:11 AM
This revision is now accepted and ready to land.Jan 29 2020, 8:11 AM
meven added a comment.Jan 30 2020, 8:32 AM

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.

This revision was automatically updated to reflect the committed changes.