D9344: [KDevelop] : consistent use of the project name (WIP!)

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a project: KDevelop.

REVISION SUMMARY
This is a WIP aiming to make KDevelop use the user-specified project name consistently and throughout.

Currently, that name is used only in a few places, the projects toolview and in the titlebar. In all other places where projects are listed by *a* name, the project file name is used which isn't always enough. QMake-based projects in particular will often show up with an ambiguous name like "src" (qtbase, qttools, qtwhatever components imported as projects; their toplevel .pro files don't support that).

Ideally, the user-selected project name should also be used

- to name the .kdev4 project files
- in the output of kdevelop -l and kdevelop --ps

This will remove any ambiguity that could arise from seeing projects listed by their directory name. It will also make it possible to have multiple projects defined for a single source tree (e.g. for separate components in different subfolders, for building mutually-exclusive Qt4, Qt5 or GTk variants, etc.)

It is currently already possible to achieve this goal by renaming and editing the generated .kdev4 files (after closing/unloading the corresponding project).

---

The current patch is just the first step: generating the project file name with minimal code/API changes. It can be used "as is" but then the "override or use existing file?" dialog will pop up when the project is closed and reloaded.

BUG: 384955

TEST PLAN
- figure out how to integrate the new naming with the code that puts up the "override/use existing" dialog (`equalProjectFile()` called by `ProjectDialogProvider::askProjectConfigLocation()`)
- nothing should change if the user selects the predetermined project name; verify if this is indeed the case (esp. for the QMake proj. manager)
- check what happens with projects that have different project filename and name (e.g. `src.kdev4` for a project called `qtbase`).

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp

To: rjvbb
Cc: kdevelop-devel, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

openprojectdialog.cpp:90
- const bool useKdeFileDialog = qEnvironmentVariableIsSet("KDE_FULL_SESSION");
QStringList filters, allEntry;

Apologies, please disregard this unrelated hunk; evidently it's not to be part of this mod.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb
Cc: kdevelop-devel, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


In `ProjectDialogProvider::askProjectConfigLocation()`:

qCWarning(SHELL) << "selected project:" << projectFileUrl << "selectedUrl=" << dlg->selectedUrl()
<< "projectName=" << dlg->projectName() << "projectManager=" << dlg->projectManager();

and in `equalProjectFile()`:

qWarning() << Q_FUNC_INFO << "configPath=" << configPath << "defaultName=" << defaultName
<< "cfg.Name=" << grp.readEntry( "Name", QString() ) << "projectName=" << dlg->projectName();

gives the following observations:

1: Opening an existing project of which I renamed the .kdev4 files manually in the past, project dir `kdevelop-git-5`:

kdevplatform.shell: selected project: QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kdevelop-git-5/kf5-kdevelop-5.kdev4")
selectedUrl= QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kdevelop-git-5/kf5-kdevelop-5.kdev4")
projectName= "kdevelop-git-5" projectManager= "<built-in>"

2: Creating new project, closing it, reopening it (project dir `kcompl-git`, no shouldAsk dialog):

kdevplatform.shell: selected project: QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/kcompl-git.kdev4")
selectedUrl= QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/kcompl-git.kdev4")
projectName= "kcompl-git" projectManager= "KDevCMakeManager"
bool KDevelop::equalProjectFile(const QString &, KDevelop::OpenProjectDialog *) configPath= "/home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/kcompl-git.kdev4"
defaultName= "kcompl-git" cfg.Name= "kcompl-git" projectName= "kcompl-git"

3: After renaming it to 5:kcompl-git, on-disk and inside 5:kcompl-git.kdev4 (NO shouldAsk dialog posted):

kdevplatform.shell: selected project: QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/5:kcompl-git.kdev4")
selectedUrl= QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/5:kcompl-git.kdev4")
projectName= "kcompl-git" projectManager= "<built-in>"

4: Creating as 5:kcompl-git.kdev4 from scratch (= lines 322,3 in hunk 2 above NOT commented-out):

kdevplatform.shell: selected project: QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/5:kcompl-git.kdev4")
selectedUrl= QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/CMakeLists.txt")
projectName= "5:kcompl-git" projectManager= "KDevCMakeManager"

