Differential D1099

Populate global CMake executable setting
ClosedPublic
Actions

Authored by obogdan on Mar 10 2016, 12:04 AM.

Details

Reviewers
apol
Group Reviewers
KDevelop
Summary

Add a default setting with the path to the cmake binary.

Test Plan

Compiled and runned.

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
obogdan updated this revision to Diff 2652.Mar 10 2016, 12:04 AM
obogdan retitled this revision from to Populate global CMake executable setting.
obogdan updated this object.
obogdan edited the test plan for this revision. (Show Details)
obogdan added a reviewer: KDevelop.
obogdan set the repository for this revision to R32 KDevelop.
obogdan added a project: KDevelop.
obogdan added a subscriber: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 10 2016, 12:04 AM
arrowd added a subscriber: arrowd.Mar 10 2016, 7:30 AM

These settings are currently not used at all by the CMakeManager due to this: https://phabricator.kde.org/T1315

projectbuilders/cmakebuilder/cmakebuilderconfig.kcfg
10–11

There is CMake::findExecutable() in cmakeutils.h that takes in account Windows standard installation directories.

11

How about mentioning that it is default CMake executable? Because user can also select executable when importing new project.

obogdan updated this revision to Diff 2705.Mar 11 2016, 12:46 PM
obogdan marked an inline comment as done.

<whatsthis> doesn't generate a tooltip. Added a tooltip in the UI.

obogdan added inline comments.Mar 11 2016, 12:48 PM
projectbuilders/cmakebuilder/cmakebuilderconfig.kcfg
10–11 ↗(On Diff #2705)

QStandardPaths::findExecutable() does the same. Is CMake::findExecutable() better in some way?

obogdan added a comment.Mar 11 2016, 12:49 PM
In D1099#20831, @arrowdodger wrote:

These settings are currently not used at all by the CMakeManager due to this: https://phabricator.kde.org/T1315

I consider this to be a step towards solving that.

apol added a subscriber: apol.Mar 11 2016, 1:18 PM

Other than that, looks good. Thanks!

projectbuilders/cmakebuilder/cmakebuilderconfig.kcfg
10–11 ↗(On Diff #2705)

+1 about using CMake::findExecutable().
@obogdan: see the code.

projectbuilders/cmakebuilder/cmakebuilderconfig.kcfgc
5 ↗(On Diff #2705)

This is probably not needed anymore, is it?

obogdan updated this revision to Diff 2707.Mar 11 2016, 2:32 PM

Replaced QStandardPaths search with CMake search.
Fixed tooltip string duplication.

obogdan marked 3 inline comments as done.Mar 11 2016, 2:33 PM
obogdan added inline comments.
projectbuilders/cmakebuilder/cmakebuilderconfig.kcfgc
5

Yes, it's needed to give access to the tooltips string.

obogdan marked 2 inline comments as done.Mar 11 2016, 2:33 PM
apol added a comment.Mar 11 2016, 2:36 PM

Looks good.

Can you make sure the "What's this" it gets translated?

obogdan added a comment.Mar 11 2016, 7:29 PM
In D1099#21229, @apol wrote:

Looks good.

Can you make sure the "What's this" it gets translated?

Generated code is itemCmakeExe->setWhatsThis( QCoreApplication::translate("CMakeBuilderSettings", "The path to the default CMake executable.") ); so I'd say it's being translated.

apol accepted this revision.Mar 11 2016, 7:56 PM
apol added a reviewer: apol.
This revision is now accepted and ready to land.Mar 11 2016, 7:56 PM
obogdan closed this revision.Mar 12 2016, 10:39 AM
kfunk added a subscriber: kfunk.Mar 12 2016, 5:25 PM

@apol: This should probably go to 5.0?

apol added a comment.Mar 12 2016, 5:43 PM

Cherry-picked to 5.0

Revision Contents
Changeset List

Diff 2707

View Options

projectbuilders/cmakebuilder/cmakebuilderconfig.kcfg

Loading...
View Options

projectbuilders/cmakebuilder/cmakebuilderconfig.kcfgc

Loading...
View Options

projectbuilders/cmakebuilder/cmakebuilderpreferences.cpp

Loading...
View Options

projectbuilders/cmakebuilder/cmakebuilderpreferences.ui

Loading...