Make SourceFormatter plugins enablable, hide actions with no plugins
ClosedPublic

Authored by kossebau on Oct 26 2017, 11:16 AM.

Details

Summary

Source formatting is a feature seemingly only used by some.
So it makes sense to allow disabling this feature, for some minimal smaller
runtime footprint and less unused clutter in the UI (like menu actions or
settings).

This patch achieves this to a good degree:

  • makes the plugins astyle & customscript normal global plugins which can be enabled/disabled by the user
  • removes the SourceFormatterController actions from the menus when there are no formatters available
  • shows no source formatting settings page in the project settings when there are no formatters available

Not yet done is to hide the source formatting settings page from the
application settings dialog in case no formatters are available. That might
need some more custom logic all over the shell code, which is not so nice.
Instead the option should be investigated to make the source formatting
controller a normal plugin with a suited interface, which then can be
queried by other code needing that service.

Test Plan

See plugins astyle & customscript turn up in plugin selection settings.
Disable and enable both and see how actions in the main Edit menu and the
context menu on file items are present or not present, as well as the
formatting settings page in the project settings dialog being shown or not.

See also the formatting settings pages for projects and the global one only
offering the formatters of the enabled plugins.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Oct 26 2017, 11:16 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 26 2017, 11:16 AM

*ping* Any Yay/Nay/Whut/Dontcare/Meh?

kfunk requested changes to this revision.Nov 16 2017, 5:23 PM
kfunk added a subscriber: kfunk.

Not yet done is to hide the source formatting settings page from the application settings dialog in case no formatters are available.

Not sure if this is over the top.

kdevplatform/shell/sourceformattercontroller.cpp
190

Shouldn't this be >= 1?

190

Would factor this out into a separate function (i.e. resetUi() as you did in the other class) and use that from both pluginLoaded and unloadingPlugin.

727

Minor: Remove extra parens

This revision now requires changes to proceed.Nov 16 2017, 5:23 PM
kossebau updated this revision to Diff 22489.Nov 16 2017, 9:24 PM
kossebau marked 3 inline comments as done.

integrate kfunk's review feedback

kdevplatform/shell/sourceformattercontroller.cpp
190

== 1 only. Because for > 1 the actions should have been already enabled before, no state change when there are more than one plugin. Unless things are flawed elsewhere,, which they should not :)
would you prefer better safe-then-sorry logic instead?

190

resetUi() done as proposed.

mwolff added a subscriber: mwolff.Nov 19 2017, 8:32 PM
mwolff added inline comments.
kdevplatform/shell/sourceformattercontroller.cpp
190

add a comment on the == 1 case to explain why this is OK.

Also, personal pet-peeve: please try to use size() everywhere in preference over count(), to make this more in align with C++/STL.

320

future optimization opportunity: use splitRef

328

use .first() instead of .at(0)

kdevplatform/shell/sourceformatterselectionedit.cpp
121

auto* , to make clear this is a pointer. Otherwise it should be const & or const &&

181–195

use remove_if + erase

204

this cleanup could be done in a separate patch and committed directly

244

I realize this is old code, but could you do me a favor and clean it up (potentially in a follow-up commit?)

It makes my eyes bleed. Instead, do something like

// Sort the languages, preferring firstly active, then loaded languages, then any others
for (const auto& languages : {ICore::self()->languageController()->activeLanguages(),
                              ICore::self()->languageController()->loadedLanguages()})
{
    for (const auto* language : languages) {
        if (d->languages.contains(language->name()) {
            auto it = std::find(languagesToAdd.begin(), languagesToAdd.end(), language);
            if (it == languagesToAdd.end()) {
                return;
            }
            languagesToAdd.erase(it);
            d->ui.cbLanguages->addItem(name);
        }
    }
}

for (const auto &language : languagesToAdd) {
    d->ui.cbLanguages->addItem(language);
}
mwolff added inline comments.Nov 19 2017, 9:43 PM
plugins/astyle/kdevastyle.json
31

unrelated?

plugins/customscript/kdevcustomscript.json
31

unrelated? different patch?

plugins/sourceformatter/sourceformatterplugin.cpp
43–44

why not simply make hasFormatters public, i.e. add it to the sourceFormatterController API?

mwolff requested changes to this revision.Nov 19 2017, 9:53 PM
This revision now requires changes to proceed.Nov 19 2017, 9:53 PM
kossebau updated this revision to Diff 22620.Nov 19 2017, 10:39 PM
kossebau marked 6 inline comments as done.

integrate milia's review

kdevplatform/shell/sourceformatterselectionedit.cpp
181–195

Idea here is that while iterating over the structure to delete things at the same time also data is updated, cmp. the snippet behind // reset selected formatter if needed.
Would you prefer thus another dedicated loop for that, instead of one integrated?

204

What looks like a clean-up though is switching from a QStringList to iterate over to a QMap. With code which is reused from the old version, but actually a left-over from a method whose content is now distributed over multiple methods.

The old code used the sorted list of languages also for loading from the config, where though the sorting does not matter, so the new code which just cares for the loading iterates directly over the map. I don't think another intermediate change here helps tracking the changes?

plugins/sourceformatter/sourceformatterplugin.cpp
43–44

I did not dare to also propose changes to the abstract interface yet. But followed your suggestion, added together with signal.

@mwolff : satisfied with my replies & code changes to your review? @kfunk Any more comments from your side?

kfunk accepted this revision.Nov 23 2017, 4:23 PM

Please wait for Milian to give a +1, too

kdevplatform/shell/sourceformattercontroller.cpp
184

Minor: Would prefer early-return here.

if (!sourceFormatter) {
    return;
}
200

Dito

kdevplatform/shell/sourceformatterselectionedit.cpp
253

Minor: Odd coding style

plugins/customscript/kdevcustomscript.json
31

+1. Push separately.

kossebau updated this revision to Diff 22840.Nov 23 2017, 4:39 PM
kossebau marked an inline comment as done.

add early return as requested by kfunk

kdevplatform/shell/sourceformatterselectionedit.cpp
253

See Milian's comment above. Old code which was moved around (but the diff display manages to blur this :/) Candidate for separate clean-up as requested above by Milian.

kossebau updated this revision to Diff 22841.Nov 23 2017, 4:42 PM

the other as well, sigh at editor not doing autosave&commitamend
where is AI when really needed

kossebau marked an inline comment as done.Nov 23 2017, 4:43 PM

@mwolff ping? satisfied with my replies & code changes to your review in D8492#169755 ?

mwolff accepted this revision.Nov 30 2017, 11:10 PM

a quick glance doesn't show anything bad, go for it!

thanks

This revision is now accepted and ready to land.Nov 30 2017, 11:10 PM
This revision was automatically updated to reflect the committed changes.