5: reloading 5:kcompl-git after closing (a shouldAsk dialog IS posted):

kdevplatform.shell: selected project: QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/5:kcompl-git.kdev4")
selectedUrl= QUrl("file:///home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/5:kcompl-git.kdev4")
projectName= "kcompl-git" projectManager= "KDevCustomBuildSystem"
bool KDevelop::equalProjectFile(const QString &, KDevelop::OpenProjectDialog *) configPath= "/home/bertin/cworks/Scratch/KDE/KF5/kcompl-git/5:kcompl-git.kdev4"
defaultName= "kcompl-git" cfg.Name= "5:kcompl-git" projectName= "kcompl-git"

I can see the difference between 3 and 5 and why 5 would post a dialog, but don't see the difference between 1 and 5 (also why `equalProjectFile()` appears not to be called).

The real question here might be why an override dialog should be posted at all when the user reopens a .kdev4 file. That makes sense if you select a (C)Makefile or .pro file but it probably is the wrong question to ask when the user opens an existing .kdev4 file. Especially when only a single such file exists. Wouldn't it be enough in that case to verify if .kdev4 filename and the Name property stored therein correspond?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb
Cc: kdevelop-devel, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb updated this revision to Diff 23973.
rjvbb added a comment.


This minimal(ist) implementation seems to do what I'm after, without introducing unwanted side-effects that I've been able to find. Existing projects import as they should, new projects are saved with a safe version of the name given by the user (which will still default to the same naming scheme).

In the end I think that there's no need even to check if a new configuration must be written when the user opens a .kdev4 file. I hope I'm not overlooking something obvious but will be testing this IRL.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=23952&id=23973

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/projectcontroller.cpp

To: rjvbb
Cc: kdevelop-devel, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb retitled this revision from "[KDevelop] : consistent use of the project name (WIP!)" to "[KDevelop] : consistent use of the project name (WIP)".
rjvbb edited the summary of this revision.
rjvbb edited the test plan for this revision.
rjvbb added a reviewer: KDevelop.
rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop
Cc: kdevelop-devel, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

openprojectdialog.cpp:312
+ url.setPath(url.path() + '/' + safeName + '.' + ShellExtension::getInstance()->projectFileExtension());

use QRegularExpression, wrap pattern in QStringLiteral

openprojectdialog.cpp:313
+ url.setPath(url.path() + '/' + safeName + '.' + ShellExtension::getInstance()->projectFileExtension());
+ m_urlIsDirectory = false;

use QLatin1Char

projectcontroller.cpp:443
// check whether config is equal
- bool shouldAsk = true;
- if( projectFileUrl == dlg->selectedUrl() )
+ bool isKDevProject = QFileInfo(projectFileUrl.url()).completeSuffix() == QStringLiteral("kdev4");
+ bool shouldAsk = !isKDevProject;

I don't get this change, can you explain? the old code checks whether the profileFileUrl (which should *always* ends on `.kdev4`, no?) exists. In that case, we want to ask the user if he wants to override, except if the project file is equal to what we'd write out anyways.

your change seems to completely break this, as isKDevProject should always be true, and then shouldAsk always false?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.

Post by Milian Wolff

projectcontroller.cpp:443

I don't get this change, can you explain? the old code checks whether the profileFileUrl (which should *always* ends on `.kdev4`, no?) exists. In that case, we want to ask the user if he wants to override, except if the project file is equal to what we'd write out anyways.

"Except", are you sure about that? I'm pretty certain I got the override dialog when I forced a new import from, say, the project's CMake file, and <dirname>.kdev4 existed already.

Post by Milian Wolff
your change seems to completely break this, as isKDevProject should always be true, and then shouldAsk always false?

Of course I've already half forgotten why I had to make this change, more than 3 months ago. Did you read the observations I posted in comments?

I think that this modifies the "we want to ask the user if he wants to override, except if the project file is equal to what we'd write out anyways" to include user-selected .kdev4 filename. Getting the project filename to be written under a name that reflects the actual project name was relatively easy, but I got the override dialog when reopening the project from that file afterwards. That shouldn't happen of course, according to me at least.

