[svn] Added SVN Checkout dialog.
ClosedPublic

Authored by nikolaik on May 6 2020, 5:45 PM.

Details

Summary


Added SVN Checkout dialog with basic and most commonly used SVN Checkout functionality:

  • set repo URL;
  • change working copy path;
  • omit externals.

Additional features:

  • automatically pasting repo URL on dialog creation (if its valid);
  • automatically forming working copy path based on repo URL (trying to predict repository name);
  • user can pick an output folder instead of typing in;
  • few error-controls to prevent running 'svn co' with invalid params (disabling 'OK' button).

SVN Checkout can be time consuming.

Diff Detail

Repository
R449 Plugins for Dolphin
Branch
svn_checkout (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26505
Build 26523: arc lint + arc unit
nikolaik requested review of this revision.May 6 2020, 5:45 PM
nikolaik created this revision.
nikolaik retitled this revision from Added SVN Checkout dialog. to [svn] Added SVN Checkout dialog..May 6 2020, 5:50 PM
nikolaik edited the summary of this revision. (Show Details)
anthonyfieroni added inline comments.
svn/fileviewsvnplugin.cpp
127–130 ↗(On Diff #82126)

This action is shown when plugin is loaded, but that's when it has folder .svn

nikolaik marked an inline comment as done.May 6 2020, 6:36 PM
nikolaik added inline comments.
svn/fileviewsvnplugin.cpp
127–130 ↗(On Diff #82126)

There is a new public virtual function for out-of-version-control folders, https://phabricator.kde.org/D29041

meven added a comment.May 7 2020, 12:31 PM

A couple of minor points, but this looks great already.

svn/svncheckoutdialog.cpp
72 ↗(On Diff #82126)

Check that repoPath looks like a remote svn repo url with schemes svn:/ https:/ or http:/ (can svn use others ?)

73 ↗(On Diff #82126)

Don't need to else here.
A user might have the url in the clipboard and the target dir selected.

90 ↗(On Diff #82126)

Can reuse variable stripped

meven added inline comments.May 7 2020, 12:32 PM
svn/svncommands.cpp
420 ↗(On Diff #82126)

If we could add some progress dialog of some kind, allowing the user to kill the process.

nikolaik edited the summary of this revision. (Show Details)May 7 2020, 12:57 PM
nikolaik marked 2 inline comments as done.
nikolaik marked an inline comment as done.May 7 2020, 1:22 PM
nikolaik added inline comments.
svn/svncheckoutdialog.cpp
73 ↗(On Diff #82126)

Yes, that is correct, this is the most common use strategy.
The idea was "if clipboard contains correct URL set it through on_leRepository_textChanged() which sets work directory path, if no URL just set the work directory path to the current dir".
Do you think about something better?

svn/svncommands.cpp
420 ↗(On Diff #82126)

This is a huge problem now and it is in todo list.
SvnCommands should become a singleton with signal-slots connection to run svn process and not block Dolphin main thread.
I think this is a separate commit change.
If we can't accept this without a normal multithreading it's ok i will try to implement it first.

nikolaik updated this revision to Diff 82198.May 7 2020, 1:24 PM
nikolaik marked an inline comment as done.

Update in response to review comments: added more reuse of 'stripped' variable.

nikolaik marked an inline comment as done.May 7 2020, 1:25 PM
nikolaik added inline comments.May 7 2020, 1:46 PM
svn/svncheckoutdialog.cpp
72 ↗(On Diff #82126)

http://svnbook.red-bean.com/en/1.2/svn-book.html#svn.basic.in-action.wc.tbl-1
Ok, we can check probably with regex. Some basic checks: this is URL (QUrl().isValid()) and starts correct.

nikolaik updated this revision to Diff 82225.May 7 2020, 5:18 PM

More appropriate repo URL validation.

nikolaik marked 2 inline comments as done and an inline comment as not done.May 7 2020, 5:19 PM
meven accepted this revision as: meven.May 7 2020, 5:44 PM

All in all this is good.

I hope you'll do a git dialog ;)
Unless I do first.

svn/svncheckoutdialog.cpp
51 ↗(On Diff #82126)

I would have reversed this, first QUrl::fromUserInput, then isvalid() then .scheme() in [listof svn repo url scheme], but this is fine

73 ↗(On Diff #82126)

I missed textChanged edited leCheckoutDir.
Your idea is nice.

This revision is now accepted and ready to land.May 7 2020, 5:44 PM
nikolaik updated this revision to Diff 82229.May 7 2020, 6:46 PM

Improved URL checks.

nikolaik marked 4 inline comments as done.May 7 2020, 6:47 PM
meven accepted this revision.May 11 2020, 6:41 AM
meven added inline comments.
svn/svncheckoutdialog.cpp
54 ↗(On Diff #82126)

You could return here.

nikolaik updated this revision to Diff 82496.May 11 2020, 8:38 AM

Update in response to review comments.

nikolaik marked an inline comment as done.May 11 2020, 8:39 AM
meven added a comment.May 11 2020, 1:08 PM

I think you can land it, code is nice and changes should be consensual.

svn/svncheckoutdialog.cpp
68 ↗(On Diff #82126)

Do you really need QObject:: here ?

70 ↗(On Diff #82126)

And here ?

elvisangelaccio accepted this revision.May 11 2020, 5:40 PM
elvisangelaccio added inline comments.
svn/svncheckoutdialog.h
36–37 ↗(On Diff #82126)

Please use camelCase.

nikolaik updated this revision to Diff 82605.May 11 2020, 6:38 PM

Moved from QObject::connect to connect.

nikolaik marked 5 inline comments as done.May 11 2020, 6:42 PM
nikolaik added inline comments.
svn/svncheckoutdialog.h
36–37 ↗(On Diff #82126)

This is for slots autoconnection: https://doc.qt.io/qt-5/qmetaobject.html#connectSlotsByName
I can manually connect if needed.

elvisangelaccio accepted this revision.May 11 2020, 8:48 PM
elvisangelaccio added inline comments.
svn/svncheckoutdialog.h
36–37 ↗(On Diff #82126)

Ah sorry, didn't know about this feature :)

nikolaik marked 3 inline comments as done.May 12 2020, 5:44 AM
This revision was automatically updated to reflect the committed changes.