Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

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

(Updated oct. 12, 2015, 11:36 matin)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

When we select text we can provide a search from internet.


Diffs
-

  autotests/CMakeLists.txt 989acd4 
  autotests/kurifiltersearchprovideractionstest.h PRE-CREATION 
  autotests/kurifiltersearchprovideractionstest.cpp PRE-CREATION 
  src/widgets/CMakeLists.txt 9fd2fc0 
  src/widgets/kurifiltersearchprovideractions.h PRE-CREATION 
  src/widgets/kurifiltersearchprovideractions.cpp 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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

> On oct. 12, 2015, 7:02 matin, David Faure wrote:
> > autotests/kurifiltersearchprovideractionstest.cpp, line 51
> > 
> >
> > Well, KIO itself installs a large number of search providers, so this 
> > unittest can rely on them being present. I think this could be a lot more 
> > precise (checking qaction names for instance, like I did in 
> > konqpopupmenutest, or checking qaction texts for substrings, like 
> > knewfilemenutest does).

Ok I will try it.


- Laurent


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


On oct. 10, 2015, 12:56 après-midi, Laurent Montel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> ---
> 
> (Updated oct. 10, 2015, 12:56 après-midi)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/kurifiltersearchprovideractionstest.h PRE-CREATION 
>   autotests/kurifiltersearchprovideractionstest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 9fd2fc0 
>   src/widgets/kurifiltersearchprovideractions.h PRE-CREATION 
>   src/widgets/kurifiltersearchprovideractions.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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/#review86691
---

Ship it!



autotests/kurifiltersearchprovideractionstest.cpp (line 51)


Well, KIO itself installs a large number of search providers, so this 
unittest can rely on them being present. I think this could be a lot more 
precise (checking qaction names for instance, like I did in konqpopupmenutest, 
or checking qaction texts for substrings, like knewfilemenutest does).



src/widgets/CMakeLists.txt (line 162)


trailing whitespace



src/widgets/kurifiltersearchprovideractions.cpp (line 76)


simplified() already takes care of \n and \r, so this can be simplified 
(haha) to

const QString searchText = d->mSelectedText.simplified();


- David Faure


On Oct. 10, 2015, 12:56 p.m., Laurent Montel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> ---
> 
> (Updated Oct. 10, 2015, 12:56 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/kurifiltersearchprovideractionstest.h PRE-CREATION 
>   autotests/kurifiltersearchprovideractionstest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 9fd2fc0 
>   src/widgets/kurifiltersearchprovideractions.h PRE-CREATION 
>   src/widgets/kurifiltersearchprovideractions.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

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


Ping ?:)

- Laurent Montel


On oct. 10, 2015, 12:56 après-midi, Laurent Montel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> ---
> 
> (Updated oct. 10, 2015, 12:56 après-midi)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/kurifiltersearchprovideractionstest.h PRE-CREATION 
>   autotests/kurifiltersearchprovideractionstest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 9fd2fc0 
>   src/widgets/kurifiltersearchprovideractions.h PRE-CREATION 
>   src/widgets/kurifiltersearchprovideractions.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

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

(Updated oct. 10, 2015, 12:56 après-midi)


Review request for KDE Frameworks and David Faure.


Changes
---

Rename class


Repository: kio


Description
---

When we select text we can provide a search from internet.


Diffs (updated)
-

  autotests/CMakeLists.txt 989acd4 
  autotests/kurifiltersearchprovideractionstest.h PRE-CREATION 
  autotests/kurifiltersearchprovideractionstest.cpp PRE-CREATION 
  src/widgets/CMakeLists.txt 9fd2fc0 
  src/widgets/kurifiltersearchprovideractions.h PRE-CREATION 
  src/widgets/kurifiltersearchprovideractions.cpp 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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

> On oct. 7, 2015, 5:27 matin, Laurent Montel wrote:
> > What about rename class ?

No news so I will rename it to KUriFilterSearchProviderActions


- Laurent


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