The purpose of this change is to give user control over the project.kdev4 file name, and one reason to do that is that it allows to create different projects in a single source tree, which can be opened concurrently. In that case you shouldn't be proposing an override in general unless you're 100% certain the user might indeed be overriding (overwriting) something.

There's only 1 case in that context where I can imagine that an override request would make sense: when you try to recreate a project from the CMake/QMake/Make file and give it the exact same name (override means overwrite). In all other cases you'd be creating a new .kdev4 file, and then there's no need for asking the user if he wants to override an existing project. The answer to that is implicitly no ("I just told you to create a project with a different name"). The import wizard will already auto-select an existing .kdev4 file in the 2nd step even when you started the procedure by selecting a CMake/QMake/Make file. So re-selecting that file instead of the .kdev4 file should be indication enough that you don't want to use an existing project definition, no? Except when you give that project the name of an existing project, so the opposite of what you wrote above. I suppose that assumption is why I've always felt an ambiguity in the situation around project files and names.

I've been using this modification for almost 4 months now, without ever running into unexpected behaviour. If you can think of a sequence of actions that will no longer produce the intended result I can test of course.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Milian Wolff]

mwolff added a comment.

Post by René J.V. Bertin

Post by Milian Wolff

projectcontroller.cpp:443

I don't get this change, can you explain? the old code checks whether the profileFileUrl (which should *always* ends on `.kdev4`, no?) exists. In that case, we want to ask the user if he wants to override, except if the project file is equal to what we'd write out anyways.

"Except", are you sure about that? I'm pretty certain I got the override dialog when I forced a new import from, say, the project's CMake file, and <dirname>.kdev4 existed already.

Ah, yes - that could be it. Then we should probably change the code to first construct a projectFileUrl that actually points to a .kdev4 file, and then use that in the conditional. And add comments that explain what is going on here.

Post by René J.V. Bertin

Post by Milian Wolff
your change seems to completely break this, as isKDevProject should always be true, and then shouldAsk always false?

Of course I've already half forgotten why I had to make this change, more than 3 months ago. Did you read the observations I posted in comments?

Yes, but it didn't answer my question. Your comment above does though, so make sure to add that to the code too to simplify the understanding from others.

Post by René J.V. Bertin
I think that this modifies the "we want to ask the user if he wants to override, except if the project file is equal to what we'd write out anyways" to include user-selected .kdev4 filename. Getting the project filename to be written under a name that reflects the actual project name was relatively easy, but I got the override dialog when reopening the project from that file afterwards. That shouldn't happen of course, according to me at least.

Neither according to me, which makes me wonder why the `equalProjectFile` code is apparently broken?

Post by René J.V. Bertin
The purpose of this change is to give user control over the project.kdev4 file name, and one reason to do that is that it allows to create different projects in a single source tree, which can be opened concurrently. In that case you shouldn't be proposing an override in general unless you're 100% certain the user might indeed be overriding (overwriting) something.

I agree in principle, I'm just trying to understand the old code and your changes.

Post by René J.V. Bertin
There's only 1 case in that context where I can imagine that an override request would make sense: when you try to recreate a project from the CMake/QMake/Make file and give it the exact same name (override means overwrite). In all other cases you'd be creating a new .kdev4 file, and then there's no need for asking the user if he wants to override an existing project. The answer to that is implicitly no ("I just told you to create a project with a different name"). The import wizard will already auto-select an existing .kdev4 file in the 2nd step even when you started the procedure by selecting a CMake/QMake/Make file. So re-selecting that file instead of the .kdev4 file should be indication enough that you don't want to use an existing project definition, no? Except when you give that project the name of an existing project, so the opposite of what you wrote above. I suppose that assumption is why I've always felt an ambiguity in the situation around project files and names.

I agree with what you write. I don't understand why you think what I wrote above is the opposite though? Simply put: Unless you open a `*.kdev4` file explicitly, we want to ensure that we don't overwrite another existing project file with *different* contents. That's what I think the existing code was trying to do.

Post by René J.V. Bertin
I've been using this modification for almost 4 months now, without ever running into unexpected behaviour. If you can think of a sequence of actions that will no longer produce the intended result I can test of course.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb marked 3 inline comments as done.
rjvbb added a comment.

Post by Milian Wolff

Post by René J.V. Bertin
"Except", are you sure about that? I'm pretty certain I got the override dialog when I forced a new import from, say, the project's CMake file, and <dirname>.kdev4 existed already.

Ah, yes - that could be it. Then we should probably change the code to first construct a projectFileUrl that actually points to a .kdev4 file, and then use that in the conditional. And add comments that explain what is going on here.

I don't think that is necessary; will add comments to the code trying to explain what goes and why.

Post by Milian Wolff
Neither according to me, which makes me wonder why the `equalProjectFile` code is apparently broken?

I don't think it is - I didn't see a need to change it after all :)
It was probably just used in a way that wasn't sufficiently well thought out and worked sufficiently well because the project controller would only create new project files that called <dirname>.kdev4 .

I do wonder now if `equalProjectFile()` is even necessary.

Either way, I just tested again; the changed code allows me to create and reopen multiple projects from the same CMake file as long as I give them different names during the initial import procedure. The project.kdev4 files reflect those names and can thus live in the same directory. I only get the override dialog if I import the CMake file and then specify a name that was already used for a project in that same directory.

Post by Milian Wolff
I agree with what you write. I don't understand why you think what I wrote above is the opposite though?

Because you said we should post the override dialog EXCEPT (= unless) when we're going to overwrite an existing project file. I think we should only do it WHEN we're going to overwrite.

BTW, here's another interesting observation: the default project file in a directory like kfoo-18.04.2 will be called kfoo-18.04.2.kdev4 . The check `QFileInfo(projectFileUrl.url()).completeSuffix()` is actually broken, because it will return "04.2.kdev4", I'll be fixing that too (the check should use dlg->selectedUrl() anyway).

R

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb updated this revision to Diff 31321.
rjvbb added a comment.


Updated as discussed.

I've added a debug trace in `equalProjectFile()` and a warning when that function is going to be called because I cannot think of any situation where that might happen with my changes in place. See the inline comment(s).

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=23973&id=31321

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Milian Wolff]

mwolff accepted this revision.
mwolff added a comment.
This revision is now accepted and ready to land.


ok, let's go for it then

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


That was quick! :)

What branch? I've been doing this in/for 5.2 but I'm fine with pushing it to master and letting someone else if/when to merge it into 5.2 .

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Kevin Funk]

kfunk added a comment.


Push to master, I'd say.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R32:a44554ef038a: Use the project name more consistently (authored by rjvbb).

REPOSITORY
R32 KDevelop

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=31321&id=31368

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Max Schwarz]

mschwarz added a comment.


Not sure if this is the right place to discuss, but this breaks the generic project manager for me.

Test: Open an empty folder using the generic project manager. If the folder name is "xyz", KDevelop will create a "xyz.kdev4" folder in the *parent* directory and use that as the project directory - which is pretty useless ;-)

I'm pretty sure this is due to `QUrl::RemoveFilename`, which removes the last directory if the URL refers to a directory.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


That's possible (your analysis I mean) and annoying that we missed it during the review. I'm away from my dev environment for a while, so cannot do much right now. IIRC most of the changes are around the location where it's also decided whether an override dialog has to be posted. If that's also where the last path component is removed, it should be possible to make that removal conditional on whether or not the selectedUrl points to a file or to a directory.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Milian Wolff]

mwolff added a comment.


Let's revert and then we can fix it later. This patch is imo less important than a working generic manager. I'll apply the revert now.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Milian Wolff]

mwolff added a comment.


Done, Rene - feel free to respin with the issue fixed.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.

Post by Max Schwarz
I'm pretty sure this is due to `QUrl::RemoveFilename`, which removes the last directory if the URL refers to a directory.

Looking into this now.

Your assumption is wrong though. The RemoveFilename bit is necessary to avoid growing m_url because of using itself in repeated setPath() calls. Turns out that OpenProjectDialog::m_url is already set to the parent directory when first called by the generic project manager so the glitch must be in the projectcontroller.cpp hunk.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb reopened this revision.
This revision is now accepted and ready to land.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb updated this revision to Diff 33322.
rjvbb added a comment.


Please double-check if the issue is indeed solved and no new regressions are introduced.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=31368&id=33322

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/openprojectdialog.h
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb requested review of this revision.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


Ping?

If no objections are made I'll interpret Milian's "feel free to respin" as "feel free to commit" in a couple of days.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb updated this revision to Diff 37281.
rjvbb added a comment.


This should now work as intended in all situations.

I had not realised that the project name validation function was also already getting called during the project selection stage, which led to corrupting `m_url` because both functions were modifying it recursively. Except when importing a cmake or qmake file from the project toplevel directory.

Please double-check.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=33322&id=37281

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/openprojectdialog.h
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb retitled this revision from "[KDevelop] : consistent use of the project name (WIP)" to "[KDevelop] : [fixed] consistent use of the project name".
rjvbb edited the test plan for this revision.
rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb updated this revision to Diff 42451.
rjvbb added a comment.


This fixes a mix-up that slipped through and above all, puts deleting the .kdev4 under user control.
Apparently there are situations where this directory should be deleted because its presence (empty) can confuse the project manager when overwriting an existing project (?!, T6262 <https://phabricator.kde.org/T6262>). /Methinks that's something that must be fixed in the confused place(s), not worked around by removing .kdev4. The whole point of this patch is to allow the creation of multiple projects in a single source tree, after all.

I have thus added an additional dialog asking if it's safe to delete the directory, when necessary (and until a proper fix is implemented).

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=37281&id=42451

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/openprojectdialog.h
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb retitled this revision from "[KDevelop] : [fixed] consistent use of the project name" to "[KDevelop] : [fixed] consistent use of the project name allowing to create multiple projects in a single source tree".
rjvbb edited the test plan for this revision.
rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Gleb Popov]

arrowd added a comment.


I don't quite get it. In which situation do we want to override the project (delete `.kdev` file), but do not clear `.kdev` dir?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


In any situation where we'd already be overwriting the settings file but where there are other projects defined in the same sourcetree. The summary of this DR argues how and why that could be the case.

In practice I rarely find myself overwriting an existing settings file now that I can (again) add a new project and the .kdev4 files are named after the project.

From your answer on T6262 <https://phabricator.kde.org/T6262> it seems that the presence of the .kdev4 directory was never the issue, but presence of an existing settings file. A proper solution would be to delete just the settings file, if overwriting is to be done. That would make my current workaround a lot easier: no more need for an extra dialog, nor (probably) to change the scope for the `shouldAsk` variable.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Gleb Popov]

arrowd added a comment.

Post by René J.V. Bertin
A proper solution would be to delete just the settings file, if overwriting is to be done.

With that I fully agree. I'll do it this weekend or you can do it if you wish.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


I'm doing it now and will test it as part of this patch; afterward there are 3 options

1. commit it directly
2. put up a dedicated review
3. keep it as part of this one - a priori this has been accepted once, was committed and then reverted because of an unforeseen issue I have since fixed.

I'm not 100% certain if I interpret Milian's revert message correctly that suggests I could recommit after fixing. He's not reacted to questions about that so one could assume that he has no objections against that.

It would be nice if this could go in for 5.3 of course.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 42472.
rjvbb added a comment.


Updated as discussed: any previous project settings file in .kdev4 will be removed if and before overriding/writing it (instead of simply trashing the entire .kdev4 directory).

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=42451&id=42472

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/openprojectdialog.h
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 42564.
rjvbb added a comment.


Another fix for the generic Makefile project manager; I had missed the fact that `URLInfo::isDir` is undefined when `URLInfo::isValid` is false. This lead to personalised project files of the type ```/path/to/projectFoo/projectFoo.kdev4/customname.kdev4```.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=42472&id=42564

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/openprojectdialog.h
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


In absence of instructions to the contrary I'll interpret Milian's "feel free to respin" as "feel free to recommit", and push a commit during the day. This has been waiting to be re-applied long enough.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Friedrich W. H. Kossebau]

kossebau added a comment.

Post by René J.V. Bertin
In absence of instructions to the contrary I'll interpret Milian's "feel free to respin" as "feel free to recommit", and push a commit during the day. This has been waiting to be re-applied long enough.

Given this being a free-time volunteer project, let's do such will-do-x-if-no-one-objects deadlines to at least include some days, ideally at least a WE (in general 7 days).

I totally see the frustration with having no-one reply, we all experience this in some places ourselves. Milian is sadly busy with non-kdevelop life, and no-one else so far (incl. me) had enough spare resources to try to understand what this patch does. Ideally the strategy when it comes to project file names would be documented somewhere to developers in a comment.
No spare time otherwise for this patch until WE.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


I'd be pushing less if this hadn't already been committed and then reverted because of an issue that got through the initial review process.

The main purpose of this patch is to make it possible to import (define) multipe projects from a single source directory, for instance for software that can be built either for Qt4 OR Qt5 (or Qt6), or that could use either CMake or QMake. Or a source directory on a NAS that is used from different platforms (Linux, Mac, MS Windows, etc.).
It does this by using the project name that is entered through the import wizard (1st dialog after the project file/dir selection) as the basis for the .kdev4 project files.

That's the brunt of it really; most of the complexity comes from ensuring that the project files are stored where they should be, and to integrate with the existing optional override. The details of when to ask for overriding have not changed w.r.t. the initial review; basically the override dialog is relevant (and posted) only when the new project name gives a project file (/path/to/src/foo.kdev4) that exists already.
There's an additional complication compared to the previous commit in the 5.2 branch: Greg Popov's commit that threw out the entire .kdev4 directory when importing a new project. He agreed fully that the issue which led to that change was addressed just as well by deleting the .kdev4/foo.kdev4 file if it exists already and the user chose to override the existing project.

Note that the patch doesn't make it impossible to have multiple projects in a single source tree. One could already clone, rename and edit existing .kdev4 files outside of KDevelop, and then open those. The patch just makes it possible to do this through the import wizard.

HtH...

Post by Friedrich W. H. Kossebau
Ideally the strategy when it comes to project file names would be documented somewhere to developers in a comment.

I'm tempted to say that the code should speak for itself, combined with runtime behaviour. It always struck me as counter-intuitive that KDevelop asked for a project name, and then used something else for the .kdev4 files.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Friedrich W. H. Kossebau]

kossebau requested changes to this revision.
kossebau added a comment.
This revision now requires changes to proceed.

Post by René J.V. Bertin
HtH...

Thanks for the long explanation. Though I was hoping to get things documented for future KDevelop contributors (incl. our older selves) directly in the code.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
Ideally the strategy when it comes to project file names would be documented somewhere to developers in a comment.

I'm tempted to say that the code should speak for itself, combined with runtime behaviour.

Runtime behaviour does not tell whether this is the intended behaviour.. Bug or feature? And code only does what is possible on that level, by the concepts available in the used language and methods.
What I am looking for is some human reader description of the intention. Most perfect when accompanied with unit test data as samples.

INLINE COMMENTS

Post by René J.V. Bertin
openprojectdialog.cpp:77
: KAssistantDialog( parent )
+ , m_projectDirUrl(QUrl())
, m_urlIsDirectory(false)

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.

