Review Request 123883: DolphinSearchBox: Add a "More search tools..." menu button

[Emmanuel Pescosta]

> On May 28, 2015, 8:49 p.m., Emmanuel Pescosta wrote:
> > src/search/dolphinsearchbox.cpp, line 389
> > <https://git.reviewboard.kde.org/r/123883/diff/1/?file=370556#file370556line389>
> >
> >     Please remove the lazy loading from here, makes the code harder to read than necessary.
> >     
> >     If you want lazy loading of items (which makes sense :), then please add it to KMoreTools.
> >     
> >     The menu factory could create a specialized QMenu object which loads and inserts the items on the first show event - so all the users of KMoreTools can make use of it and we have a nicer client-side code base :)
> >     
> >     class LazyMenu : QMenu {
> >       onshow {
> >          if (firstShow) load tools and add them to the menu
> >       }
> >     
> >     
> >     KMoreToolsMenuFactory {
> >        QMenu createMenuFromGroupingNames {
> >            return new LazyMenu
> >        }
> >     }
> 
> Gregor Mi wrote:
>     Emmanuel, sorry for the long-delayed reply. I applied your suggestion here de2c0667e3f101911dfbe7a3d933ca2674a45db2 and updated this RR accordingly.
> 
> Gregor Mi wrote:
>     With the latest update I re-added the lazy code because it should be possible to pass the current directory to the search tool. Or do you see a simpler way of doing it?

> Or do you see a simpler way of doing it?

The current solution looks really limited to exactly one use case (tools which can handle exactly one url).
But what if you have other tools which require other information (e.g. current location for Marble, ...), 
or what if your tool can handle multiple urls?! Everytime you have a new option for a tool you have to change 
the api and overload methods.

Please replace it by using so-called data transfer objects or use a key-value map to pass on information for the tools (see QVariantMap or QVariantHash).
For this use case I would choose the key-value map, which has the advantage that you can add new keys without changing the api/abi. The only thing which
you have to do is to properly document all optional/required keys (+ value type) for tools or tool-groups. If not all required keys where given or the
have the wrong data type, don't add the tool to the menu and maybe add a debug info why the tool couldn't be added.


- Emmanuel


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


On Nov. 19, 2015, 9:25 a.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123883/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2015, 9:25 a.m.)
> 
> 
> Review request for Dolphin and Emmanuel Pescosta.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Adds a "More search tools..." button to the DolphinSearchBox.
> 
> Additionally, moves the "More/Fewer options" button from right to left to reduce the mouse travelling distance when the dolphin is maximized on a large screen (see screenshots)
> 
> The current location url will be passed to the search tool to set the initial search root directory.
> 
> 
> Diffs
> -----
> 
>   src/search/dolphinsearchbox.cpp 743d9113a9ae0f7820d1bed594462a208b2883c3 
> 
> Diff: https://git.reviewboard.kde.org/r/123883/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> Currently there is a long mouse traveling distance
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/05/23/8d717a70-215d-4587-89d0-2fee89263731__MoreOptionsTravellingDistance.png
> "More search tools" and the "More/fewer options" buttons moved more to the left
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/05/23/3009c4e7-5666-4377-bf39-5ec1967858e6__MoreSearchTools.png
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

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