Changeset View
Standalone View
kdevplatform/shell/projectcontroller.cpp
| Show First 20 Lines • Show All 257 Lines • ▼ Show 20 Line(s) | 257 | if (itemContext) { | |||
|---|---|---|---|---|---|
| 258 | itemCount = itemContext->items().count(); | 258 | itemCount = itemContext->items().count(); | ||
| 259 | } | 259 | } | ||
| 260 | } | 260 | } | ||
| 261 | 261 | | |||
| 262 | m_openConfig->setEnabled(itemCount == 1); | 262 | m_openConfig->setEnabled(itemCount == 1); | ||
| 263 | m_closeProject->setEnabled(itemCount > 0); | 263 | m_closeProject->setEnabled(itemCount > 0); | ||
| 264 | } | 264 | } | ||
| 265 | 265 | | |||
| 266 | void openProjectConfig() | 266 | QSet<IProject*> selectedProjects() | ||
kossebau: This patch also changed the logic of openProjectConfig(). Which seems unrelated to what the… | |||||
mwolff: QSet | |||||
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? rjvbb: Am I missing a way to get direct access to a specific element in a QSet that doesn't use an… | |||||
what do you mean by direct access to a specific element? by index? or do you want to get the first or last item? mwolff: what do you mean by direct access to a specific element? by index? or do you want to get the… | |||||
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. rjvbb: Well, the routine always checked if only a single project was selected because opening multiple… | |||||
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. mwolff: deref the iterator returned by `QSet::begin()`
your QList won't work when you have multiple… | |||||
I'm dereferencing the QSet::constBegin() iterator, is that wrong (and if so, why is it allowed?) rjvbb: I'm dereferencing the QSet::constBegin() iterator, is that wrong (and if so, why is it allowed?) | |||||
| 267 | { | | |||
| 268 | // if only one project loaded, this is our target | | |||
| 269 | IProject *project = (m_projects.count() == 1) ? m_projects.at(0) : nullptr; | | |||
| 270 | | ||||
| 271 | // otherwise base on selection | | |||
| 272 | if (!project) { | | |||
| 273 | ProjectItemContext* ctx = dynamic_cast<ProjectItemContext*>(ICore::self()->selectionController()->currentSelection()); | | |||
| 274 | if (ctx && ctx->items().count() == 1) { | | |||
| 275 | project = ctx->items().at(0)->project(); | | |||
| 276 | } | | |||
| 277 | } | | |||
| 278 | | ||||
| 279 | if (project) { | | |||
| 280 | q->configureProject(project); | | |||
| 281 | } | | |||
| 282 | } | | |||
| 283 | | ||||
| 284 | void closeSelectedProjects() | | |||
| 285 | { | 267 | { | ||
| 286 | QSet<IProject*> projects; | 268 | QSet<IProject*> projects; | ||
| 287 | 269 | | |||
| 288 | // if only one project loaded, this is our target | 270 | // if only one project loaded, this is our target | ||
| 289 | if (m_projects.count() == 1) { | 271 | if (m_projects.count() == 1) { | ||
| 290 | projects.insert(m_projects.at(0)); | 272 | projects.insert(m_projects.at(0)); | ||
| 291 | } else { | 273 | } else { | ||
| 292 | // otherwise base on selection | 274 | // otherwise base on selection | ||
| 293 | ProjectItemContext* ctx = dynamic_cast<ProjectItemContext*>(ICore::self()->selectionController()->currentSelection()); | 275 | ProjectItemContext* ctx = dynamic_cast<ProjectItemContext*>(ICore::self()->selectionController()->currentSelection()); | ||
mwolff: double whitespace | |||||
| 294 | if (ctx) { | 276 | if (ctx) { | ||
| 295 | foreach (ProjectBaseItem* item, ctx->items()) { | 277 | foreach (ProjectBaseItem* item, ctx->items()) { | ||
| 296 | projects.insert(item->project()); | 278 | projects.insert(item->project()); | ||
| 297 | } | 279 | } | ||
| 298 | } | 280 | } | ||
| 299 | } | 281 | } | ||
| 282 | return projects; | ||||
| 283 | } | ||||
| 284 | | ||||
| 285 | void openProjectConfig() | ||||
| 286 | { | ||||
| 287 | auto projects = selectedProjects(); | ||||
| 288 | | ||||
| 289 | if (projects.count() == 1) { | ||||
| 290 | q->configureProject(*projects.constBegin()); | ||||
| 291 | } | ||||
| 292 | } | ||||
| 300 | 293 | | |||
| 301 | foreach (IProject* project, projects) { | 294 | void closeSelectedProjects() | ||
| 295 | { | ||||
| 296 | foreach (IProject* project, selectedProjects()) { | ||||
| 302 | q->closeProject(project); | 297 | q->closeProject(project); | ||
| 303 | } | 298 | } | ||
| 304 | } | 299 | } | ||
| 305 | 300 | | |||
| 306 | void importProject(const QUrl& url_) | 301 | void importProject(const QUrl& url_) | ||
| 307 | { | 302 | { | ||
| 308 | QUrl url(url_); | 303 | QUrl url(url_); | ||
| 309 | if (url.isLocalFile()) { | 304 | if (url.isLocalFile()) { | ||
| ▲ Show 20 Lines • Show All 865 Lines • ▼ Show 20 Line(s) | 1169 | } else { | |||
| 1175 | return prefixText + url.fileName(); | 1170 | return prefixText + url.fileName(); | ||
| 1176 | } | 1171 | } | ||
| 1177 | } | 1172 | } | ||
| 1178 | 1173 | | |||
| 1179 | ContextMenuExtension ProjectController::contextMenuExtension(Context* ctx, QWidget* parent) | 1174 | ContextMenuExtension ProjectController::contextMenuExtension(Context* ctx, QWidget* parent) | ||
| 1180 | { | 1175 | { | ||
| 1181 | Q_UNUSED(parent); | 1176 | Q_UNUSED(parent); | ||
| 1182 | ContextMenuExtension ext; | 1177 | ContextMenuExtension ext; | ||
| 1183 | if ( ctx->type() != Context::ProjectItemContext || !static_cast<ProjectItemContext*>(ctx)->items().isEmpty() ) { | 1178 | if ( ctx->type() != Context::ProjectItemContext) { | ||
| 1179 | return ext; | ||||
| 1180 | } | ||||
| 1181 | if (!static_cast<ProjectItemContext*>(ctx)->items().isEmpty() ) { | ||||
| 1182 | | ||||
| 1183 | QAction *action = new QAction(this); | ||||
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. mwolff: style:
```
auto* action = new QAction(tr("Reparse the Entire Project"), this);
...
```
And is… | |||||
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. kossebau: Relevant HIG is here: https://hig.kde.org/style/writing/capitalization.html#title… | |||||
| 1184 | connect(action, &QAction::triggered, this, [&] { | ||||
| 1185 | foreach (auto project, d->selectedProjects()) { | ||||
| 1186 | // can't use reparseProject() here because we need the forceAll argument | ||||
mwolff: you could add the forceAll arg there too, no? | |||||
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: Yes, but then I also have to add it to iProjectController::reparseProject(), no? Are there any… | |||||
mwolff: sure, but that's fine - we can break API/ABI in master as much as we want | |||||
Is there any other reason why this could not go into the current release branch? rjvbb: Is there any other reason why this could not go into the current release branch? | |||||
mwolff: it's not a bug fix, and you add new strings too | |||||
rjvbb: Well, outside of a freeze-before-a-release period of course... | |||||
| 1187 | if (auto job = d->m_parseJobs.value(project)) { | ||||
| 1188 | job->kill(); | ||||
| 1189 | } | ||||
| 1190 | d->m_parseJobs[project] = new KDevelop::ParseProjectJob(project, true, true); | ||||
mwolff: use `ProjectController::reparseProject` | |||||
| 1191 | ICore::self()->runController()->registerJob(d->m_parseJobs[project]); | ||||
| 1192 | } | ||||
| 1193 | }); | ||||
| 1194 | | ||||
| 1195 | action->setText( i18n( "Reparse the Entire Project" ) ); | ||||
| 1196 | ext.addAction(ContextMenuExtension::ProjectGroup, action); | ||||
| 1184 | return ext; | 1197 | return ext; | ||
| 1185 | } | 1198 | } | ||
| 1199 | | ||||
| 1186 | ext.addAction(ContextMenuExtension::ProjectGroup, d->m_openProject); | 1200 | ext.addAction(ContextMenuExtension::ProjectGroup, d->m_openProject); | ||
| 1187 | ext.addAction(ContextMenuExtension::ProjectGroup, d->m_fetchProject); | 1201 | ext.addAction(ContextMenuExtension::ProjectGroup, d->m_fetchProject); | ||
| 1188 | ext.addAction(ContextMenuExtension::ProjectGroup, d->m_recentProjectsAction); | 1202 | ext.addAction(ContextMenuExtension::ProjectGroup, d->m_recentProjectsAction); | ||
| 1203 | | ||||
| 1189 | return ext; | 1204 | return ext; | ||
| 1190 | } | 1205 | } | ||
| 1191 | 1206 | | |||
| 1192 | ProjectBuildSetModel* ProjectController::buildSetModel() | 1207 | ProjectBuildSetModel* ProjectController::buildSetModel() | ||
| 1193 | { | 1208 | { | ||
| 1194 | return d->buildset; | 1209 | return d->buildset; | ||
| 1195 | } | 1210 | } | ||
| 1196 | 1211 | | |||
| ▲ Show 20 Lines • Show All 89 Lines • ▼ Show 20 Line(s) | |||||
| 1286 | } | 1301 | } | ||
| 1287 | 1302 | | |||
| 1288 | void ProjectController::reparseProject( IProject* project, bool forceUpdate ) | 1303 | void ProjectController::reparseProject( IProject* project, bool forceUpdate ) | ||
| 1289 | { | 1304 | { | ||
| 1290 | if (auto job = d->m_parseJobs.value(project)) { | 1305 | if (auto job = d->m_parseJobs.value(project)) { | ||
| 1291 | job->kill(); | 1306 | job->kill(); | ||
| 1292 | } | 1307 | } | ||
| 1293 | 1308 | | |||
| 1294 | d->m_parseJobs[project] = new KDevelop::ParseProjectJob(project, forceUpdate); | 1309 | d->m_parseJobs[project] = new KDevelop::ParseProjectJob(project, forceUpdate); | ||
mwolff: expand this here | |||||
rjvbb: did you mean anything more than just passing the forceAll argument? | |||||
| 1295 | ICore::self()->runController()->registerJob(d->m_parseJobs[project]); | 1310 | ICore::self()->runController()->registerJob(d->m_parseJobs[project]); | ||
| 1296 | } | 1311 | } | ||
| 1297 | 1312 | | |||
| 1298 | } | 1313 | } | ||
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.