Post by René J.V. Bertin
openprojectdialog.cpp:315
void OpenProjectDialog::validateProjectName( const QString& name )
{

The name `validateProjectName(` promises the project name is validated.
But the new code in this methods does also other stuff as sideeffect. This needs to be documented somewhere. Either in the method name (preferred), or as API comment.

Post by René J.V. Bertin
openprojectdialog.cpp:318
+ if (name != m_projectName) {
+ bool settingName = currentPage() == projectInfoPage;
+ 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);

Post by René J.V. Bertin
openprojectdialog.cpp:332
+ const QUrl url(m_projectDirUrl);

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?

Post by René J.V. Bertin
openprojectdialog.cpp:335-337
+ safeName = safeName.replace(QLatin1Char(':'), QLatin1Char('='));
+ safeName = safeName.replace(QRegExp(QStringLiteral("\\s")), QStringLiteral("_"));

No need to assign to `safeName`, `QString::replace(...)` operates on same object and just returns reference for call chains (see also usage line above).

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

openprojectdialog.cpp:277
}
- m_url.setPath( m_url.path() + QLatin1Char('/') + m_url.fileName() + QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension() );
+ if (!m_url.toLocalFile().endsWith(QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension())) {
+ m_url.setPath( m_url.path() + QLatin1Char('/') + m_url.fileName() + QLatin1Char('.') + ShellExtension::getInstance()->projectFileExtension() );

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?
Please add a code comment to make this clear. There has been some related discussion in the review comments, but on a few minutes read I have not grasped this logic, so at least for code readers like me some code comment explanation is needed.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb marked 7 inline comments as done.
rjvbb added a comment.


Where in the code would you want a documentation of the intent of this change?
The best place might be a in the handbook or the project import wizard GUI ... but we're in string freeze, no?

INLINE COMMENTS

kossebau wrote in openprojectdialog.cpp:277
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?
Please add a code comment to make this clear. There has been some related discussion in the review comments, but on a few minutes read I have not grasped this logic, so at least for code readers like me some code comment explanation is needed.

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.

kossebau wrote in openprojectdialog.cpp:315
The name `validateProjectName(` promises the project name is validated.
But the new code in this methods does also other stuff as sideeffect. This needs to be documented somewhere. Either in the method name (preferred), or as API comment.

the method originally *set* the project name, and then validated the project info - the name was thus not very appropriate already.
My change makes the method do an actual validation of the project name - the term implies that checks are made that the name is actually valid.

kossebau wrote in openprojectdialog.cpp:318
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`.
const bool isDefiningProjectName = (currentPage() == projectInfoPage);

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`).

kossebau wrote in openprojectdialog.cpp:332
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?

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.
Are you aware of other places where filenames are sanitised in a similar fashion?

kossebau wrote in openprojectdialog.cpp:335-337
No need to assign to `safeName`, `QString::replace(...)` operates on same object and just returns reference for call chains (see also usage line above).

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).

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb updated this revision to Diff 43764.
rjvbb marked 5 inline comments as done.
rjvbb added a comment.


Updated as suggested

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9344?vs=42564&id=43764

REVISION DETAIL
https://phabricator.kde.org/D9344

AFFECTED FILES
kdevplatform/shell/openprojectdialog.cpp
kdevplatform/shell/openprojectdialog.h
kdevplatform/shell/projectcontroller.cpp

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Friedrich W. H. Kossebau]

kossebau added a comment.


Rereading the patch and the related code once more, my personal opinion now is: this logic has been confusing before and only gets more confusing with the proposed patch.

This needs refactoring. e.g. by moving the actual projectfile name generation/definition into a new dedicated class caring only for this very aspect. Which then can also be properly unit tested to cover all the conditions which can be expected.

Not the most chilling review comment one likes to get, I know myself. It's the disadvantage of being the person who wants to add a feature to existing convoluted code :)
But the final result should be code which we all should feel well with and which helps to keep/make kdevelop's code base maintainable and easier to extend.

The current patch I still cannot oversee (though also due to the existing code), so I would have to grab the -1 sign for now if on the jury.

INLINE COMMENTS

rjvbb wrote in openprojectdialog.cpp:277
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.

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?
Even more, how would we actually end in this branch? It's condition is

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?
Only the personal project directory, which is hidden though in the file select dialog, no? (Not sure what is done on none unixoid systems) That might be something to give special handling to catch in case, but then rather at the begin of the if branch. Doing any bad case handling at this point seems to be like we missed something before.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
The current patch I still cannot oversee (though also due to the existing code), so I would have to grab the -1 sign for now if on the jury.

Question is: are you, and should the refactoring be done before or after this change? I'm quite sure I won't have time for a serious overhaul the coming few weeks.

Post by Friedrich W. H. Kossebau
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?

