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
