Review Request 122911: dolphin: SpaceInfoToolsMenu: introduce KMoreTools

[Emmanuel Pescosta]

> On April 17, 2015, 10:19 a.m., Emmanuel Pescosta wrote:
> > src/statusbar/spaceinfotoolsmenu.h, line 45
> > <https://git.reviewboard.kde.org/r/122911/diff/5/?file=359838#file359838line45>
> >
> >     Maybe make this class a menu factory?
> 
> Gregor Mi wrote:
>     What would be the advantage? (i.e. I don't know exactly what is meant)
> 
> Emmanuel Pescosta wrote:
>     Factory pattern http://www.oodesign.com/factory-pattern.html
>     
>     The advantages are:
>     * Hides away the instantiation logic from the client
>     * Avoids client side changes when things in KMoreTools are changed/extended
>     * You can move it into KMoreTools -> We only need 3 loc in Dolphin and other applications can make use of it too
>     
>     KMoreToolsMenuFactory factory;
>     QMenu menu = factory.create({"disk-usage", "disk-partitions"}, url);
>     menu.exec(QCursor::pos());
> 
> Gregor Mi wrote:
>     Ah, I know the generic pattern. :-) I thought you meant something QT/KDE specific. I like the code you proposed. Although, it is maybe a bit too generic here: we would lose the possibility to distinguish between different urls for filelight (current directory, current volume and all), would'nt we?
>     
>     Take also a look here: https://git.reviewboard.kde.org/r/122374/diff/12/ (git menu). There is also a special case ("View history") which makes the factory approach not quite straightforward to implement.
>     
>     However, in context of alternative applications suggestions (which I plan to do next with KMoreTools), the factory would fit very good.
> 
> Gregor Mi wrote:
>     Cancel my concerns... :) You probably meant to move pretty much the whole code of SpaceInfoToolsMenu::buildMenu() to KMoreTools. I'll try that and come back with an update.

> move pretty much the whole code

Exactly :)

> There is also a special case ("View history") which makes the factory approach

You can also pass on a url and do the magic inside create.


- Emmanuel


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


On April 18, 2015, 12:34 a.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122911/
> -----------------------------------------------------------
> 
> (Updated April 18, 2015, 12:34 a.m.)
> 
> 
> Review request for Dolphin and Emmanuel Pescosta.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Moved from old dolphin repo (kde-baseapps, https://git.reviewboard.kde.org/r/122352/)
> 
> 
> Diffs
> -----
> 
>   src/statusbar/statusbarspaceinfo.cpp 127641e601d1468479ef925f9539ecd31dc12fb7 
>   src/statusbar/statusbarspaceinfo.h 326c419ab3dc709f5bc4ab23f73e727d1106b3a4 
>   src/statusbar/spaceinfotoolsmenu.cpp 40ca56e19ebec33518bae0c94dcb56318b473290 
>   src/statusbar/spaceinfotoolsmenu.h 3ca2e184f03b31d10d661c5a931644485ffc46e6 
> 
> Diff: https://git.reviewboard.kde.org/r/122911/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> less entries by default than before
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/77cb7101-49ae-4812-9958-373cc3961a73__less_entries_by_default_than_before.png
> more entries in submenu
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/64a2f205-2db1-4f39-a252-74832f8bb196__more_entries_in_submenu.png
> configure menu dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/e0a46310-7090-4d95-a0c9-198d5783a5eb__configure_menu_dialog.png
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

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