Changeset View
Standalone View
kdevplatform/shell/openprojectdialog.cpp
| Show All 9 Lines | |||||
| 10 | 10 | | |||
| 11 | #include "openprojectdialog.h" | 11 | #include "openprojectdialog.h" | ||
| 12 | #include "openprojectpage.h" | 12 | #include "openprojectpage.h" | ||
| 13 | #include "projectinfopage.h" | 13 | #include "projectinfopage.h" | ||
| 14 | 14 | | |||
| 15 | #include <QPushButton> | 15 | #include <QPushButton> | ||
| 16 | #include <QFileInfo> | 16 | #include <QFileInfo> | ||
| 17 | #include <QFileDialog> | 17 | #include <QFileDialog> | ||
| 18 | #include <QRegularExpression> | ||||
| 18 | 19 | | |||
| 19 | #include <KColorScheme> | 20 | #include <KColorScheme> | ||
| 20 | #include <KIO/StatJob> | 21 | #include <KIO/StatJob> | ||
| 21 | #include <KIO/ListJob> | 22 | #include <KIO/ListJob> | ||
| 22 | #include <KJobWidgets> | 23 | #include <KJobWidgets> | ||
| 23 | #include <KLocalizedString> | 24 | #include <KLocalizedString> | ||
| 24 | 25 | | |||
| 25 | #include "core.h" | 26 | #include "core.h" | ||
| ▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Line(s) | |||||
| 68 | 69 | | |||
| 69 | namespace KDevelop | 70 | namespace KDevelop | ||
| 70 | { | 71 | { | ||
| 71 | 72 | | |||
| 72 | OpenProjectDialog::OpenProjectDialog(bool fetch, const QUrl& startUrl, | 73 | OpenProjectDialog::OpenProjectDialog(bool fetch, const QUrl& startUrl, | ||
| 73 | const QUrl& repoUrl, IPlugin* vcsOrProviderPlugin, | 74 | const QUrl& repoUrl, IPlugin* vcsOrProviderPlugin, | ||
| 74 | QWidget* parent) | 75 | QWidget* parent) | ||
| 75 | : KAssistantDialog( parent ) | 76 | : KAssistantDialog( parent ) | ||
| 76 | , m_urlIsDirectory(false) | 77 | , m_urlIsDirectory(false) | ||
kossebau: This will create a temporary QUrl object, which then gets passed as argument to the copy… | |||||
| 77 | , sourcePage(nullptr) | 78 | , sourcePage(nullptr) | ||
| 78 | , openPage(nullptr) | 79 | , openPage(nullptr) | ||
| 79 | , projectInfoPage(nullptr) | 80 | , projectInfoPage(nullptr) | ||
| 80 | { | 81 | { | ||
| 81 | resize(QSize(700, 500)); | 82 | resize(QSize(700, 500)); | ||
| 82 | 83 | | |||
| 83 | // KAssistantDialog creates a help button by default, no option to prevent that | 84 | // KAssistantDialog creates a help button by default, no option to prevent that | ||
| 84 | auto helpButton = button(QDialogButtonBox::Help); | 85 | auto helpButton = button(QDialogButtonBox::Help); | ||
| 85 | if (helpButton) { | 86 | if (helpButton) { | ||
| 86 | buttonBox()->removeButton(helpButton); | 87 | buttonBox()->removeButton(helpButton); | ||
| 87 | delete helpButton; | 88 | delete helpButton; | ||
| 88 | } | 89 | } | ||
| 89 | 90 | | |||
| 90 | const bool useKdeFileDialog = qEnvironmentVariableIsSet("KDE_FULL_SESSION"); | 91 | const bool useKdeFileDialog = qEnvironmentVariableIsSet("KDE_FULL_SESSION"); | ||
Apologies, please disregard this unrelated hunk; evidently it's not to be part of this mod. rjvbb: Apologies, please disregard this unrelated hunk; evidently it's not to be part of this mod. | |||||
| 91 | QStringList filters, allEntry; | 92 | QStringList filters, allEntry; | ||
| 92 | QString filterFormat = useKdeFileDialog | 93 | QString filterFormat = useKdeFileDialog | ||
| 93 | ? QStringLiteral("%1|%2 (%1)") | 94 | ? QStringLiteral("%1|%2 (%1)") | ||
| 94 | : QStringLiteral("%2 (%1)"); | 95 | : QStringLiteral("%2 (%1)"); | ||
| 95 | allEntry << QLatin1String("*.") + ShellExtension::getInstance()->projectFileExtension(); | 96 | allEntry << QLatin1String("*.") + ShellExtension::getInstance()->projectFileExtension(); | ||
| 96 | filters << filterFormat.arg(QLatin1String("*.") + ShellExtension::getInstance()->projectFileExtension(), ShellExtension::getInstance()->projectFileDescription()); | 97 | filters << filterFormat.arg(QLatin1String("*.") + ShellExtension::getInstance()->projectFileExtension(), ShellExtension::getInstance()->projectFileDescription()); | ||
| 97 | const QVector<KPluginMetaData> plugins = ICore::self()->pluginController()->queryExtensionPlugins(QStringLiteral("org.kdevelop.IProjectFileManager")); | 98 | const QVector<KPluginMetaData> plugins = ICore::self()->pluginController()->queryExtensionPlugins(QStringLiteral("org.kdevelop.IProjectFileManager")); | ||
| 98 | for (const KPluginMetaData& info : plugins) { | 99 | for (const KPluginMetaData& info : plugins) { | ||
| ▲ Show 20 Lines • Show All 168 Lines • ▼ Show 20 Line(s) | 267 | // add managers that work in any case (e.g. KDevGenericManager) | |||
| 267 | choices.reserve(choices.size() + m_genericProjectPlugins.size()); | 268 | choices.reserve(choices.size() + m_genericProjectPlugins.size()); | ||
| 268 | Q_FOREACH ( const auto& plugin, m_genericProjectPlugins ) { | 269 | Q_FOREACH ( const auto& plugin, m_genericProjectPlugins ) { | ||
| 269 | qCDebug(SHELL) << plugin; | 270 | qCDebug(SHELL) << plugin; | ||
| 270 | auto meta = m_projectPlugins.value(plugin); | 271 | auto meta = m_projectPlugins.value(plugin); | ||
| 271 | choices.append({plugin, meta.pluginId(), meta.iconName()}); | 272 | choices.append({plugin, meta.pluginId(), meta.iconName()}); | ||
| 272 | } | 273 | } | ||
| 273 | page->populateProjectFileCombo(choices); | 274 | page->populateProjectFileCombo(choices); | ||
| 274 | } | 275 | } | ||
| 276 | // Turn m_url into the full path to the project filename (default: /path/to/projectSrc/projectSrc.kdev4). | ||||
| 277 | // This is done only when m_url doesn't already point to such a file, typically because the user selected | ||||
| 278 | // one in the project open dialog. Omitting this check could lead to project files of the form | ||||
| 279 | // /path/to/projectSrc/SourceProject.kdev4/projectSrc.kdev4 . | ||||
| 280 | if (!m_url.toLocalFile().endsWith(QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension())) { | ||||
| 275 | m_url.setPath( m_url.path() + QLatin1Char('/') + m_url.fileName() + QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension() ); | 281 | m_url.setPath( m_url.path() + QLatin1Char('/') + m_url.fileName() + QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension() ); | ||
| 282 | } | ||||
When can this situation happen? After all m_url is handled above with if( !urlInfo.isDir ) {
m_url = m_url.adjusted(QUrl::StripTrailingSlash | QUrl::RemoveFilename);
}Do people have directories using the projectFileExtension in the dir name? Or would they select the hidden directories with the personal kdevelop project data? Why should the global project filename not be set in this case? kossebau: When can this situation happen? After all `m_url` is handled above with
```
if( ! | |||||
As a matter of fact I cannot find (really) related discussion. I hope the comment I'm adding makes clear enough what the code does and why. rjvbb: As a matter of fact I cannot find (really) related discussion. I hope the comment I'm adding… | |||||
Re: related discussion, I might have misunderstood some of the comments in this review,, could not find it back on a re-read. Thanks for adding the comment to the code, but I am still wondering by the code how this situation can happen. If a user selected an existing file with that extension and thus m_url is "/path/to/projectSrc/projectSrc.kdev", the very if( !urlInfo.isDir ) {
m_url = m_url.adjusted(QUrl::StripTrailingSlash | QUrl::RemoveFilename);
}earlier should handle this, no? Or does this not do what is expected (in some Qt version)? Or csan the incoming path have a trailing slash already which spoils dropping the last path element by QUrl::RemoveFilename? if( urlInfo.isDir || urlInfo.extension != ShellExtension::getInstance()->projectFileExtension() ) so we only can arrive here with a "/path/to/projectSrc/projectSrc.kdev" if there is some existing directory selected which has a suffix ".kdev4". KDevelop itself does not generate such directories, or? kossebau: Re: related discussion, I might have misunderstood some of the comments in this review,, could… | |||||
| 276 | } else { | 283 | } else { | ||
| 277 | setAppropriate( projectInfoPage, false ); | 284 | setAppropriate( projectInfoPage, false ); | ||
| 278 | m_url = url; | 285 | m_url = url; | ||
| 279 | m_urlIsDirectory = false; | 286 | m_urlIsDirectory = false; | ||
| 280 | } | 287 | } | ||
| 281 | validateProjectInfo(); | 288 | validateProjectInfo(); | ||
| 282 | setValid( openPage, true ); | 289 | setValid( openPage, true ); | ||
| 283 | } | 290 | } | ||
| Show All 19 Lines | |||||
| 303 | 310 | | |||
| 304 | void OpenProjectDialog::openPageAccepted() | 311 | void OpenProjectDialog::openPageAccepted() | ||
| 305 | { | 312 | { | ||
| 306 | if ( isValid( openPage ) ) { | 313 | if ( isValid( openPage ) ) { | ||
| 307 | next(); | 314 | next(); | ||
| 308 | } | 315 | } | ||
| 309 | } | 316 | } | ||
| 310 | 317 | | |||
| 311 | void OpenProjectDialog::validateProjectName( const QString& name ) | 318 | void OpenProjectDialog::validateProjectName( const QString& name ) | ||
The name validateProjectName( promises the project name is validated. kossebau: The name `validateProjectName(` promises the project name is validated.
But the new code in… | |||||
the method originally *set* the project name, and then validated the project info - the name was thus not very appropriate already. rjvbb: the method originally *set* the project name, and then validated the project info - the name… | |||||
| 312 | { | 319 | { | ||
| 320 | if (name != m_projectName) { | ||||
| 313 | m_projectName = name; | 321 | m_projectName = name; | ||
To me without context it is totally unclear why certain things (which actually in high level perspective?) are only done for that page, but not any other page? Perhaps this method should not be called otherwise? Or split into two separate methods, one to be called for the projectpage, the other for any other pages? In any case, given settingName can be misunderstood on first read, let's rename to something less ambiguous, like isDefiningProjectName. Personally also would favour adding brackets around the comparison, to speed up some human readers. So in total: const bool isDefiningProjectName = (currentPage() == projectInfoPage); kossebau: To me without context it is totally unclear why certain things (which actually in high level… | |||||
Normally I'd have used brackets, I think I omitted them here to avoid requests to reduce redundancy... I mostly agree that the underlying design is unclear here; I had not expected at all that this method could be called before the dialog for entering a project name was posted. However splitting it up may not be straightforward (it's used only as a slot connected to a single signal) and would probably lead to some code duplication. I did simplify the function a bit: I had overlooked that there's no reason to calculate the local url and safeName variables when !settingName (now isEnteringProjectName). rjvbb: Normally I'd have used brackets, I think I omitted them here to avoid requests to reduce… | |||||
| 322 | bool isEnteringProjectName = (currentPage() == projectInfoPage); | ||||
| 323 | // don't interfere with m_url when validateOpenUrl() is also likely to change it | ||||
| 324 | if (isEnteringProjectName) { | ||||
| 325 | if (m_projectDirUrl.isEmpty()) { | ||||
mwolff: use QRegularExpression, wrap pattern in QStringLiteral | |||||
| 326 | // cache the selected project directory | ||||
mwolff: use QLatin1Char | |||||
| 327 | const auto urlInfo = ::urlInfo(m_url); | ||||
| 328 | if (urlInfo.isValid && urlInfo.isDir) { | ||||
| 329 | m_projectDirUrl = m_url.adjusted(QUrl::StripTrailingSlash); | ||||
| 330 | } else { | ||||
| 331 | // if !urlInfo.isValid the url almost certainly refers to a file that doesn't exist (yet) | ||||
| 332 | // With the Generic Makefile proj.manager it can be the project file url, for instance. | ||||
| 333 | m_projectDirUrl = m_url.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash); | ||||
| 334 | } | ||||
| 335 | } | ||||
What is the definition of "safe"? Where can be conflicts? This is totally unclear to a reader of the actual code and code comments. Could this perhaps be factored out into some util class, which then also could get proper unit testing? kossebau: What is the definition of "safe"? Where can be conflicts? This is totally unclear to a reader… | |||||
I supposed KDevelop::Path could have a method that creates a "safe" instance from an input QString/QUrl but that'd be a separate change. rjvbb: I supposed `KDevelop::Path` could have a method that creates a "safe" instance from an input… | |||||
| 336 | | ||||
| 337 | const QUrl url(m_projectDirUrl); | ||||
| 338 | // construct a version of the project name that is safe for use as a filename, i.e. | ||||
| 339 | // a version that does not contain characters that are illegal or are best avoided: | ||||
| 340 | // replace square braces and dir separator-like characters with a '@' placeholder | ||||
No need to assign to safeName, QString::replace(...) operates on same object and just returns reference for call chains (see also usage line above). kossebau: No need to assign to `safeName`, `QString::replace(...)` operates on same object and just… | |||||
Done, but in this case I found the additional assign increased readability (= I'm not convinced by any of the line folding approaches I tried). rjvbb: Done, but in this case I found the additional assign increased readability (= I'm not convinced… | |||||
| 341 | // replace colons with '=' and whitespace with underscores. | ||||
| 342 | QString safeName = m_projectName; | ||||
| 343 | safeName.replace(QRegularExpression(QStringLiteral("[\\\\/]")), QStringLiteral("@")); | ||||
| 344 | safeName = safeName.replace(QLatin1Char(':'), QLatin1Char('=')) \ | ||||
| 345 | .replace(QRegExp(QStringLiteral("\\s")), QStringLiteral("_")); | ||||
| 346 | safeName += QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension(); | ||||
| 347 | | ||||
| 348 | m_url.setPath(url.path() + QLatin1Char('/') + safeName); | ||||
| 349 | m_urlIsDirectory = false; | ||||
| 350 | qCDebug(SHELL) << "project name:" << m_projectName << "file name:" << safeName << "in" << url.path(); | ||||
| 351 | } | ||||
| 352 | } | ||||
| 314 | validateProjectInfo(); | 353 | validateProjectInfo(); | ||
| 315 | } | 354 | } | ||
| 316 | 355 | | |||
| 317 | void OpenProjectDialog::validateProjectInfo() | 356 | void OpenProjectDialog::validateProjectInfo() | ||
| 318 | { | 357 | { | ||
| 319 | setValid( projectInfoPage, (!projectName().isEmpty() && !projectManager().isEmpty()) ); | 358 | setValid( projectInfoPage, (!projectName().isEmpty() && !projectManager().isEmpty()) ); | ||
| 320 | } | 359 | } | ||
| 321 | 360 | | |||
| ▲ Show 20 Lines • Show All 46 Lines • Show Last 20 Lines | |||||
This will create a temporary QUrl object, which then gets passed as argument to the copy constructor of the QUrl of m_projectDirUrl, no? Not wanted here, or?
Just remove the line and leave default constructor as applied by compiler do its job.