Review Request: Use the KVersionControlPlugin for the next highest folder not the first one that finds a folder anywhere up in the filesystem

[Frank Reininghaus]

> On Oct. 27, 2012, 1:38 p.m., Frank Reininghaus wrote:
> > Thanks for the patch! Good catch, I wasn't aware of this problem.
> > 
> > Looks good, I'm just wondering if we could maybe do it in a slightly simpler way, without the new function: Maybe one could move the code starting with "// Version control systems..." out of the foreach-loop, then replace "while (upUrl != dirUrl)" by "if (upUrl != dirUrl)" and have the function call itself recursively if that is the case?
> 
> Dominik Schmidt wrote:
>     Of course we could squeeze it all into one big function, but I'd find it way harder to read - just matter of taste. 
>     
>     Also I'm not exactly sure how/what the static locals are meant to work/do and I'd be afraid to break stuff when changing too much and recursively calling that function ;-)

With my suggestion, the modified function VersionControlObserver::searchPlugin() would actually be *shorter* than it is now, so I don't think that this approach can be called "squeezing it all into one big function" ;-) And why that would be "way harder to read" is also not clear to me.

An alternative to using recursion would be to still move the entire "upUrl" stuff out of the foreach-loop, and put the search for "fileName" into a new foreach-loop that iterates over the plugins. But the recursive solution requires less code, and that makes the code easier to read IMHO.

Static locals keep their values between calls of the function. They are initialised the first time the function is called, and subsequent calls are faster because the expensive search for the plugins is not needed any more.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107066/#review21003
-----------------------------------------------------------


On Oct. 26, 2012, 11:52 p.m., Dominik Schmidt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107066/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2012, 11:52 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Sorry, for the confusing summary, couldn't describe it better in one sentence, suggestions for a commit message are very much appreciated.
> 
> 
> The current behaviour of dolphin is:
> if for the first plugin in the list of loaded plugins the fileName is not found in the current directory, it looks in the directory above, if it's not found there it looks in the directory above and so on - all with the same plugin.
> This leads to a problematic situation with for example Dropbox. Dropbox-Sync folders are identified by the .dropbox folder in their top-level. but also there is a ~/.dropbox. So when you enable the dolphin-box-plugin, basically no other KVersionControlPlugin will be able to work. It will go up to your home and think everything inside it is tracked by Dropbox.
> 
> This patch makes dolphin check all plugins for the current directory, then go one directory up, check all plugins again, go one up etc.. 
> 
> 
> P.S.: Is there any better/more flexible API to use for sync-plugins? 
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/versioncontrol/versioncontrolobserver.h 501af7d 
>   dolphin/src/views/versioncontrol/versioncontrolobserver.cpp 42e00de 
> 
> Diff: http://git.reviewboard.kde.org/r/107066/diff/
> 
> 
> Testing
> -------
> 
> I can use other plugins simultaneously with the Dropbox one now... 
> 
> 
> Thanks,
> 
> Dominik Schmidt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121028/af1127db/attachment.htm>