That's what I also thought in the beginning, and it is indeed what happens with cmake-based projects (for which I developed this patch initially).
Then someone reported that other project managers caused other behaviour, including saving the project file in the parent directory and creating a directory under the project file's name.
I think this must be due to the different ways different project managers leverage the import wizard, notably the fact that not all use all of the wizard's dialogs (pages).

Post by Friedrich W. H. Kossebau
Doing any bad case handling at this point seems to be like we missed something before.

I think that the underlying problem here is that a lot is going on while the import dialog(s) are open, which means the code has to be written so it works when called iteratively (the one validating the project name at each key strike, for instance).
Things would be a lot easier if user input were obtained first, and the derived variables were set afterwards. But I don't know to what extent such an implementation could support the entire current feature set, including backing up to a previous stage etc.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.


One more thing: could establishing the project file name not simply be done by a method in the ProjectController class rather than by a dedicated class? Because, in how many different functions would you split that logic?

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[Friedrich W. H. Kossebau]

kossebau added a comment.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
The current patch I still cannot oversee (though also due to the existing code), so I would have to grab the -1 sign for now if on the jury.

Question is: are you, and should the refactoring be done before or after this change? I'm quite sure I won't have time for a serious overhaul the coming few weeks.

Sorry to say, but you want to add the feature, so you would have to scratch this itch. I as co-contributor just raise my concern about making the current code more convoluted.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
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?

That's what I also thought in the beginning, and it is indeed what happens with cmake-based projects (for which I developed this patch initially).
Then someone reported that other project managers caused other behaviour, including saving the project file in the parent directory and creating a directory under the project file's name.
I think this must be due to the different ways different project managers leverage the import wizard, notably the fact that not all use all of the wizard's dialogs (pages).

But do they change things behind our back after this method has been entered? I still miss to see how m_url suddenly could be in that state latter in the method. If that happens we should rather fix this from happening, instead of covering over it here.

Post by René J.V. Bertin
One more thing: could establishing the project file name not simply be done by a method in the ProjectController class rather than by a dedicated class? Because, in how many different functions would you split that logic?

I cannot foresee how complicated the complete logic and states for estimating/maintaining a proper project file name/path is. But personally I would first look into encapsulating this aspect in a simple dedicated class instead of using the general object manager class as dumping ground for "any logic related to projects".

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
But do they change things behind our back after this method has been entered?

If you mean "they" = other project managers, then I certainly hope they don't. What I meant to say is that we're dealing here with 2 event-driven callback functions, and that control flow thus can make strange jumps.

Post by Friedrich W. H. Kossebau
I still miss to see how m_url suddenly could be in that state latter in the method. If that happens we should rather fix this from happening, instead of covering over it here.

I've been doing a lot of tracing in a debugger to understand and iron out the quirks. Sadly I didn't take notes about the exact symptoms prevented by each change, how they were triggered, etc. I'd have to break out the debugger again and try to get a hit on a well-placed breakpoint. Not to tell you to do my work, but it *would* be somewhat more efficient if you tried that too...

Post by Friedrich W. H. Kossebau
I cannot foresee how complicated the complete logic and states for estimating/maintaining a proper project file name/path is.

I cannot see how this would require a whole class rather than just a single function...

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

... you want to add the feature

BTW, I think of this more as finishing & polishing up a feature that already exists. Currently the user is asked for a project name which is hardly used at all - and as explained in the above, it is already possible to achieve the desired outcome (multiple projects defined from a single source tree). Just not through KDevelop itself and, since 5.3, as long as you don't use the project importer at all after the initial import (because it'll wipe the entire .kdev4 directory).

I'll have another look if it's possible to defer the entire m_url calculation until after the wizard exits but I seem to recall I already did that once and failed (or just gave up). There's almost no documentation of how that wizard does what it does, as you must have observed yourself.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
Sorry to say, but you want to add the feature, so you would have to scratch this itch

Sorry me too, but you cannot expect someone to scratch your itches if all you can say is where not to scratch (no matter how well I know that kind of itch).

I guess this will then be just another one of my proposed changes that remains in limbo until someone else gives a green light or I/someone finally decide to see what other requist will come in after figuring out how to deal with this one. :-/

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight