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

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.

In D9344#239331, @rjvbb wrote:

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

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.

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?

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.

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.

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.

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

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.

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

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 .

Push to master, I'd say.

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.

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.

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.

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

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.

Contrary to what I wrote earlier, your assumption was correct. The RemoveFilename bit was necessary to avoid growing m_url because of using itself in repeated setPath() calls which in turn was because I had hoped to avoid having to add an additional member variable that holds the initial project dir URL when the user starts changing the default project name.

I fixed the issue by adding that member variable, setting it only when needed.

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.

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?

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

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.

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.

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.

In D9344#339729, @rjvbb wrote:

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.

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

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.

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?

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.

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.

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

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.

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?

In D9344#346281, @rjvbb wrote:

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.

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.

In D9344#346362, @rjvbb wrote:

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

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.

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

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

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

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. :-/