Review Request 125675: Use the new KOverlayIconPlugin interface from KIO::Widgets

[Emmanuel Pescosta]

> On Oct. 19, 2015, 3:46 p.m., Emmanuel Pescosta wrote:
> > src/kitemviews/kfileitemmodelrolesupdater.cpp, line 52
> > <https://git.reviewboard.kde.org/r/125675/diff/2/?file=411729#file411729line52>
> >
> >     Please remove this version check and increase the min. KF version to 5.16 instead. Such checks only lead to problems and increase the amount of testing dramatically, so only add them if they are unavoidable. (KF 5.16 will be released before the beta release of Applications 15.12)
> 
> Olivier Goffart wrote:
>     Fine with me.  this just means that dolphin from git will need an unreleased version of kf5.  Is that acceptable?

> Is that acceptable?

Yes ;)


> On Oct. 19, 2015, 3:46 p.m., Emmanuel Pescosta wrote:
> > src/kitemviews/kfileitemmodelrolesupdater.cpp, lines 140-150
> > <https://git.reviewboard.kde.org/r/125675/diff/2/?file=411729#file411729line140>
> >
> >     IMHO this and all the foreach overlay plugin loops should also go into KIO hidden behind
> >     a nice service api (getOverlays and overlaysChanged signal). Clients are usually not interested where the overlays come from and where the necessary plugins are located ...
> >     
> >     This avoids duplicated code when other clients want to make use of the overlay infrastructure and makes it easier to extend in future.
> 
> Olivier Goffart wrote:
>     This just converts a QList<QObject*> to a QList<KOverlayIconPlugin*>.   (maybe KPluginLoader::instentiatePlugin could take a template parametter to do that?)
>     But it also connects signals to dolphin itself

> maybe KPluginLoader::instentiatePlugin could take a template parametter to do that?

Good idea :)

But I meant more the hidding of implementation details of the whole overlay functionality:

OverlayService {
  get()
  signal changed()
}

The client has one method to get overlays and one changed signal to connect with, completely irrelevant
if the overlays come from plugins or anywhere else. So we can easily extend it in future (e.g. implementing
enabling/disabling of plugins, ...) without adjusting all users of the overlay infrastructure (only Dolphin atm
but I think it would also be interesting for the folder view applet ;) ).


- Emmanuel


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


On Oct. 19, 2015, 1:14 p.m., Olivier Goffart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125675/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2015, 1:14 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Depends on, https://git.reviewboard.kde.org/r/125436
> 
> Replaces https://git.reviewboard.kde.org/r/125136/
> 
> 
> Diffs
> -----
> 
>   src/kitemviews/kfileitemmodelrolesupdater.cpp 14e3015 
>   src/CMakeLists.txt 413260b 
>   src/kitemviews/kfileitemmodelrolesupdater.h 6e7559f 
> 
> Diff: https://git.reviewboard.kde.org/r/125675/diff/
> 
> 
> Testing
> -------
> 
> Tested with owncloud plugin.
> 
> 
> Thanks,
> 
> Olivier Goffart
> 
>

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