D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  I'm not at a Linux machine right now. @renatoo, would you mind investigating 
that in a few non-Dolphin apps' file pickers and open and save dialogs?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Henrik Fehlauer]

rkflx added a comment.


  Before the ball gets rolling, it would be nice if someone familiar with that 
part of the code could comment on what's going on with what I mentioned or can 
reproduce at least (it sounds like a generic problem to me):
  
  > Also some entries show an error message or are broken / show nothing at all 
(while the same entry works fine in Dolphin).
  
  
  
  ---
  
  (My previous comment was not ordered by priority yet, sorry for that…)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  Thanks for that investigation, @rkflx. I aspire to your level of thoroughness.
  
  macOS file pickers have a vertical scrollbar by default to show nearly 
equivalent content, so I don't think that's a huge deal. We could perhaps hide 
some sections by default (as long as it's obvious they're hidden and users have 
an easy and visible way to get them back), and I agree that the Devices section 
is probably more useful to more people than the Search section. We could 
consider re-ordering them.
  
  And you make a good point that some of the search sections aren't relevant to 
all apps. @renatoo seems to have a reasonable idea on that front. @renatoo, it 
would be even lovelier if you could also submit patches for affected apps to 
use the new flag once you've written that part. It's worth mentioning that not 
even macOS does this; the Open dialog for a text editor shows search options 
for pictures, music, and videos, for example. We could leapfrog Apple 
usability-wise here. :)
  
  Do you guys want to open some Bugzilla tickets to track the pieces of this 
work, or are we gonna get going right now?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a comment.


  In https://phabricator.kde.org/D8332#179633, @rkflx wrote:
  
  > This change caused a little bit of fallout for #Gwenview. Apparently the 
review focussed more on the code, but less so on the behaviour in users of the 
class. I'm not complaining, but given one of our focus goals is on usability 
and quality of the basic apps, it would be great if:
  >
  > - changes were tested more broadly in the future in addition to only 
looking at the code
  > - there was some help to fix the fallout
  >
  >   Please head over to https://bugs.kde.org/show_bug.cgi?id=387824 if you 
can help Gwenview, thanks!
  >
  >   ---
  >
  >   In addition to Gwenview I also looked on lxr and did some testing based 
on what I found:
  > - The sidebar in `KDirSelectDialog` is now awful to use, because
  >   - The devices entry (which for some users is more 
important/useful/frequently used than the search entries) is hidden from view 
(bad) and requires scrolling (annoying). → We should discuss reordering or 
(even better) adding collapsing and then collapsing some groups by default.
  >   - The additional scrollbar makes the sidebar so small that you can't read 
most of the entries. → Add splitter and improve default width.
  > - To a lesser extent, this also applies to the normal file dialog (no 
scrollbar by default would be nice).
  > - Filesystem sidebars in https://phabricator.kde.org/tag/kdevelop/, 
#Okteta, https://phabricator.kde.org/tag/kile/, 
https://phabricator.kde.org/tag/kate/ and 
https://phabricator.kde.org/tag/krusader/: Some of the entries do not make 
sense in some of those apps at all, e.g. Videos/Images/…. Also some entries 
show an error message or are broken / show nothing at all (while the same entry 
works fine in Dolphin).
  > - That's it at first sight, luckily ;)
  >
  >   Would be nice to fix those too… Let me know what's the best way forward 
here, i.e. what are Frameworks issues and where we'd need bugs filed against 
individual apps.
  
  
  I agree with you comments some entries in the place model does not make sense 
for some app.
  
  What I suggest is create a new function in the model that could filter the 
entries based on the context of the app.  (Docs, Video, Music, Picture) and you 
can set a flag with any combination of this value.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Henrik Fehlauer]

rkflx added a comment.


  This change caused a little bit of fallout for #Gwenview. Apparently the 
review focussed more on the code, but less so on the behaviour in users of the 
class. I'm not complaining, but given one of our focus goals is on usability 
and quality of the basic apps, it would be great if:
  
  - changes were tested more broadly in the future in addition to only looking 
at the code
  - there was some help to fix the fallout
  
  Please head over to https://bugs.kde.org/show_bug.cgi?id=387824 if you can 
help Gwenview, thanks!
  
  ---
  
  In addition to Gwenview I also looked on lxr and did some testing based on 
what I found:
  
  - The sidebar in `KDirSelectDialog` is now awful to use, because
- The devices entry (which for some users is more 
important/useful/frequently used than the search entries) is hidden from view 
(bad) and requires scrolling (annoying). → We should discuss reordering or 
(even better) adding collapsing and then collapsing some groups by default.
- The additional scrollbar makes the sidebar so small that you can't read 
most of the entries. → Add splitter and improve default width.
  - To a lesser extent, this also applies to the normal file dialog (no 
scrollbar by default would be nice).
  - Filesystem sidebars in https://phabricator.kde.org/tag/kdevelop/, #Okteta, 
