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

[Emmanuel Pescosta]

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


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)


src/kitemviews/kfileitemmodelrolesupdater.cpp (lines 133 - 145)
<https://git.reviewboard.kde.org/r/125136/#comment58874>

    Suggestion: Maybe put this into a new "OverlayIconsService" (static instance), which instantiates all the available plugins (only once) and encapsulates all the plugin handling.
    
    connect(OverlayIconService::instance(), &OverlayIconService::overlaysChanged, this, &KFileItemModelRolesUpdater::slotOverlaysChanged);



src/kitemviews/kfileitemmodelrolesupdater.cpp (lines 1083 - 1085)
<https://git.reviewboard.kde.org/r/125136/#comment58875>

    Suggestion: Also something for the "OverlayIconService" ;)
    
    OverlayIconService::instance()->getOverlays(item)



src/kitemviews/kfileitemmodelrolesupdater.cpp (line 1099)
<https://git.reviewboard.kde.org/r/125136/#comment58870>

    const



src/kitemviews/kfileitemmodelrolesupdater.cpp (line 1100)
<https://git.reviewboard.kde.org/r/125136/#comment58869>

    coding style: always use brackets



src/kitemviews/kfileitemmodelrolesupdater.cpp (line 1102)
<https://git.reviewboard.kde.org/r/125136/#comment58871>

    const



src/kitemviews/kfileitemmodelrolesupdater.cpp (line 1103)
<https://git.reviewboard.kde.org/r/125136/#comment58872>

    coding style: remove space between QHash and <



src/kitemviews/kfileitemmodelrolesupdater.cpp (lines 1104 - 1107)
<https://git.reviewboard.kde.org/r/125136/#comment58873>

    overlays_new.count >= overlay_old.count after each slotOverlaysChanged call -> the list of overlays will grow indefinitely!



src/views/versioncontrol/koverlayiconplugin.cpp (line 21)
<https://git.reviewboard.kde.org/r/125136/#comment58864>

    coding style: initializer list should go into a new line; QObject *parent



src/views/versioncontrol/koverlayiconplugin.h (line 26)
<https://git.reviewboard.kde.org/r/125136/#comment58868>

    QUrl :)



src/views/versioncontrol/koverlayiconplugin.h (line 37)
<https://git.reviewboard.kde.org/r/125136/#comment58867>

    coding style: { should go into a new line



src/views/versioncontrol/koverlayiconplugin.h (line 39)
<https://git.reviewboard.kde.org/r/125136/#comment58865>

    unused?



src/views/versioncontrol/koverlayiconplugin.h (line 41)
<https://git.reviewboard.kde.org/r/125136/#comment58866>

    nullptr



src/views/versioncontrol/koverlayiconplugin.h (line 48)
<https://git.reviewboard.kde.org/r/125136/#comment58876>

    Do we really need a KFileItem? Maybe we can use QUrl instead? (would it make more consistent with the overlaysChanged signal)


- Emmanuel Pescosta


On Sept. 10, 2015, 1:43 p.m., Olivier Goffart wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125136/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2015, 1:43 p.m.)
> 
> 
> 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/20150910/8e82e531/attachment.htm>