KDevelop : "Reparse Entire Project" action in the project manager context menu
ClosedPublic

Authored by rjvbb on Apr 4 2018, 9:34 PM.

Details

Summary

The vast majority of the time I am perfectly happy running KDevelop with Settings/Projects/Schedule all project files for parsing unticked. The files I open and the headers they include are still being parsed which means I rarely really feel I miss functionality.

There are situations though in which I'd like an entire project to be (re)parsed. This change makes that a lot easier by adding an action to the Project Manager's context menu that works on the selected project or projects.

I've done a bit of refactoring to avoid code duplication. For now I've put the action in the context menu only; I'm not convinced this will be used often enough to warrant using the approach followed with the other menu actions which are all available in different menus.

Test Plan

Works as intended as far as I can tell.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7850
Build 7868: arc lint + arc unit
rjvbb requested review of this revision.Apr 4 2018, 9:34 PM
rjvbb created this revision.
mwolff requested changes to this revision.Jan 27 2019, 7:15 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
kdevplatform/language/backgroundparser/parseprojectjob.h
36 ↗(On Diff #31331)

please document the difference between forcing an update and forcing all - I've figured it out but it wasn't directly obvious to me

50 ↗(On Diff #31331)

move that to the dptr

kdevplatform/shell/projectcontroller.cpp
247 ↗(On Diff #31331)

QSet

1141 ↗(On Diff #31331)

you could add the forceAll arg there too, no?

This revision now requires changes to proceed.Jan 27 2019, 7:15 PM
rjvbb updated this revision to Diff 50830.Feb 4 2019, 11:31 AM
rjvbb marked 3 inline comments as done.

Updated as requested (minus the potential change to iProjectController::reparseProject).

rjvbb added inline comments.Feb 4 2019, 11:31 AM
kdevplatform/language/backgroundparser/parseprojectjob.h
36 ↗(On Diff #31331)

I can imagine, I had to figure it out for myself again too :-/

kdevplatform/shell/projectcontroller.cpp
247 ↗(On Diff #31331)

Am I missing a way to get direct access to a specific element in a QSet that doesn't use an intermediary QList or an iterator?

1141 ↗(On Diff #31331)

Yes, but then I also have to add it to iProjectController::reparseProject(), no? Are there any API/ABI breakage consequences of that? I find only a single iProjectController subclass in KDevelop itself so breakage may not be an issue?

rjvbb set the repository for this revision to R32 KDevelop.Feb 4 2019, 11:32 AM
mwolff requested changes to this revision.Feb 5 2019, 7:35 AM
mwolff added inline comments.
kdevplatform/shell/projectcontroller.cpp
247 ↗(On Diff #31331)

what do you mean by direct access to a specific element? by index? or do you want to get the first or last item?

1141 ↗(On Diff #31331)

sure, but that's fine - we can break API/ABI in master as much as we want

This revision now requires changes to proceed.Feb 5 2019, 7:35 AM
rjvbb added inline comments.Feb 5 2019, 11:01 AM
kdevplatform/language/backgroundparser/parseprojectjob.h
36 ↗(On Diff #31331)

In fact, what do you think of making this

explicit ParseProjectJob(KDevelop::IProject* project, bool forceUpdate = false, bool forceAll = ICore::self()->projectController()->parseAllProjectSources() );

You'd need to include 2 icore headers in this file but it'd make the behaviour of the option easier to explain. But we'd probably also need to do the same thing for the corresponding new argument to ProjectController::reparseProject() (but not iProjectController::reparseProjet(), I think)?

kdevplatform/shell/projectcontroller.cpp
247 ↗(On Diff #31331)

Well, the routine always checked if only a single project was selected because opening multiple project config dialogs isn't a very good idea (if not only because they're modal dialogs AFAICT).

So the answer to your question is "yes" :) We need access to the first, last, one-and-only element.

To be honest, I'm not convinced by the idea of making this collection a QSet. I doubt a performance argument carries much weight here and a QList seems more logical; elsewhere this information is used to loops over the projects. I for one always expect some influence of the order in which I select targets on the order in which commands are executed. I know that isn't always true but if it's not the execution order is usually the order in which the targets are displayed in the list. With a QSet the execution order will be unspecified.

1141 ↗(On Diff #31331)

Is there any other reason why this could not go into the current release branch?

mwolff added inline comments.Feb 10 2019, 9:56 PM
kdevplatform/language/backgroundparser/parseprojectjob.h
36 ↗(On Diff #31331)

please don't do that. rather, reuse ProjectController::reparseProject below instead of creating the job manually

kdevplatform/shell/projectcontroller.cpp
247 ↗(On Diff #31331)

deref the iterator returned by QSet::begin()

your QList won't work when you have multiple folders in a single project selected, you'll get a list with two entries, both pointing to the same project.

256 ↗(On Diff #31331)

double whitespace

1141 ↗(On Diff #31331)

it's not a bug fix, and you add new strings too

1145 ↗(On Diff #31331)

use ProjectController::reparseProject

1264 ↗(On Diff #31331)

expand this here

rjvbb marked 3 inline comments as done.Feb 11 2019, 2:19 PM
rjvbb added inline comments.
kdevplatform/language/backgroundparser/parseprojectjob.h
36 ↗(On Diff #31331)

The idea was to move the "should we parse everything and why" logic outside of the function; it's not entirely relevant where the condition is moved to. Could be to this function's declaration, but could probably also be to the reparseProject definition.
The point is that parameter could then simply be called "parseAll" and would be easier to explain. You'd also get more control over what the function does; in particular trigger a partial reparse no matter what parseAllProjectSources() returns.

kdevplatform/shell/projectcontroller.cpp
247 ↗(On Diff #31331)

I'm dereferencing the QSet::constBegin() iterator, is that wrong (and if so, why is it allowed?)

1141 ↗(On Diff #31331)

Well, outside of a freeze-before-a-release period of course...

1264 ↗(On Diff #31331)

did you mean anything more than just passing the forceAll argument?

rjvbb updated this revision to Diff 51412.Feb 11 2019, 2:52 PM

Updated as requested.

rjvbb set the repository for this revision to R32 KDevelop.Feb 11 2019, 2:53 PM
rjvbb added a project: KDevelop.
mwolff accepted this revision.Feb 11 2019, 6:12 PM

could you fixup the two minor nits please? then you can push directly

kdevplatform/interfaces/iprojectcontroller.h
156 ↗(On Diff #51412)

while at it, please lowercase the forceUpate

kdevplatform/shell/projectcontroller.cpp
1138 ↗(On Diff #31331)

style:

auto* action = new QAction(tr("Reparse the Entire Project"), this);
...

And is the capitalization HIG conform? I always forget it. Looks somewhat funny that the the is lower case but Entire isn't. But could be that this is correct - if this was a deliberate decision by you I'm fine with it.

This revision is now accepted and ready to land.Feb 11 2019, 6:12 PM
rjvbb added a comment.Feb 11 2019, 7:37 PM

auto* action = new QAction(tr("Reparse the Entire Project"), this);

Last minute check: tr instead of i18n, really?

And, yes, I think the lower case article ("the") is compliant with the style used throughout KDevelop where this kind of word usually not capitalised. (See also "Open Project for Current File" in the same file.

kossebau added inline comments.
kdevplatform/shell/projectcontroller.cpp
1138 ↗(On Diff #31331)

Relevant HIG is here: https://hig.kde.org/style/writing/capitalization.html#title-capitalization

So yes, "the" & Co. are lower-cased with the (English(Default) action titles.

auto* action = new QAction(tr("Reparse the Entire Project"), this);

Last minute check: tr instead of i18n, really?

Sorry, i18n in KDE projects of course.

And, yes, I think the lower case article ("the") is compliant with the style used throughout KDevelop where this kind of word usually not capitalised. (See also "Open Project for Current File" in the same file.

Thanks for checking you two!

This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.Feb 17 2019, 5:26 PM
kdevplatform/shell/projectcontroller.cpp
266

This patch also changed the logic of openProjectConfig(). Which seems unrelated to what the commit message talks about.

Sorry, I have no time for kdevelop these days to do proper reviews. but this here seems not discussed, so please give a reasoning or revert this specific change.

This patch also changed the logic of openProjectConfig().

No it doesn't?!

That function only opened the project config dialog if a single project was selected, and that's still the case. The change to using a QSet was requested and discussed. The fact that those containers are unordered is moot when there's only a single element.

I have no time for kdevelop these days to do proper reviews.

Then please only intervene (in accepted and applied changes) in case of blatant errors...?
FI: I submitted this change a bit over TEN months ago.

"Hi Friedirch, thanks for your pointer and the question. Though your quick post-commit read missed the fact that the new implementation of openProjectConfig(), while looking shorter, now reuses the new code introduced for the actual purpose of the commit. I am sorry that I missed to properly document this in the commit message that I not only added a new action, but also rewrote as side effect some existing otherwise unrelated code for the purpose of code reuse. I also understand that reasons for code changes should not be buried in long discussions on review boards, but be with the final code result, as no future code reader has time to find and rescan complete review discussions."