[versioncontrolobserver] Do not use static plugin objects

Authored by anthonyfieroni on Dec 17 2018, 2:54 PM.



Using static object result in showing error message in one versioned tab to be shown in all versioned

Diff Detail

R318 Dolphin
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
anthonyfieroni created this revision.Dec 17 2018, 2:54 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptDec 17 2018, 2:54 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
anthonyfieroni requested review of this revision.Dec 17 2018, 2:54 PM

Use parent in createInstance

@elvisangelaccio i want to go ahead with this patch.

elvisangelaccio changed the repository for this revision from R449 Plugins for Dolphin to R318 Dolphin.Dec 30 2018, 7:08 PM
elvisangelaccio requested changes to this revision.Dec 30 2018, 7:23 PM
elvisangelaccio added inline comments.

This looks like an unrelated change. If a parent here is necessary, please add it with another commit.

If instead this change is indeed related to this patch, please explain why in the commit message. I'm not confident about this code because there is multi-threading involved.

This revision now requires changes to proceed.Dec 30 2018, 7:23 PM
anthonyfieroni added inline comments.Dec 30 2018, 8:04 PM

It prevents leak, in previous commit plugins leave to application lifetime, now not.

Ping, we can remove parent but i don't see advantage of this.

I want to have this in 18.12.1, i really not prefer to have custom changes against upstream.

elvisangelaccio accepted this revision.Jan 5 2019, 6:52 PM

Well ok, you seem confident enough :)

This revision is now accepted and ready to land.Jan 5 2019, 6:52 PM
anthonyfieroni updated this revision to Diff 48767.EditedJan 5 2019, 10:59 PM

Apparently it has a problem, if 2 or more versioned tabs are open (in distinct git directories) switching between does not update status/actions. It looks like in dolphin-plugin is used static object as well. So after all we don't want error in one tab to be multiplied to all opened, i've seen it often, since i'm using plugin daily, so easy fix is to update directory on tab activated, since observer has only private slots i'm using old syntax did you have better idea?

anthonyfieroni added inline comments.Jan 14 2019, 6:25 PM
191 ↗(On Diff #48767)

@elvisangelaccio are you ok with this line?

elvisangelaccio requested changes to this revision.Jan 15 2019, 4:22 PM

Sorry but I can't reproduce the bug you put in the commit message.

I tried to do a git checkout on a dirty repo, but the error message only shows up in the versioned tab (not in all tabs).

Please update the commit message with a proper test plan.

This revision now requires changes to proceed.Jan 15 2019, 4:22 PM
anthonyfieroni edited the summary of this revision. (Show Details)Jan 16 2019, 5:47 AM

Sorry but I can't reproduce the bug you put in the commit message.

I tried to do a git checkout on a dirty repo, but the error message only shows up in the versioned tab (not in all tabs).

Ok, make second tab versioned too, now error from first tab is shown in second as well.

ping, issue is real and annoying.

@alexeymin, can you try this patch as well, it's pretty annoying to have 5+ versioned tabs, error in one to be shown in others.

alexeymin added a comment.EditedJan 28 2019, 1:31 PM

I can reproduce an issue. Without patch, VCS error generated in one tab is displayed in all versioned tabs (but not in normal unversioned tabs). Also strangely error is multiplied only on the first occurence, if after first appearance of error I close all error messages in all tabs, and trigger error again, it only appears in current tab, and not in any other.

With this patch I cannot reproduce error multiplication among any tabs.

191 ↗(On Diff #48767)

Not really. What's this connection needed for?

If we really need it, we should find a better way.

anthonyfieroni added inline comments.Jan 29 2019, 6:35 AM
191 ↗(On Diff #48767)

Ensures current tab is in sync with gitWrapper in dolphin plugins witch is singleton i.e. when you click tab it call git status on current directory. This line is pretty simple fix, without adding other slot, if we want new syntax we should add new public slot that call internally delayedDirectoryVerification

191 ↗(On Diff #48767)

Ok, so it's another bugfix but unrelated from this patch. Please move it to another commit then.

And yes, we should try to use the new syntax if possible.

This revision was not accepted when it landed; it landed in state Needs Revision.Jan 29 2019, 6:28 PM
This revision was automatically updated to reflect the committed changes.

@anthonyfieroni Next time please add the summary of the revision to the actual commit message. You may want to setup arc to do that.