Review Request 125136: Add an interface which allow plugin to show custom overlay icons

[Olivier Goffart]

> On Sept. 10, 2015, 7:55 nachm., Emmanuel Pescosta wrote:
> > Thanks for the patch! :)
> > 
> > Yes the KVersionControlPlugin has some major drawbacks :/
> > 
> > > What I do is that if I don't have the thng cached
> > 
> > Should each plugin implement his own cache? 
> > (Maybe add the caching to the overlay icons service instead if you like the idea of the 'service', so that the plugins don't need to worry about the lookup caching)

I think it's fine for plugin to have their cache. I looked at the current dolphin-plugin and they also maintain a cache.  It is not really an icon cache, it is a status cache, that is domain specific to what the plugin is doing.


> On Sept. 10, 2015, 7:55 nachm., Emmanuel Pescosta wrote:
> > src/kitemviews/kfileitemmodelrolesupdater.cpp, lines 1104-1107
> > <https://git.reviewboard.kde.org/r/125136/diff/1/?file=402574#file402574line1104>
> >
> >     overlays_new.count >= overlay_old.count after each slotOverlaysChanged call -> the list of overlays will grow indefinitely!

I don't really understand your comment.  itm.overlays() are the overlays returned by KIO which does not contains any overlay of any plugins.
It is not the same as the data "iconOverlays".


> On Sept. 10, 2015, 7:55 nachm., Emmanuel Pescosta wrote:
> > src/views/versioncontrol/koverlayiconplugin.h, line 39
> > <https://git.reviewboard.kde.org/r/125136/diff/1/?file=402577#file402577line39>
> >
> >     unused?

Reserved for future use.  
You don't do that?


> On Sept. 10, 2015, 7:55 nachm., Emmanuel Pescosta wrote:
> > src/views/versioncontrol/koverlayiconplugin.h, line 48
> > <https://git.reviewboard.kde.org/r/125136/diff/1/?file=402577#file402577line48>
> >
> >     Do we really need a KFileItem? Maybe we can use QUrl instead? (would it make more consistent with the overlaysChanged signal)

QUrl is probably fine.  KFileItem is what other interfaces use (KAbstractFileItemActionPlugin, KVersionControlPlugin)

I changed it to QUrl anyway.


- Olivier


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125136/#review85134
-----------------------------------------------------------


On Sept. 11, 2015, 9:08 vorm., Olivier Goffart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125136/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 9:08 vorm.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Add an interface which allow plugin to show custom overlay icons
> 
> 
> My use case is an owncloud plugin.
> 
> I could not simply use KVersionControlPlugin because it is lacking some state (the shared state).
> Also, that plugin does not let me update only one file at the time. (there is no signal to tell dolphin that only one icon has changed.)  KVersionControlPlugin::versionState is supposed to be blocking because it is called from a thread but then it calls it for all the files sequencially which is slow if there are many files.
> 
> Instead, this new KOverlayPlugin::getOverlays is supposed to return fast. What I do is that if I don't have the thng cached, i return nothing, but then i start the query, and then call the overlaysChanged changed signal.
> 
> 
> I currently put this class in the version control library.  Should it go in KIOWidgets instead? (next to KAbstractFileItemActionPlugin)
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt e0f91de 
>   src/kitemviews/kfileitemmodelrolesupdater.h 216b0a5 
>   src/kitemviews/kfileitemmodelrolesupdater.cpp b03fd9c 
>   src/views/versioncontrol/koverlayiconplugin.cpp PRE-CREATION 
>   src/views/versioncontrol/koverlayiconplugin.desktop PRE-CREATION 
>   src/views/versioncontrol/koverlayiconplugin.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125136/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Olivier Goffart
> 
>

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