Differential D29488

Added VCS plugins signals connections for outOfVersionControl() actions.
ClosedPublic
Actions

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

Details

Reviewers
meven
elvisangelaccio
Group Reviewers
Dolphin
Commits
R318:9b7fc506aa3a: Added VCS plugins signals connections for outOfVersionControl() actions.
Summary

Plugins can now do out of version control actions (D29041), so we need them to inform of their state.
This change connects every plugin signal to Dolphin on plugin instantiation.

Test Plan
  1. Try SVN Update of inaccessible repository: see SVN error message shows up.
  2. Try pull inaccessible git repository: see GIT error message shows up.
  3. Try SVN Commit: see SVN message.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nikolaik created this revision.May 6 2020, 5:33 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMay 6 2020, 5:33 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
nikolaik requested review of this revision.May 6 2020, 5:33 PM
nikolaik edited the test plan for this revision. (Show Details)May 6 2020, 5:38 PM
nikolaik added reviewers: Dolphin, meven, elvisangelaccio.
elvisangelaccio requested changes to this revision.May 11 2020, 8:52 PM
elvisangelaccio added inline comments.
src/views/versioncontrol/versioncontrolobserver.cpp
170–171

What do you mean with "connect needed after" ?

Btw I'd write "Disconnect every plugin"

171

Please call the variable plugin.

195

Same here.

This revision now requires changes to proceed.May 11 2020, 8:52 PM
nikolaik updated this revision to Diff 82618.May 12 2020, 6:35 AM

Update in response to review.

nikolaik marked 3 inline comments as done.May 12 2020, 6:45 AM
nikolaik added inline comments.
src/views/versioncontrol/versioncontrolobserver.cpp
170–171

Then better without comment at all: code is self-explaining here.
The purpose of the comment was to show the idea: we disconnect and connect later.

nikolaik marked an inline comment as done.May 12 2020, 6:45 AM
meven added inline comments.May 15 2020, 11:12 AM
src/views/versioncontrol/versioncontrolobserver.cpp
184

So this happens when a directory was previously versioned ?

196

Those lines are present line 178, we could avoid the repetition maybe.
It seems we can make those connect at plugin instantiation.

nikolaik updated this revision to Diff 82927.May 15 2020, 12:35 PM
nikolaik marked 2 inline comments as done.

Update in response to review comments.

nikolaik added inline comments.May 15 2020, 12:36 PM
src/views/versioncontrol/versioncontrolobserver.cpp
184

This happens for currently unversioned directory for any reason.

196

We can connect all signals (even itemVersionsChanged) on plugin creation. This is a more straightforward way, its simpler. No need of this "disconnect all - connect needed" every time on directory update.
On the other hand this changes current behavior, might be difficult to debug finding signal sender, broken plugin may broke everything else.
But we are in control of dolphin-plugins aren't we?

Nice idea, i will code it.

meven added inline comments.May 15 2020, 12:53 PM
src/views/versioncontrol/versioncontrolobserver.cpp
183

You can avoid touching this block now :) replacing back else { if { to else if {

nikolaik updated this revision to Diff 82930.May 15 2020, 1:13 PM

Update in response to review.

nikolaik marked 4 inline comments as done.May 15 2020, 1:14 PM
meven accepted this revision.May 15 2020, 1:48 PM

Commit message should be updated with latest changes.

nikolaik edited the summary of this revision. (Show Details)May 15 2020, 2:34 PM
In D29488#671698, @meven wrote:

Commit message should be updated with latest changes.

Ready to land.

meven added a comment.May 15 2020, 3:06 PM
In D29488#671753, @nikolaik wrote:
In D29488#671698, @meven wrote:

Commit message should be updated with latest changes.

Ready to land.

I think this can land.
So you can move forward on other great improvements to those dolphin plugins.

This revision was not accepted when it landed; it landed in state Needs Review.May 15 2020, 8:09 PM
Closed by commit R318:9b7fc506aa3a: Added VCS plugins signals connections for outOfVersionControl() actions. (authored by nikolaik). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 82976

View Options

src/views/versioncontrol/versioncontrolobserver.cpp

Loading...