Re: Review Request 114253: Port favicons dataengine to KF5

[Andrea Scarpino]

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

(Updated Dec. 3, 2013, 10:53 a.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

Diff updated.


Repository: kde-workspace


Description
---

= subject.

I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Please check if I 
replaced them correctly.


Diffs (updated)
-

  plasma/generic/dataengines/CMakeLists.txt 3e94325 
  plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
  plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
  plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
  plasma/generic/dataengines/favicons/favicons.h 79565bf 
  plasma/generic/dataengines/favicons/favicons.cpp 2eae673 

Diff: http://git.reviewboard.kde.org/r/114253/diff/


Testing
---

Builds.


Thanks,

Andrea Scarpino

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114253/#review45147
---



plasma/generic/dataengines/favicons/faviconprovider.h


why is this needed now, if it wasn't needed before?



plasma/generic/dataengines/favicons/faviconprovider.h


This one is unnecessary for sure.
class KJob; would be enough.



plasma/generic/dataengines/favicons/faviconprovider.cpp


Are you sure that "url" is always a full URL here, and never a local path? 
KUrl(QString) could deal with local paths, QUrl(QString) can't. It would be 
much better to change the constructor to take a QUrl argument instead, for type 
safety.



plasma/generic/dataengines/favicons/faviconprovider.cpp


missing '/' before favicons ->  make it "/favicons/"



plasma/generic/dataengines/favicons/faviconprovider.cpp


This line is a noop. fromLocalFile is a static method.
I think setPath was actually correct here, you're changing the path of an 
existing full URL.


- David Faure


On Dec. 3, 2013, 9:53 a.m., Andrea Scarpino wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114253/
> ---
> 
> (Updated Dec. 3, 2013, 9:53 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> = subject.
> 
> I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Please check if 
> I replaced them correctly.
> 
> 
> Diffs
> -
> 
>   plasma/generic/dataengines/CMakeLists.txt 3e94325 
>   plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
>   plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
>   plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
>   plasma/generic/dataengines/favicons/favicons.h 79565bf 
>   plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
> 
> Diff: http://git.reviewboard.kde.org/r/114253/diff/
> 
> 
> Testing
> ---
> 
> Builds.
> 
> 
> Thanks,
> 
> Andrea Scarpino
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[Andrea Scarpino]

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

(Updated Dec. 5, 2013, 11:48 a.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

David's fixes.


Repository: kde-workspace


Description
---

= subject.

I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Please check if I 
replaced them correctly.


Diffs (updated)
-

  plasma/generic/dataengines/CMakeLists.txt 3e94325 
  plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
  plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
  plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
  plasma/generic/dataengines/favicons/favicons.h 79565bf 
  plasma/generic/dataengines/favicons/favicons.cpp 2eae673 

Diff: http://git.reviewboard.kde.org/r/114253/diff/


Testing
---

Builds.


Thanks,

Andrea Scarpino

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114253/#review45174
---



plasma/generic/dataengines/favicons/favicons.cpp


Hmm, this has simply moved the problem here.

But now I see: this "identifier" actually comes from FaviconProvider in the 
first place, so it has full control over that string?

In that case, having the conversion to QUrl should indeed be closer to the 
conversion from QUrl to QString, i.e. inside Provider, as it was originally.

But I'm missing something, this is a chicken an egg situation. Who creates  
this identifier initially, before the Provider is created in the first place?

In other words, who calls sourceRequestEvent?
Sorry I don't know anything about data engines - but I'm smelling a 
possible QUrl misuse somewhere here.




plasma/generic/dataengines/favicons/favicons.cpp


unrelated, but this should most certainly be .isNull() instead of creating 
a temporary QImage just for comparison purposes.


- David Faure


On Dec. 5, 2013, 10:48 a.m., Andrea Scarpino wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114253/
> ---
> 
> (Updated Dec. 5, 2013, 10:48 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> = subject.
> 
> I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Please check if 
> I replaced them correctly.
> 
> 
> Diffs
> -
> 
>   plasma/generic/dataengines/CMakeLists.txt 3e94325 
>   plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
>   plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
>   plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
>   plasma/generic/dataengines/favicons/favicons.h 79565bf 
>   plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
> 
> Diff: http://git.reviewboard.kde.org/r/114253/diff/
> 
> 
> Testing
> ---
> 
> Builds.
> 
> 
> Thanks,
> 
> Andrea Scarpino
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[Andrea Scarpino]

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

(Updated Dec. 5, 2013, 7:01 p.m.)


Review request for Plasma.


Changes
---

Use QString instead of QUrl for the identifier so we have two different 
identifiers for different urls for the same host (e.g. http://kde.org and 
kde.org). The current engine does this way.


Repository: kde-workspace


Description (updated)
---

I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Also I cleanup 
includes.

The url given in input could be absolute or relative but not local, then I used 
QUrl::fromUserInput().


Diffs (updated)
-

  plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
  plasma/generic/dataengines/favicons/favicons.h 79565bf 
  plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
  plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
  plasma/generic/dataengines/CMakeLists.txt 3e94325 
  plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 

Diff: http://git.reviewboard.kde.org/r/114253/diff/


Testing (updated)
---

If I try to fetch several favicons an empty image is returned, except the first 
one which contains a valid favicon.


Thanks,

Andrea Scarpino

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114253/#review45222
---

Ship it!


Ship It!

- David Faure


On Dec. 5, 2013, 6:01 p.m., Andrea Scarpino wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114253/
> ---
> 
> (Updated Dec. 5, 2013, 6:01 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Also I cleanup 
> includes.
> 
> The url given in input could be absolute or relative but not local, then I 
> used QUrl::fromUserInput().
> 
> 
> Diffs
> -
> 
>   plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
>   plasma/generic/dataengines/favicons/favicons.h 79565bf 
>   plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
>   plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
>   plasma/generic/dataengines/CMakeLists.txt 3e94325 
>   plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
> 
> Diff: http://git.reviewboard.kde.org/r/114253/diff/
> 
> 
> Testing
> ---
> 
> If I try to fetch several favicons an empty image is returned, except the 
> first one which contains a valid favicon.
> 
> 
> Thanks,
> 
> Andrea Scarpino
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[Andrea Scarpino]

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

(Updated Dec. 5, 2013, 7:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Repository: kde-workspace


Description
---

I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Also I cleanup 
includes.

The url given in input could be absolute or relative but not local, then I used 
QUrl::fromUserInput().


Diffs
-

  plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
  plasma/generic/dataengines/favicons/favicons.h 79565bf 
  plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
  plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
  plasma/generic/dataengines/CMakeLists.txt 3e94325 
  plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 

Diff: http://git.reviewboard.kde.org/r/114253/diff/


Testing
---

If I try to fetch several favicons an empty image is returned, except the first 
one which contains a valid favicon.


Thanks,

Andrea Scarpino

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 114253: Port favicons dataengine to KF5

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114253/#review45225
---


This review has been submitted with commit 
5bf3928ec160f92dcfc1ad70fd9832e4f03594d3 by Andrea Scarpino to branch master.

- Commit Hook


On Dec. 5, 2013, 6:01 p.m., Andrea Scarpino wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114253/
> ---
> 
> (Updated Dec. 5, 2013, 6:01 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> I replaced KUrl by QUrl and KStandardDirs by QStandardPaths. Also I cleanup 
> includes.
> 
> The url given in input could be absolute or relative but not local, then I 
> used QUrl::fromUserInput().
> 
> 
> Diffs
> -
> 
>   plasma/generic/dataengines/favicons/favicons.cpp 2eae673 
>   plasma/generic/dataengines/favicons/favicons.h 79565bf 
>   plasma/generic/dataengines/favicons/faviconprovider.cpp bc72a66 
>   plasma/generic/dataengines/favicons/faviconprovider.h 1f370f9 
>   plasma/generic/dataengines/CMakeLists.txt 3e94325 
>   plasma/generic/dataengines/favicons/CMakeLists.txt 4af0f14 
> 
> Diff: http://git.reviewboard.kde.org/r/114253/diff/
> 
> 
> Testing
> ---
> 
> If I try to fetch several favicons an empty image is returned, except the 
> first one which contains a valid favicon.
> 
> 
> Thanks,
> 
> Andrea Scarpino
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel