Review Request 125514: Add a web shortcut manager

[David Faure]

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


Seems useful to have, I've seen such actions in multiple places, including KHTML and KWebkit (and I know you're coming from kdepim with this).

I'm wondering if the naming couldn't be improved though. This has very little to do with "shortcuts", it only shares the search provider definitions. I'm thinking of KIO::SearchProviderActions, in line with KFileItemActions and KDesktopFileActions.

(or maybe KUriFilterSearchProviderActions, since KUriFilterSearchProvider exists? Although that class is for the plugins, so apps don't really know about it).
I suggest to wait for more input before renaming, let's see what others think.


autotests/webshortcutmenumanagertest.cpp (line 31)
<https://git.reviewboard.kde.org/r/125514/#comment59467>

    ctor and dtor are not useful for test objects
    (if any code needs to be added, initTestCase/cleanupTestCase is better anyway)
    
    => better let the compiler generate the automatic ctor and dtor.



autotests/webshortcutmenumanagertest.cpp (line 62)
<https://git.reviewboard.kde.org/r/125514/#comment59468>

    no way to be more specific about what we expect to see?
    
    If it's all plugins then indeed it's difficult.



src/widgets/CMakeLists.txt (line 141)
<https://git.reviewboard.kde.org/r/125514/#comment59469>

    Shouldn't this be installed as KIO/WebShortcutsMenuManager, to match the classname? In that case, it should be moved to the next list of headers, those that end up under KIO/



src/widgets/webshortcutsmenumanager.h (line 31)
<https://git.reviewboard.kde.org/r/125514/#comment59471>

    s/add/set the/   (adding usually means you can call it multiple times, to add more data)



src/widgets/webshortcutsmenumanager.h (line 40)
<https://git.reviewboard.kde.org/r/125514/#comment59470>

    The class docu should be just above this line,
    and should have @since 5.16



src/widgets/webshortcutsmenumanager.h (line 59)
<https://git.reviewboard.kde.org/r/125514/#comment59472>

    @param selectedText the text to search for



src/widgets/webshortcutsmenumanager.h (line 64)
<https://git.reviewboard.kde.org/r/125514/#comment59473>

    s/WebShortCut action/web shortcut actions/



src/widgets/webshortcutsmenumanager.cpp (line 96)
<https://git.reviewboard.kde.org/r/125514/#comment59474>

    Better declare the var inside the loop (and further down where used), to be more C++ and less C.



src/widgets/webshortcutsmenumanager.cpp (line 109)
<https://git.reviewboard.kde.org/r/125514/#comment59475>

    if (!QStandardPaths::findExecutable("kcmshell5").isEmpty()) {
    
    just in case.


- David Faure


On Oct. 4, 2015, 5:44 a.m., Laurent Montel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> -----------------------------------------------------------
> 
> (Updated Oct. 4, 2015, 5:44 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -----
> 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/webshortcutsmenumanager.h PRE-CREATION 
>   autotests/webshortcutmenumanagertest.cpp PRE-CREATION 
>   src/widgets/webshortcutsmenumanager.cpp PRE-CREATION 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/webshortcutmenumanagertest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125514/diff/
> 
> 
> Testing
> -------
> 
> Tested from long time in kdepim.
> I added autotests for it.
> 
> 
> Thanks,
> 
> Laurent Montel
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151004/050a57ef/attachment-0001.html>