Differential D8492

Make SourceFormatter plugins enablable, hide actions with no plugins
ClosedPublic
Actions

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

Details

Reviewers
kfunk
mwolff
Group Reviewers
KDevelop
Commits
R32:a6a42ec30a7c: Make SourceFormatter plugins enablable, hide actions with no plugins
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
Branch
makeFormatterPluginsDisableable
Lint
No Linters Available
Unit
No Unit Test Coverage
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
kossebau added a comment.Nov 16 2017, 4:55 PM

*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
192 ↗(On Diff #21366)

Shouldn't this be >= 1?

192 ↗(On Diff #21366)

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.

725 ↗(On Diff #21366)

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
192 ↗(On Diff #21366)

== 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?

192 ↗(On Diff #21366)

resetUi() done as proposed.

mwolff added a subscriber: mwolff.Nov 19 2017, 8:32 PM
mwolff added inline comments.
kdevplatform/shell/sourceformattercontroller.cpp
192 ↗(On Diff #21366)

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.

318 ↗(On Diff #21366)

future optimization opportunity: use splitRef

326 ↗(On Diff #21366)

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

kdevplatform/shell/sourceformatterselectionedit.cpp
121 ↗(On Diff #21366)

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

181 ↗(On Diff #21366)

use remove_if + erase

204 ↗(On Diff #21366)

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

244 ↗(On Diff #21366)

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

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 ↗(On Diff #21366)

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 ↗(On Diff #21366)

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

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

kossebau added a comment.Nov 23 2017, 4:02 PM

@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
186 ↗(On Diff #21366)

Minor: Would prefer early-return here.

if (!sourceFormatter) {
    return;
}
202 ↗(On Diff #21366)

Dito

kdevplatform/shell/sourceformatterselectionedit.cpp
253 ↗(On Diff #21366)

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 ↗(On Diff #21366)

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
kossebau added a comment.Nov 28 2017, 4:51 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
Closed by commit R32:a6a42ec30a7c: Make SourceFormatter plugins enablable, hide actions with no plugins (authored by kossebau). · Explain WhyNov 30 2017, 11:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 22489

View Options

kdevplatform/shell/mainwindow.cpp

Loading...
View Options

kdevplatform/shell/mainwindow_p.h

Loading...
View Options

kdevplatform/shell/mainwindow_p.cpp

Loading...
View Options

kdevplatform/shell/sourceformattercontroller.h

Loading...
View Options

kdevplatform/shell/sourceformattercontroller.cpp

Loading...
View Options

kdevplatform/shell/sourceformatterselectionedit.h

Loading...
View Options

kdevplatform/shell/sourceformatterselectionedit.cpp

Loading...
View Options

plugins/astyle/kdevastyle.json

Loading...
View Options

plugins/customscript/kdevcustomscript.json

Loading...
View Options

plugins/sourceformatter/sourceformatterplugin.cpp

Loading...