On oct. 4, 2015, 2:04 après-midi, Laurent Montel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> ---
> 
> (Updated oct. 4, 2015, 2:04 après-midi)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/webshortcutmenumanagertest.h PRE-CREATION 
>   autotests/webshortcutmenumanagertest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/webshortcutsmenumanager.h PRE-CREATION 
>   src/widgets/webshortcutsmenumanager.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

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


What about rename class ?

- Laurent Montel


On oct. 4, 2015, 2:04 après-midi, Laurent Montel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> ---
> 
> (Updated oct. 4, 2015, 2:04 après-midi)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/webshortcutmenumanagertest.h PRE-CREATION 
>   autotests/webshortcutmenumanagertest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/webshortcutsmenumanager.h PRE-CREATION 
>   src/widgets/webshortcutsmenumanager.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

> On oct. 5, 2015, 2:01 après-midi, Eike Hein wrote:
> > Comparing this to the implementation in Konversation/Konsole/Okular might 
> > be interesting.

I looked code and it's similar.
Do you have more info about it ? (missing feature from this widget etc ?)


- Laurent


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


On oct. 4, 2015, 2:04 après-midi, Laurent Montel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125514/
> ---
> 
> (Updated oct. 4, 2015, 2:04 après-midi)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/webshortcutmenumanagertest.h PRE-CREATION 
>   autotests/webshortcutmenumanagertest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/webshortcutsmenumanager.h PRE-CREATION 
>   src/widgets/webshortcutsmenumanager.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Eike Hein]

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


Comparing this to the implementation in Konversation/Konsole/Okular might be 
interesting.

- Eike Hein


On Oct. 4, 2015, 2:04 p.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, 2:04 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When we select text we can provide a search from internet.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 989acd4 
>   autotests/webshortcutmenumanagertest.h PRE-CREATION 
>   autotests/webshortcutmenumanagertest.cpp PRE-CREATION 
>   src/widgets/CMakeLists.txt 820cd34 
>   src/widgets/webshortcutsmenumanager.h PRE-CREATION 
>   src/widgets/webshortcutsmenumanager.cpp 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

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

(Updated oct. 4, 2015, 2:04 après-midi)


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

When we select text we can provide a search from internet.


Diffs (updated)
-

  autotests/CMakeLists.txt 989acd4 
  autotests/webshortcutmenumanagertest.h PRE-CREATION 
  autotests/webshortcutmenumanagertest.cpp PRE-CREATION 
  src/widgets/CMakeLists.txt 820cd34 
  src/widgets/webshortcutsmenumanager.h PRE-CREATION 
  src/widgets/webshortcutsmenumanager.cpp 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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125514: Add a web shortcut manager

[Laurent Montel]

> On oct. 4, 2015, 11:52 matin, David Faure wrote:
> > 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.

Ok I wait but KUriFilterSearchProviderActions seems good as name


> On oct. 4, 2015, 11:52 matin, David Faure wrote:
> > autotests/webshortcutmenumanagertest.cpp, line 62
> > 
> >
> > no way to be more specific about what we expect to see?
> > 
> > If it's all plugins then indeed it's difficult.

It's specific about config. so we can't be sure that we have specific plugin.
So I verify just that menu is not empty => it generates a correct menu.


> On oct. 4, 2015, 11:52 matin, David Faure wrote:
> > src/widgets/CMakeLists.txt, line 141
> > 
> >
> > 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/

indeed I will fix it.


- Laurent


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


On oct. 4, 2015, 5:44 matin, 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 matin)
> 
> 
> 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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)


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)


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)


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)


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



src/widgets/webshortcutsmenumanager.h (line 40)


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



src/widgets/webshortcutsmenumanager.h (line 59)


@param selectedText the text to search for



src/widgets/webshortcutsmenumanager.h (line 64)


s/WebShortCut action/web shortcut actions/



src/widgets/webshortcutsmenumanager.cpp (line 96)


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)


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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 125514: Add a web shortcut manager

[Laurent Montel]

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

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel