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("<qt>You changed the default component of your choice, do want to save that change now ?</qt>"),QString(),KStandardGuiItem::save(),KStandardGuiItem::discard())==KMessageBox::Yes) save();
> -	}
> -	const QString &service = 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<CfgPlugin *>((*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<CfgPlugin*>( configWidget );
> -		if( plugin )
> -		{
> -			KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> -			plugin->load( &cfg );
> -		}
> +    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<CfgPlugin*>( 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 &configWidget : configWidgetMap) {
>          dynamic_cast<CfgPlugin*>(configWidget)->defaults();

The hash contains pointers, so `auto *`

> componentchooser.cpp:161
> -	{
> -		loadedConfigWidget = new CfgComponent(configContainer);
> -		static_cast<CfgComponent*>(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
> -    browserCombo->addItem(QIcon::fromTheme(QStringLiteral("application-x-shellscript")), i18n("Other..."), QStringLiteral());
> +    addItem(QIcon::fromTheme(QStringLiteral("application-x-shellscript")), i18n("Other..."), QStringLiteral());
>  

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

> componentchooseremail.cpp:149
>              const auto icon = QIcon::fromTheme(!service->icon().isEmpty() ? service->icon() : QStringLiteral("application-x-shellscript"));
> -            emailClientsCombo->insertItem(emailClientsCombo->count() - 1, icon, service->name() + " (" + KShell::tildeCollapse(service->entryPath()) + ")", service->storageId());
> +            insertItem(count() - 1, icon, service->name() + " (" + KShell::tildeCollapse(service->entryPath()) + ")", service->storageId());
>  

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

> componentchooserfilemanager.cpp:54
> +{
> +    return m_currentIndex != -1 && m_currentIndex != currentIndex();
>  }

This seems to be the same in all classes. Since now all of them are just `QComboBox`es, would it make sense to move any such functionality (and also a `defaultIndex`) into the base class?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  new-component-chooser

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

To: meven, #plasma, #vdg, ngraham, ervin
Cc: 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200123/7326eeda/attachment-0001.html>