Differential D26259

Fixes svn plugin issuing "mkfifo" on Windows.
ClosedPublic
Actions

Authored by nikolaik on Dec 28 2019, 4:30 PM.

Details

Reviewers
elvisangelaccio
meven
ngraham
Group Reviewers
Dolphin
Commits
R449:0df2e299f415: Fixes svn plugin issuing "mkfifo" on Windows.
Summary

BUG: 319546
Non-blocking portable solution without any shell use.

Diff Detail

Repository
R449 Plugins for Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nikolaik requested review of this revision.Dec 28 2019, 4:30 PM
nikolaik created this revision.
meven requested changes to this revision.Jan 7 2020, 2:57 PM
meven added inline comments.
svn/CMakeLists.txt
24 ↗(On Diff #72302)

Remove

svn/fileviewsvnplugin.cpp
339

No need for exceptions here, if you throw it yourself, you can can call what you put in your catch catch directly here, and then return

This revision now requires changes to proceed.Jan 7 2020, 2:57 PM
nikolaik updated this revision to Diff 73014.Jan 7 2020, 7:50 PM

Updated in response to review comments: no exceptions.

meven added a comment.Jan 8 2020, 9:53 AM

Can we make sure to clean the temp file ?

meven added a comment.Jan 8 2020, 9:57 AM
In D26259#590136, @meven wrote:

Can we make sure to clean the temp file ?

Just read doc of QTemporaryFile, this is already taken care of.

svn/fileviewsvnplugin.cpp
312

You don't need a pointer I think QTemporaryFile file(tmpFileNameTemplate, process);

nikolaik added inline comments.Jan 8 2020, 10:34 AM
svn/fileviewsvnplugin.cpp
312

The QTemporaryFile object will be destroyed right after function ends not when QProcess got destroyed. At that time comparator process could not even start to loading it (I see it on my machine).
I think we should destroy temporary file when comparator process no longer needs it. So as long as comparator process is running the file must be there. The best way to achive this is through the QTemporaryFile *file = new QTemporaryFile(tmpFileNameTemplate, process);

meven added a comment.Jan 8 2020, 10:36 AM

Seems fine to me, please wait for others to have some time to review.
I will accept it eventually.

svn/fileviewsvnplugin.cpp
312

Thanks

elvisangelaccio requested changes to this revision.Jan 12 2020, 6:32 PM
elvisangelaccio added inline comments.
svn/fileviewsvnplugin.cpp
311

Please use QStringLiteral("%1/%2.XXXXXX").arg(...) instead. No need to use QDir::separator().

315

No space before/after ()

315

I'd just show a generic "Could not show local SVN changes.". Maybe we could print a qWarning() saying that the temporary file could not be created (which is unlikely though...).

330

"Could not show local SVN changes: svn diff failed".

343

"Could not show local SVN changes: could not start kompare". This would also help in the case kompare is not installed.

This revision now requires changes to proceed.Jan 12 2020, 6:32 PM
nikolaik updated this revision to Diff 73370.Jan 13 2020, 7:35 AM

Updated in response to review comments.

nikolaik marked 8 inline comments as done.Jan 17 2020, 10:13 AM
nikolaik added inline comments.
svn/fileviewsvnplugin.cpp
315

Don't know if we should add qWarning() only here. Maybe we should add it on every error in this function or nowhere.
Anyway i am ready to make all the required corrections.

elvisangelaccio requested changes to this revision.Jan 19 2020, 9:51 PM

I noticed the following warning if I close dolphin while kompare is still running:

QProcess: Destroyed while process ("kompare") is still running.

We should find a way to close kompare gracefully in this case, or even keep it running if possible (which is the current behavior).

svn/fileviewsvnplugin.cpp
315

It's okay, it's never gonna happen anyway...

This revision now requires changes to proceed.Jan 19 2020, 9:51 PM
anthonyfieroni added a subscriber: anthonyfieroni.Jan 20 2020, 7:02 AM

Use startDetached instead.

nikolaik updated this revision to Diff 74226.Jan 23 2020, 1:22 PM

Thanks for comments.
Updated: now we are using startDetached() because one may close Dolphin but need kompare running.
Provided solution has a flaw: temporary files will be deleted on Dolphin close. So we make an assumption: when the file got deleted diff program should already load it and no longer needs it.
This flaw seems insignificant.
Also patch keeps current behavior of kompare running after Dolphin close intact.

nikolaik marked 3 inline comments as done.Jan 23 2020, 1:23 PM
elvisangelaccio added inline comments.Jan 25 2020, 11:26 AM
svn/fileviewsvnplugin.cpp
311

Please write "when the file gets deleted kompare has already loaded it and no longer needs it."

336

Please call it started and make it const.

nikolaik updated this revision to Diff 74356.Jan 25 2020, 12:01 PM

Updated in response to review comments.

nikolaik marked 2 inline comments as done.Jan 25 2020, 12:02 PM
elvisangelaccio accepted this revision.Jan 26 2020, 10:06 AM

LGTM now.

@meven What about you?

meven accepted this revision.Jan 26 2020, 8:14 PM
This revision is now accepted and ready to land.Jan 26 2020, 8:14 PM
elvisangelaccio added a comment.Jan 26 2020, 9:05 PM

Patch doesn't seem to apply, can you rebase it on master please?

svn/fileviewsvnplugin.cpp
345

This return is redundant since there is nothing after.

nikolaik updated this revision to Diff 74407.EditedJan 27 2020, 7:26 AM

Removed redundant return statement, feature branch rebased on master.

nikolaik marked an inline comment as done.Jan 27 2020, 7:27 AM
elvisangelaccio added a comment.Feb 1 2020, 9:55 PM

Still does not apply for me.

nikolaik updated this revision to Diff 74889.Feb 3 2020, 7:32 AM

One more try: applied changes directly to master and upload diff.

nikolaik edited the summary of this revision. (Show Details)Feb 3 2020, 7:33 AM
meven added a comment.Feb 14 2020, 6:12 PM

The patch applies fine on master.
I think you can merge.

meven accepted this revision.Feb 14 2020, 6:12 PM
nikolaik added a comment.Feb 14 2020, 7:18 PM

I wish i could but i don't have developer account. Need invitation or something i guess.

meven added a comment.Feb 17 2020, 8:55 AM

I can merge this for you, I just need your email, I already know your name thanks to phabricator but it does not let me see your actual commit for some reason and your email that's in there.

nikolaik added a comment.Feb 17 2020, 10:05 AM

nkrasheninnikov@yandex.ru
Thanks

Closed by commit R449:0df2e299f415: Fixes svn plugin issuing "mkfifo" on Windows. (authored by nikolaik, committed by meven). · Explain WhyFeb 18 2020, 8:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msvn/fileviewsvnplugin.cpp (40 lines)

Diff 75884

View Options

svn/fileviewsvnplugin.cpp

Loading...