Oh, I usually squash my changes into the original commit before uploading the change (i.e. via arc diff). I do not use an extra branch either.

In D21156#486838, @kfunk wrote:

Huh? It's easier than that:

• If you do not have the commit yet locally:
  
  ` arc patch --nobranch <ID> git push `

Or probably a simpler approach is to run these two commands irregarding the fact that I have the commits locally or not. So I just pull the 'clean' modification with the correct commit message and then push to a target branch. E.g: git push origin 5.3

• If you do have the commit and it is the latest one in your history: ` arc amend git push `

Sorry but it is still not clear if this would work. In this diff I have three commits (modification_1, modification_2, revert modifications_2) in a feature branch forked from master. I need to push to 5.3. If I run those two commands I expect that three commits are pushed (not a single one with the total modification) and I would expect that those are pushed to origin/my-feature-branch not in 5.3. Isn't it?

> I suggest to do a `arc amend` (to basically update the commit message with current reviewers, "Differentiatl Revision" line, etc.) and then `git push` your change manually to the right branch. Let's you use your normal git command-line to actually push changes, which to me is a much more thrust-worthy approach than to rely on arc to do that for me...

Probably this should be added to the guide. And probably it should also be added that the commits should be squashed (thing that `arc land` does automatically).

Huh? It's easier than that:

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

I suggest to do a arc amend (to basically update the commit message with current reviewers, "Differentiatl Revision" line, etc.) and then git push your change manually to the right branch. Let's you use your normal git command-line to actually push changes, which to me is a much more thrust-worthy approach than to rely on arc to do that for me...

In D21156#486774, @simgunz wrote:

In the phabricator guide it is suggested to push manually when the target branch in not master:

https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

In D21156#486774, @simgunz wrote:

In D21156#486717, @aacid wrote:

If you're not using arc you should have included the "Differential Revision" line in the commit so it would close automatically as stated in https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

I'll close this manually for you :)

In the phabricator guide it is suggested to push manually when the target branch in not master:

https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

In D21156#486717, @aacid wrote:

If you're not using arc you should have included the "Differential Revision" line in the commit so it would close automatically as stated in https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

I'll close this manually for you :)

If you're not using arc you should have included the "Differential Revision" line in the commit so it would close automatically as stated in https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

I have pushed to 5.3 and then merged to master manually, as specified in the Phabricator page guide. How do I close the revision now?

lgtm, thanks - do you have commit rights? if so, please push to the 5.3 branch

ScriptErrorFilteringStrategyFix: all the items where set as InformativeItem even when there were no matches

This patch adds some patterns to ScriptErrorFilterStrategy to handle some PHP errors so that you can click on the message and the matching file is opened.

• Document feature

I reverted the commit were the Ctrl feature was disabled, going back to the first fix I proposed

In D18758#481378, @arrowd wrote:

R32:bd048e67f056b5be25ed57fb2be947444f68c24e

In D18758#481339, @mwolff wrote:

so, I've now committed an alternative fix (or so I hope...) see:

commit bd048e67f056b5be25ed57fb2be947444f68c24e
Author: Milian Wolff <mail@milianw.de>
Date:   Mon Jun 17 22:26:32 2019 +0200

    Guard against crashes when IStatus object gets destroyed at bad times

I confirm this fixes the issue for me. Yay, thanks!

In D18758#481339, @mwolff wrote:

so, I've now committed an alternative fix (or so I hope...) see:

commit bd048e67f056b5be25ed57fb2be947444f68c24e
Author: Milian Wolff <mail@milianw.de>
Date:   Mon Jun 17 22:26:32 2019 +0200

    Guard against crashes when IStatus object gets destroyed at bad times

having looked at the raw diff quickly, I like what I'm seeing. What boilerplate are you referring to?

so, I've now committed an alternative fix (or so I hope...) see:

ok, sorry for the rabbit hole I sent you down. I still think that long-term we will need something like QPromise, but phabricator doesn't even let me view the interesting changes in shell/... so I cannot comment on the boilerplate

In D21156#466636, @simgunz wrote:

In D21156#465844, @mwolff wrote:

you are removing a feature, but only partially - a lot of code would become superfluous by this change and should be cleaned up accordingly

As rjvbb said, no feature have been removed. The feature is still there through the Alt key (as it was before), while not accessible anymore with the Ctrl key. There is no reason to have the same (undocumented) feature on two different keys. Moreover there is no code to cleanup, what is there is needed to have the feature working on the Alt key (at least for my understanding of the code, I have not noticed unuseful blocks).

In D21580#481264, @kossebau wrote:

This sadly seems to have broken navigating with the "Next item"/"Previous item" actions for me. Please give this a look.

This sadly seems to have broken navigating with the "Next item"/"Previous item" actions for me. Please give this a look.

ping

In D21589#477664, @Petross404 wrote:

brauch is referring to this

for /f "usebackq tokens=3*" %%a in (`reg query "HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\VisualStudio\SxS\VS7" /s`) do (
     set vs15_path=%%a %%b
     if exist "!vs15_path!Common7\Tools\VsDevCmd.bat" (
	set "VS150COMNTOOLS=!vs15_path!Common7\Tools\"
	goto :end
     )
   )
)
:end