https://phabricator.kde.org/tag/kile/, https://phabricator.kde.org/tag/kate/ 
and https://phabricator.kde.org/tag/krusader/: Some of the entries do not make 
sense in some of those apps at all, e.g. Videos/Images/…. Also some entries 
show an error message or are broken / show nothing at all (while the same entry 
works fine in Dolphin).
  - That's it at first sight, luckily ;)
  
  Would be nice to fix those too… Let me know what's the best way forward here, 
i.e. what are generic issues and where we'd need bugs filed against individual 
apps.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:7eb6333bdb48: Added baloo urls into places model 
(authored by Renato Araujo Oliveira Filho renato.ara...@kdab.com, 
committed by ngraham).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=22810=22942

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Kevin Ottens]

ervin accepted this revision.
ervin added inline comments.

INLINE COMMENTS

> renatoo wrote in kfileplacesmodel.cpp:68
> This code came from dolphin. And it does not check at runtime for changes on 
> the configuration.
> 
> But yes I agree keep track of this will be nice, I thought about that while 
> implementing it, but I did not find a way to track it in KConfing API 
> documentation, and since dolphin do not do that, I understand that is not 
> possible.
> 
> Do you know if that is possible? What do you suggest?

const is fine then.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  I'm going to land this on 11/25/17 unless I hear any more objections from 
anyone.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[David Faure]

dfaure accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a comment.


  @dvratil, @ervin any comments?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 22810.
renatoo marked an inline comment as done.
renatoo added a comment.


  Renamed test function

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=22775=22810

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kfileplacesviewtest.cpp:70
> +
> +void KFilePlacesViewTest::testUrlChangedContaisConvertedUrl_data()
> +{

typo: Contains.

I would just have called it `testUrlChanged` because it also checks that it's 
emitted, not just what the emitted value is :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 22775.
renatoo added a comment.


  Updated from master

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=22763=22775

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 22763.
renatoo marked 2 inline comments as done.
renatoo added a comment.


  Updated unit test name

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=22451=22763

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[David Faure]

dfaure accepted this revision.
dfaure added inline comments.

INLINE COMMENTS

> kfileplacesviewtest.cpp:56
> +// Ensure we'll have a clean bookmark file to start
> +QFile::remove(bookmarksFile());
> +

You could just call cleanupTestCase() here, I usually do that too. Cleanup at 
start, cleanup at end.

> kfileplacesviewtest.cpp:102
> +
> +void KFilePlacesViewTest::testConvertUrl()
> +{

Isn't this rather a test for urlChanged()?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a dependent revision: D8943: Create 
'KFilePlacesModel::convertedUrl' static function.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Milian Wolff]

mwolff accepted this revision.
mwolff added a comment.


  lgtm from my side

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure, mwolff
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  @ervin and @dvratil, I'd like to land this in one week, since it's an open 
dependency blocking a lot of great work in Dolphin. Please respond if you have 
any remaining concerns, and preferably update your status if you don't.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a dependent revision: D8862: Extend API.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  @ervin and @dvratil, any remaining concerns?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a dependent revision: D8855: Use Kio::KPlacesModel as source 
model for PlacesItemModel.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 22451.
renatoo added a comment.


  Make 'isFileIndexingEnabled' a static function

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=22328=22451

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Milian Wolff]

mwolff added a comment.


  lgtm, one minor nit, potentially for the future

INLINE COMMENTS

> kfileplacesmodel.cpp:967
>  
> +bool KFilePlacesModel::Private::isFileIndexingEnabled() const
> +{

this could/should be a free function, not a member, considering its result is 
cached in a member variable

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 22328.
renatoo marked 2 inline comments as done.
renatoo added a comment.


  Fallback to initial url if the searchUrl is invalid.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=21932=22328

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> dvratil wrote in kfileplacesmodel.cpp:153
> typo: `withBaloo`

Minor: QLatin1String("true") is enough for a comparison (no need to use a 
16-bit-per-char string)

> kfileplacesview.cpp:1350
> +qWarning() << "Invalid search url:" << url;
> +Q_ASSERT(false);
> +}

So if I, as a user, manually add "search:/doesnotexist" as a bookmark in the 
places view, the application asserts and dies? If I'm right, then this doesn't 
seem wise.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent, dfaure
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  @ervin and @dvratil, any remaining concerns?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 21932.
renatoo marked an inline comment as done.
renatoo added a comment.


  Fixed import

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=21807=21932

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kfileplacesviewtest.cpp:30
> +
> +#include 
> +#include 

Please don't include the full module (which also contains all QtCore)

#include  is OK, #include  is not. Confusing, I know.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  @ervin and @dvratil, any remaining concerns?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Laurent Montel]

mlaurent accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 21807.
renatoo added a comment.


  Added warning message for invalid search:// urls

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=21758=21807

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  @mlaurent would you mind changing your approval status then? :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Ömer Fadıl Usta]

usta added inline comments.

INLINE COMMENTS

> kfileplacesview.cpp:1348
> +searchUrl = searchUrlForType(QStringLiteral("Video"));
> +}
> +

won't be nice to add an else statement? (for the sake of searchUrl )

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Laurent Montel]

mlaurent added a comment.


  +1 for me

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 21758.
renatoo added a comment.


  Fixed code style

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=21647=21758

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked 5 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Laurent Montel]

mlaurent added inline comments.

INLINE COMMENTS

> kfileplacesviewtest.cpp:2
> +/*  This file is part of the KDE project
> +Copyright (C) 2007 Kevin Ottens 
> +

? I don't think that you are kevin ottens :) and we are in 2017 :)

> kfileplacesviewtest.cpp:20
> +
> +#include 
> +#include 

QtCore/ is not necessary

> kfileplacesviewtest.cpp:30
> +
> +#include 
> +#include 

QtTest/ not necessart too

> kfileplacesviewtest.cpp:70
> +}
> +void KFilePlacesViewTest::testConvertUrl_data()
> +{

new line before it

> kfileplacesviewtest.cpp:111
> +
> +QSignalSpy urlChangedSpy(, SIGNAL(urlChanged(QUrl)));
> +const QModelIndex targetIndex = pv.model()->index(row, 0);

use new connect api for QSignalSpy

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Laurent Montel]

mlaurent requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin, mlaurent
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a comment.


  @dvratil and @ervin, any more concerns?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a dependent revision: D8434: Created 'remote' section.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked 4 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 21647.
renatoo marked 2 inline comments as done.
renatoo added a comment.


  Created unittest for PlacesView::convertUrl
  Refactory some small part of the code

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=21583=21647

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kfileplacesmodeltest.cpp
  autotests/kfileplacesviewtest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added inline comments.

INLINE COMMENTS

> ervin wrote in kfileplacesmodel.cpp:68
> Do we really want to keep that state? It's never reevaluated so could be a 
> const if we keep it.
> 
> Asks the question of what happens if the setting changes at runtime though.

This code came from dolphin. And it does not check at runtime for changes on 
the configuration.

But yes I agree keep track of this will be nice, I thought about that while 
implementing it, but I did not find a way to track it in KConfing API 
documentation, and since dolphin do not do that, I understand that is not 
possible.

Do you know if that is possible? What do you suggest?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  All the fiddling with URLs makes me wonder if that wouldn't be better done on 
the KIO implementations side... but that's out of scope for that patch I think.

INLINE COMMENTS

> kfileplacesmodeltest.cpp:98
>  
> +// disable baloo
> +KConfig config(QStringLiteral("baloofilerc"));

Shouldn't we have tests verifying the extra URLs are properly inserted when 
baloo is enabled? Seems like it's trying to avoid testing the behavior change 
coming from the rest of the commit.

> kfileplacesmodel.cpp:68
> +  bookmarkManager(nullptr),
> +  fileIndexingEnabled(isFileIndexingEnabled())
> +{

Do we really want to keep that state? It's never reevaluated so could be a 
const if we keep it.

Asks the question of what happens if the setting changes at runtime though.

> kfileplacesmodel.cpp:499
> +bool allowedHere = appName.isEmpty() || (appName == 
> QCoreApplication::instance()->applicationName());
> +bool allowBalooUrl = isBalooUrl(url) ? fileIndexingEnabled : true;
> +

I find that naming confusing I mean, semantically fileIndexingEnabled would be 
your "allowBalooUrl". Could we come up with something better? Like 
"isSupportedUrl" perhaps? This is what it is about somewhat, we support all 
schemes except for the baloo ones depending on fileIndexingEnabled.

> kfileplacesview.cpp:1301
> +const QString path = url.toDisplayString(QUrl::PreferLocalFile);
> +if (path.endsWith(QLatin1String("yesterday"))) {
> +const QDate date = QDate::currentDate().addDays(-1);

Don't you want to match /yesterday and so on here? Like you're matching 
/documents and so on for the method below.

> kfileplacesview.cpp:1329
> +if (day > 0) {
> +date = date.arg(QString("-%1").arg(day, 2, 10, QChar('0')));
> +} else {

This is unusual, why not concatenate + arg() in that case instead of the nested 
arg calls?

Would allow you to drop the else part too.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 21583.
renatoo added a comment.


  Updated parent branch

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=21227=21583

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Laurent Montel]

mlaurent added a dependent revision: D8367: Hidding place groups implementation 
in KFilePlacesModel.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 21227.
renatoo added a comment.


  Upated parent branch

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20952=21227

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a comment.


  In https://phabricator.kde.org/D8332#158392, @ngraham wrote:
  
  > @dvratil, any remaining objections? This looks great to me.
  >
  > @renatoo, once this is in, can we remove the now-duplicated code from 
Dolphin?
  
  
  Yes the idea is to try to use at least the model on Dolphin.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  @dvratil, any remaining objections? This looks great to me.
  
  @renatoo, once this is in, can we remove the now-duplicated code from Dolphin?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20952.
renatoo added a comment.


  Updated parent branch

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20940=20952

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20940.
renatoo edited the summary of this revision.
renatoo added a comment.


  Updated parent branch

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20918=20940

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added a reviewer: VDG.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo edited the summary of this revision.
renatoo added a dependency: D8243: Implement support for categories on 
KfilesPlacesView.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a dependent revision: D8348: Add a section for removable devices.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked an inline comment as not done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20918.
renatoo added a comment.


  Typo fixed

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20906=20918

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Ömer Fadıl Usta]

usta added inline comments.

INLINE COMMENTS

> kfileplacesmodel.cpp:151
>  
> +// if baloo is enabled, add new ulrs even if the bookmark file is not 
> empty
> +if (d->fileIndexingEnabled &&

typo: ulrs

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo marked 2 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20906.
renatoo added a comment.


  Fixed typos
  Use QStringLiteral when possible

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20905=20906

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20905.
renatoo marked 5 inline comments as done.
renatoo added a comment.


  Fixed typos

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20879=20905

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Laurent Montel]

mlaurent added inline comments.

INLINE COMMENTS

> kfileplacesview.cpp:1303
> +const int day = date.day();
> +timelineUrl = QUrl("timeline:/" + timelineDateString(year, month) +
> +  '/' + timelineDateString(year, month, day));

Add QStringLiteral and QLatin1Char('/') thanks

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: mlaurent, dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfileplacesmodel.cpp:151
>  
> +// if baloo is enabled, add new ulrs even if the bookbark file is not 
> empty
> +if (d->fileIndexingEnabled &&

typo: `bookbark`

> kfileplacesmodel.cpp:153
> +if (d->fileIndexingEnabled &&
> +root.metaDataItem(QStringLiteral("withBaloon")) != 
> QStringLiteral("true")) {
> +

typo: `withBaloo`

> kfileplacesmodel.cpp:155
> +
> +root.setMetaDataItem("withBaloon", "true");
> +KFilePlacesItem::createSystemBookmark(d->bookmarkManager,

typo: `withBaloo`

> kfileplacesview.cpp:1321
> +{
> +QString date = QString::number(year) + '-';
> +if (month < 10) {

You can use the 'fill' feature of `QString::arg()` to achieve the same thing 
with cleaner code:

  QString("%1-%2-%3").arg(year).arg(month, 2, 10, 0).arg(day, 2, 10, 0);

> kfileplacesview.cpp:1344
> +
> +if (path.endsWith(QLatin1String("documents"))) {
> +searchUrl = searchUrlForType(QStringLiteral("Document"));

You should probably match by `/documents` so that you don't match invalid paths 
(`search:/foodocuments`)

> kfileplacesview.cpp:1360
> +{
> +static const QString jsonQuery("{\"dayFilter\": 0,\
> + \"monthFilter\": 0, \

use `QStringLiteral` for `jsonQuery`, then it won't have to be static

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil
Cc: dvratil, ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20879.
renatoo added a comment.


  Does not load baloo urls if file index is disabled

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20873=20879

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo, #frameworks, #dolphin, #kde_applications
Cc: ngraham, #frameworks

D8332: Added baloo urls into places model

[Nathaniel Graham]

ngraham added reviewers: Frameworks, Dolphin, KDE Applications.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo, #frameworks, #dolphin, #kde_applications
Cc: ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo updated this revision to Diff 20873.
renatoo added a comment.


  Removed baloo dependency

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8332?vs=20859=20873

REVISION DETAIL
  https://phabricator.kde.org/D8332

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesview.cpp

To: renatoo
Cc: ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo
Cc: ngraham, #frameworks

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo planned changes to this revision.
renatoo added a comment.


  Remove baloon dependency using KConfig API to query if it is enable, and 
hard-code search url with a fixed string.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8332

To: renatoo
Cc: #frameworks