Review Request 123883: DolphinSearchBox: Add a "More search tools..." menu button
Emmanuel Pescosta
emmanuelpescosta099 at gmail.com
Fri Nov 20 09:40:15 GMT 2015
> 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?
>
> Emmanuel Pescosta wrote:
> > 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.
>
> Gregor Mi wrote:
> KMoreTools currently is based on the KService interface. So without special treatment it will be as capable of URL handling as what is possible with .desktop files.
>
> > The current solution looks really limited to exactly one use case (tools which can handle exactly one url)
>
> The API whould have to be extended only once: to accept a list of URLs instead of a single one. The .desktop placeholder for that is %U.
>
> > But what if you have other tools which require other information (e.g. current location for Marble, ...)
>
> As long as is can be encoded as URL it would work, no?
>
> > Everytime you have a new option for a tool you have to change the api and overload methods.
>
> Not the API would change but the desktop files supplied with KMoreTools.
>
> > Please replace it by using so-called data transfer objects or use a key-value map ... ...
>
> Yes, the current KMoreTools design has its issues and your suggestion sounds reasonable but I am not sure if this is going to be a step too far (in a sense that this would take us away from the KService concepts)?
> KMoreTools currently is based on the KService interface.
Yes but this doesn't mean that KMoreTools can't support anything else than urls.
More and more applications provide DBus interfaces for example, which can be used
to start applications, maybe KMoreTools will support this as well one-day.
> As long as is can be encoded as URL it would work, no?
Yes, but I would prefer a key-value map instead of building complex url queries when
using KMoreTools.
> I am not sure if this is going to be a step too far
It is good practice to use DTOs in such cases, because this makes the whole
thing more flexible.
I thought that KMoreTools should provide the best tools for the current context
and in addition to that, the best possible integration, like scrolling to the
currently visible side in Okular, for example:
You have a simple previewer for images, pdfs, ... Now you open the pdf and scroll down
to page x. Given that the previewer is very limited and has no remark tools,
you want a better tool for it -> KMoreTools. KMoreTools proposes you 'Awesome Okular'
in this context and you select it. Now when Okular has started you would expect that
it scrolls to the page x where you left the previewer. This means that okular has
to be started with "-p <page>".
Pass on the necessary information to KMoreTools via:
The url way: ~/foo.pdf?page=x
The key-value map way:
["url": "~/foo.pdf",
"page: x]
The key-value map will be much easier to use (both on the client and KMoreTools side)
and has some type information due QVariant.
- Emmanuel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123883/#review80935
-----------------------------------------------------------
On Nov. 19, 2015, 12:07 p.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, 12:07 p.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/20151120/ebb59781/attachment.htm>
More information about the kfm-devel
mailing list