Oops. What happened here is I was hacking on the release version which looks more like the patch I posted. I'll see if I can make sense of the master branch version and make it more like that!

brauch is referring to this

In D21589#477604, @brauch wrote:

Why did you replace the registry query by a fixed path?

Why did you replace the registry query by a fixed path?

LGTM, maybe @kfunk or @brauch know more?

Missing idx_line

lgtm

In D18758#469623, @apol wrote:

Quite possibly, it doesn't happen outside of tests after all, right?

Works fine for me on Ubuntu 19.04 -- lgtm!

Ping?

Quite possibly, it doesn't happen outside of tests after all, right?

I'm more interested in fixing the intial issue. With KIO jobs in openProject turned async, the crash now happens right in qWait(1) of KDevelop::TestCore::shutdown(). And what's strange is that adding qWait(1) at the end of testReload() test still fixes issue.

Can't we really fix this without bringing a dependency?
If we really want the dependency, can we just link against it instead of dragging it into our source code?

Changes:

• Import QtPromise 0.5.0 into kdevplatform/3dparty/ directory.
• Remove synchronious KIO calls from Project::open() and use QPromises there instead.

In D21156#465844, @mwolff wrote:

you are removing a feature, but only partially - a lot of code would become superfluous by this change and should be cleaned up accordingly

you are removing a feature

you are removing a feature, but only partially - a lot of code would become superfluous by this change and should be cleaned up accordingly

Sounds good enough for me then!

With the Alt key the bug does not appear, because the existing code disables correctly the browse mode when Alt is released. For some reasons key press and release events are different for Ctrl key but are the same for Alt. Probably the method avoidMenuAltFocus manages this behavior correctly for the Alt key (I have not analyzed in detail what it does) and then we enter correctly in the if at line 230 where source browse mode is disabled. So removing the redundant Ctrl key solves the bug completely (at least I haven't manage to reproduce it).

You identified an event chain which leads to the browsing mode not being restored correctly. This should happen less often with the Alt modifier but it can still happen (hit Alt to display a tooltip or open a menu and then move the mouse over that tooltip or select a menu item?).

• Disable Ctrl and keep only Alt

Feel free to push it into the 5.3 branch then.

No problem. I don't think I'm yet involved enough to apply - besides being a user.

In D20142#446500, @christiant wrote:

Probably not - this is my first commit. Where do I need to apply :-D

Anything I need to do?

patch lgtm now, many thanks!

In D18224#450943, @thomassc wrote:

Upon reopening a file, we should only update the problems of changed files that got updated. Otherwise we should only grab the TU for the main .cpp file and attach it, such that we can do code completion.

If I understand this correctly, then perhaps something does not behave as it should there, since in some cases, a lot of files' problems seem to get updated when re-opening a file, including lots of system headers that certainly haven't changed. In other cases, no calls to ParseSession::problemsForFile() are made. It seems that the first case can be triggered by making changes to a header, closing it without saving, and then re-opening it.

Upon reopening a file, we should only update the problems of changed files that got updated. Otherwise we should only grab the TU for the main .cpp file and attach it, such that we can do code completion.

Re-add m_diagnosticsCache with a hopefully correct caching implementation. Differing from the original situation, the new function createExternalProblem() adapts the problems it creates to the specific file they are inserted in. The cache thus only caches the original problems as created by ClangDiagnosticEvaluator::createProblem(). createExternalProblem() makes copies of these with a new copy constructor added to ClangProblem.

quite obviously libclang doesn't handle it

Do you mean something like `#include <Foo>` which then contains a `#include <foo.h>`?

In D18551#430847, @rjvbb wrote:

Re: reparsing reliably each time a headerfile is changed: wouldn't the use of forwarding headers increase the chance of missing a change?

In D18224#449416, @thomassc wrote:

I re-tested the behavior on files from an actual project and it showed the same behavior when editing files as described in my last post, regardless of how many files are included from the files that are edited. I noticed that the behavior is different when re-opening files: in this case, problemsForFile() is actually called for a larger set of files (although from only looking at this set of files, it seemed a bit random and it was not clear to me how it is determined, I'll have to dig into the source code for this at some point ...).

In D18758#450081, @apol wrote:

In D18758#449319, @mwolff wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

That would mean adding a bunch of noexcept all over the place or risk quite some performance penalty. I'd prefer keeping it localised.

In D18758#449319, @mwolff wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

I am sorry, I think that I misunderstood how the cache works. Please disregard my previous message. I am wondering where the problem that I've been seeing then comes from, seems like it has a different origin.

I re-tested the behavior on files from an actual project and it showed the same behavior when editing files as described in my last post, regardless of how many files are included from the files that are edited. I noticed that the behavior is different when re-opening files: in this case, problemsForFile() is actually called for a larger set of files (although from only looking at this set of files, it seemed a bit random and it was not clear to me how it is determined, I'll have to dig into the source code for this at some point ...).

In D18758#449319, @mwolff wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

Hey Thomas, please don't remove the cache. See f2a6941e086cdf506c8fb1798c52982bff43792d for why this was introduced. Your tests don't include other files, so probably that's why you didn't see any effect of the cache?

thanks, lgtm!