D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:f55252fbf5d6: KCM/Component Refactor UI to a single list 
of combobox (authored by meven).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=74541=74892

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven added a comment.


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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arcpatch-D26797

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven updated this revision to Diff 74541.
meven marked 7 inline comments as done.
meven added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=74304=74541

BRANCH
  arcpatch-D26797

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> componentchooser.cpp:62
> +// fill the form layout
> +const auto name = cg.readEntry("Name",i18n("Unknown"));
> +CfgPlugin *loadedConfigWidget = 
> loadConfigWidget(cfg.group(QByteArray()).readEntry("configurationType"));

Missing space after comma

> componentchooser.cpp:106
> +// check if another plugin has changed and default status
> +for (CfgPlugin * plugin: configWidgetMap) {
> +somethingChanged |= plugin->hasChanged();

No space after * and missing qAsConst

> componentchooser.cpp:137
>  void ComponentChooser::save() {
> - if( configWidget )
> - {
> - CfgPlugin* plugin = dynamic_cast( configWidget );
> - if( plugin )
> - {
> - KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> +for (auto it = configWidgetMap.constBegin(); it != 
> configWidgetMap.constEnd(); ++it){
> +

Missing space before {

> componentchooser.cpp:142
> +
> +CfgPlugin *plugin = dynamic_cast( widget );
> +if (plugin) {

No space between parenthesis

> componentchooser.cpp:154
>  void ComponentChooser::restoreDefault() {
> -if (configWidget)
> -{
> -dynamic_cast(configWidget)->defaults();
> -emitChanged(true);
> +for (CfgPlugin* plugin : configWidgetMap) {
> +plugin->defaults();

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

> ervin wrote in componentchooser.cpp:132
> No space after *, no space within parenthesis

No space within parenthesis

> broulik wrote in componentchooserbrowser.cpp:104
> As a future step I would like those default components not hardcoded in the 
> code

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> broulik wrote in componentchooseremail.cpp:149
> So here you do show an `entryPath` but not in the other components?

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.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven updated this revision to Diff 74304.
meven marked 11 inline comments as done.
meven added a comment.


  - 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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=74012=74304

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven added a comment.


  In D26797#599747 , @filipf wrote:
  
  > 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.

INLINE COMMENTS

> broulik wrote in componentchooser.cpp:93
> Check if `loadedConfigWidget` is null

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Filip Fila]

filipf added a comment.


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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: filipf, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> componentchooser.cpp:65
>  
> - if (loadedConfigWidget) {
> - configWidgetMap.insert(service, loadedConfigWidget);
> - configContainer->addWidget(loadedConfigWidget);
> - connect(loadedConfigWidget, SIGNAL(changed(bool)), this, 
> SLOT(emitChanged(bool)));
> - }
> -}
> +QLabel *label = new QLabel(i18nc("The label for the combobox : 
> browser, terminal emulator...)", "%1:", name), this);
> +label->setToolTip(cfg.group(QByteArray()).readEntry("Comment", 
> QString()));

Superfluous space before `:`

> componentchooser.cpp:70
>  
> - if (somethingChanged) {
> - if (KMessageBox::questionYesNo(this,i18n("You changed the 
> default component of your choice, do want to save that change now 
> ?"),QString(),KStandardGuiItem::save(),KStandardGuiItem::discard())==KMessageBox::Yes)
>  save();
> - }
> - const QString  = it->data(Qt::UserRole).toString();
> - KConfig cfg(service, KConfig::SimpleConfig);
> +connect(loadedConfigWidget, SIGNAL(changed(bool)), this, 
> SLOT(emitChanged()));
>  

Use new connect syntax

> componentchooser.cpp:93
> +}
> +loadedConfigWidget->setSizeAdjustPolicy(QComboBox::AdjustToContents);
>  

Check if `loadedConfigWidget` is null

> componentchooser.cpp:101
> +bool somethingChanged = false;
> +bool isDefault = true;
> +// check if another plugin has changed and default status

`isDefaults`

> componentchooser.cpp:103
> +// check if another plugin has changed and default status
> +for (auto it = configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +CfgPlugin *plugin = dynamic_cast((*it).second);

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

> componentchooser.cpp:121
>  void ComponentChooser::load() {
> - if( configWidget )
> - {
> - CfgPlugin * plugin = dynamic_cast( configWidget );
> - if( plugin )
> - {
> - KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> - plugin->load(  );
> - }
> +for (auto it = configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +

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

> componentchooser.cpp:141
> +CfgPlugin *plugin = dynamic_cast( widget );
> +if (plugin) {
> +KConfig cfg(service, KConfig::SimpleConfig);

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.

> componentchooser.cpp:152
>  void ComponentChooser::restoreDefault() {
> -if (configWidget)
> -{
> +for (const auto  : configWidgetMap) {
>  dynamic_cast(configWidget)->defaults();

The hash contains pointers, so `auto *`

> componentchooser.cpp:161
> - {
> - loadedConfigWidget = new CfgComponent(configContainer);
> - 
> static_cast(loadedConfigWidget)->ChooserDocu->setText(i18n("Choose
>  from the list below which component should be used by default for the %1 
> service.", name));

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?

> componentchooserbrowser.cpp:104
>  }
>  if (service->storageId() == 
> QStringLiteral("org.kde.falkon.desktop")) {
> +m_falkonIndex = count() - 1;

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

> componentchooserbrowser.cpp:111
>  // we have a browser specified by the user
> -
> browserCombo->addItem(QIcon::fromTheme(QStringLiteral("application-x-shellscript")),
>  browser->name(), browser->storageId());
> -browserCombo->setCurrentIndex(browserCombo->count() - 1);
> -m_currentIndex = browserCombo->count() - 1;
> +
> addItem(QIcon::fromTheme(QStringLiteral("application-x-shellscript")), 
> browser->name(), browser->storageId());
> +setCurrentIndex(count() - 1);

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.

> componentchooserbrowser.cpp:117
>  // add a other option to add a new browser
> -
> 

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven added a comment.


  In D26797#598189 , @ngraham wrote:
  
  > In D26797#598030 , @meven wrote:
  >
  > > @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 


REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Nathaniel Graham]

ngraham added a comment.


  In D26797#598030 , @meven wrote:
  
  > @ngraham I think I should wait for some feedback/review from #VDG 
 or #Plasma 
 to iron out issues if any.
  
  
  For sure.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven updated this revision to Diff 74012.
meven added a comment.


  Add missing line change

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=74011=74012

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven added a comment.


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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven updated this revision to Diff 74011.
meven added a comment.


  Add copyright mentions

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=74008=74011

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven updated this revision to Diff 74008.
meven added a comment.


  change emitChanged(bool) to emitChanged()

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=73993=74008

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in 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.
> It may skip the `plugin->hasChanged();` calls in that case, but I am not sure 
> it gcc is clever enough to accomplish this.

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> ervin wrote in componentchooser.cpp:98
> I still think val is useless.

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> componentchooser.cpp:98
>  
> +void ComponentChooser::emitChanged(bool val)
> +{

I still think val is useless.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven updated this revision to Diff 73993.
meven marked 11 inline comments as done.
meven added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26797?vs=73975=73993

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> componentchooser.cpp:80
> +
> +if (cfgType==QLatin1String("internal_email")) {
> +loadedConfigWidget = new CfgEmailClient(this);

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

> componentchooser.cpp:98
>  
> -void ComponentChooser::slotServiceSelected(QListWidgetItem* it) {
> - if (!it) return;
> -
> - if (somethingChanged) {
> - if (KMessageBox::questionYesNo(this,i18n("You changed the 
> default component of your choice, do want to save that change now 
> ?"),QString(),KStandardGuiItem::save(),KStandardGuiItem::discard())==KMessageBox::Yes)
>  save();
> - }
> - const QString  = it->data(Qt::UserRole).toString();
> - KConfig cfg(service, KConfig::SimpleConfig);
> -
> - 
> ComponentDescription->setText(cfg.group(QByteArray()).readEntry("Comment",i18n("No
>  description available")));
> - ComponentDescription->setMinimumSize(ComponentDescription->sizeHint());
> -
> - configWidget = configWidgetMap.value(service);
> - if (configWidget) {
> -configContainer->setCurrentWidget(configWidget);
> -const auto plugin = dynamic_cast(configWidget);
> -headerGroupBox->setTitle(it->text());
> -plugin->load();
> -emit defaulted(plugin->isDefaults());
> - }
> -
> - emitChanged(false);
> - latestEditedService = service;
> -}
> +void ComponentChooser::emitChanged(bool val) {
>  

Curly brace on its own line.

> componentchooser.cpp:100
>  
> +bool somethingChanged = val;
> +if (!somethingChanged) {

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.

> componentchooser.cpp:103
> +// check if another plugin has changed
> +for (auto it= configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +CfgPlugin *plugin = dynamic_cast( (*it).second );

Space before = missing

> componentchooser.cpp:104
> +for (auto it= configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +CfgPlugin *plugin = dynamic_cast( (*it).second );
> +somethingChanged |= plugin->hasChanged();

No need for the spaces within the parenthesis

> componentchooser.cpp:111
> +bool isDefault = true;
> +for (auto it= configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +CfgPlugin *plugin = dynamic_cast( (*it).second );

Ditto

> componentchooser.cpp:112
> +for (auto it= configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +CfgPlugin *plugin = dynamic_cast( (*it).second );
> +isDefault &= plugin->isDefaults();

Ditto

> componentchooser.cpp:127
>  void ComponentChooser::load() {
> - if( configWidget )
> - {
> - CfgPlugin * plugin = dynamic_cast( configWidget );
> - if( plugin )
> - {
> - KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> - plugin->load(  );
> - }
> +for (auto it= configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +

Space before =

> componentchooser.cpp:132
> +
> +CfgPlugin * plugin = dynamic_cast( widget );
> +if (plugin) {

No space after *, no space within parenthesis

> componentchooser.cpp:141
>  void ComponentChooser::save() {
> - if( configWidget )
> - {
> - CfgPlugin* plugin = dynamic_cast( configWidget );
> - if( plugin )
> - {
> - KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> +for (auto it= configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it){
> +

Ditto

> componentchooser.cpp:146
> +
> +CfgPlugin* plugin = dynamic_cast( widget );
> +if (plugin) {

Ditto

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  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: F7894402: Screenshot_20200120_114347.png 


REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26797: KCM/Component Refactor UI to a single list of combobox

[Méven Car]

meven created this revision.
meven added reviewers: Plasma, VDG, ngraham, ervin.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
meven requested review of this revision.

REVISION SUMMARY
  Refactor UI to a single list of Combobox:
  
  - Simplify code

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

REVISION DETAIL
  https://phabricator.kde.org/D26797

AFFECTED FILES
  kcms/componentchooser/CMakeLists.txt
  kcms/componentchooser/browserconfig_ui.ui
  kcms/componentchooser/componentchooser.cpp
  kcms/componentchooser/componentchooser.h
  kcms/componentchooser/componentchooser_ui.ui
  kcms/componentchooser/componentchooserbrowser.cpp
  kcms/componentchooser/componentchooserbrowser.h
  kcms/componentchooser/componentchooseremail.cpp
  kcms/componentchooser/componentchooseremail.h
  kcms/componentchooser/componentchooserfilemanager.cpp
  kcms/componentchooser/componentchooserfilemanager.h
  kcms/componentchooser/componentchooserterminal.cpp
  kcms/componentchooser/componentchooserterminal.h
  kcms/componentchooser/componentconfig_ui.ui
  kcms/componentchooser/emailclientconfig_ui.ui
  kcms/componentchooser/filemanagerconfig_ui.ui
  kcms/componentchooser/kcm_componentchooser.cpp
  kcms/componentchooser/kcm_componentchooser.h
  kcms/componentchooser/terminalemulatorconfig_ui.ui

To: meven, #plasma, #vdg, ngraham, ervin
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart