Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

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

(Updated Sept. 8, 2015, 10:03 p.m.)


Status
--

This change has been discarded.


Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh Handa.


Repository: plasma-desktop


Description
---

Added the functionality to add directories to be indexed.
Did some cleaning up.

Screenshot of the new look: http://i.imgur.com/RBWxutv.png


Diffs
-

  kcms/baloo/CMakeLists.txt 7415289 
  kcms/baloo/configwidget.ui 512e4a5 
  kcms/baloo/folderselectionwidget.h 226ab45 
  kcms/baloo/folderselectionwidget.cpp b44d111 
  kcms/baloo/kcm.h 6ff5813 
  kcms/baloo/kcm.cpp 27d93e2 

Diff: https://git.reviewboard.kde.org/r/125058/diff/


Testing
---

Tested add, remove include and exclude directories.


Thanks,

Ovidiu-Florin BOGDAN

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

> On Sept. 6, 2015, 5:22 a.m., Thomas Pfeiffer wrote:
> > Have you thought of the corner cases?
> > These come to my mind:
> > - Is it possible to add the same folder to both lists? If so: Which one 
> > wins?
> > - It ist possible to add a folder to index within an excluded folder? If 
> > so: what happens?
> > - Is it possible to exclude a folder which is _not_ within an included 
> > folder (not strictly a problem as it would simply have no effect, but 
> > strange nonetheless)?

You are right. I didn't consider those scenarios.
1. In this case, the decision is up to baloo, but I'll look into implementing a 
failsafe to not alow this.
2. Again, the question is: if baloo accepts this. I don't see why this 
shouldn't be allowed. Currently there is nothing in the KCM stoping this to 
happen.
3. Yes, it is. Maybe someone is really paranoia, and they whant to make sure 
that that directory does not get indexexd under any circumstances.

Another scenario would be if a directory is added to be excluded, but there are 
currently child directories of that directory that are included. In this case, 
I'd show a warning, but let the user do it.


P.S. Please take a look over the Review Requests that this one depends on. I 
can't submit the other patches untill these are in the repo. They depend on 
these.


- Ovidiu-Florin


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


On Sept. 5, 2015, 11:11 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 11:11 p.m.)
> 
> 
> Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh 
> Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> Screenshot of the new look: http://i.imgur.com/RBWxutv.png
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

> On Sept. 6, 2015, 2:22 a.m., Thomas Pfeiffer wrote:
> > Have you thought of the corner cases?
> > These come to my mind:
> > - Is it possible to add the same folder to both lists? If so: Which one 
> > wins?
> > - It ist possible to add a folder to index within an excluded folder? If 
> > so: what happens?
> > - Is it possible to exclude a folder which is _not_ within an included 
> > folder (not strictly a problem as it would simply have no effect, but 
> > strange nonetheless)?
> 
> Ovidiu-Florin BOGDAN wrote:
> You are right. I didn't consider those scenarios.
> 1. In this case, the decision is up to baloo, but I'll look into 
> implementing a failsafe to not alow this.
> 2. Again, the question is: if baloo accepts this. I don't see why this 
> shouldn't be allowed. Currently there is nothing in the KCM stoping this to 
> happen.
> 3. Yes, it is. Maybe someone is really paranoia, and they whant to make 
> sure that that directory does not get indexexd under any circumstances.
> 
> Another scenario would be if a directory is added to be excluded, but 
> there are currently child directories of that directory that are included. In 
> this case, I'd show a warning, but let the user do it.
> 
> 
> P.S. Please take a look over the Review Requests that this one depends 
> on. I can't submit the other patches untill these are in the repo. They 
> depend on these.
> 
> Vishesh Handa wrote:
> What review requests?

Found them. Sorry for the noise. Could you please add the Baloo group / or me 
next time. Plasma has too much traffic for me to monitor.


- Vishesh


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


On Sept. 5, 2015, 8:11 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 8:11 p.m.)
> 
> 
> Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh 
> Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> Screenshot of the new look: http://i.imgur.com/RBWxutv.png
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

> On Sept. 6, 2015, 2:22 a.m., Thomas Pfeiffer wrote:
> > Have you thought of the corner cases?
> > These come to my mind:
> > - Is it possible to add the same folder to both lists? If so: Which one 
> > wins?
> > - It ist possible to add a folder to index within an excluded folder? If 
> > so: what happens?
> > - Is it possible to exclude a folder which is _not_ within an included 
> > folder (not strictly a problem as it would simply have no effect, but 
> > strange nonetheless)?
> 
> Ovidiu-Florin BOGDAN wrote:
> You are right. I didn't consider those scenarios.
> 1. In this case, the decision is up to baloo, but I'll look into 
> implementing a failsafe to not alow this.
> 2. Again, the question is: if baloo accepts this. I don't see why this 
> shouldn't be allowed. Currently there is nothing in the KCM stoping this to 
> happen.
> 3. Yes, it is. Maybe someone is really paranoia, and they whant to make 
> sure that that directory does not get indexexd under any circumstances.
> 
> Another scenario would be if a directory is added to be excluded, but 
> there are currently child directories of that directory that are included. In 
> this case, I'd show a warning, but let the user do it.
> 
> 
> P.S. Please take a look over the Review Requests that this one depends 
> on. I can't submit the other patches untill these are in the repo. They 
> depend on these.

What review requests?


- Vishesh


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


On Sept. 5, 2015, 8:11 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 8:11 p.m.)
> 
> 
> Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh 
> Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> Screenshot of the new look: http://i.imgur.com/RBWxutv.png
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

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

Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.


Repository: plasma-desktop


Description
---

Added the functionality to add directories to be indexed.
Did some cleaning up.


Diffs
-

  kcms/baloo/CMakeLists.txt 7415289 
  kcms/baloo/configwidget.ui 512e4a5 
  kcms/baloo/folderselectionwidget.h 226ab45 
  kcms/baloo/folderselectionwidget.cpp b44d111 
  kcms/baloo/kcm.h 6ff5813 
  kcms/baloo/kcm.cpp 27d93e2 

Diff: https://git.reviewboard.kde.org/r/125058/diff/


Testing
---

Tested add, remove include and exclude directories.


Thanks,

Ovidiu-Florin BOGDAN

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

> On Sept. 5, 2015, 12:12 p.m., Vishesh Handa wrote:
> > Can you please attach some images of how the KCM now looks?
> 
> Ovidiu-Florin BOGDAN wrote:
> Should I make a new review request with each patch? Or a single one with 
> 3 diffs?

A new review request per patch, please.


- Vishesh


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


On Sept. 5, 2015, 11:33 a.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 11:33 a.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

> On Sept. 5, 2015, 12:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/configwidget.ui, line 43
> > 
> >
> > Moving the checkBox from the top to bottom? Why? Also, if we want this 
> > it goes in a seperate patch.
> 
> Ovidiu-Florin BOGDAN wrote:
> I moved it from the bottom to the top.
> 
> Vishesh Handa wrote:
> Right, bottom to top. Why?
> 
> Ovidiu-Florin BOGDAN wrote:
> It seems to me more logical this way. Here's the screenshot: 
> http://i.imgur.com/RBWxutv.png
> 
> Ovidiu-Florin BOGDAN wrote:
> Separated into a different patch.

I'm questioning this new UI quite a bit. Adding both 'include' and 'exclude' 
folders is interesting, but I'm not sure if the current approach of having 2 
widgets is the best.


- Vishesh


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


On Sept. 5, 2015, 11:33 a.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 11:33 a.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

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

(Updated Sept. 5, 2015, 10:53 p.m.)


Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh Handa.


Repository: plasma-desktop


Description (updated)
---

Added the functionality to add directories to be indexed.
Did some cleaning up.

Screenshot of the new look: http://i.imgur.com/RBWxutv.png


Diffs
-

  kcms/baloo/CMakeLists.txt 7415289 
  kcms/baloo/configwidget.ui 512e4a5 
  kcms/baloo/folderselectionwidget.h 226ab45 
  kcms/baloo/folderselectionwidget.cpp b44d111 
  kcms/baloo/kcm.h 6ff5813 
  kcms/baloo/kcm.cpp 27d93e2 

Diff: https://git.reviewboard.kde.org/r/125058/diff/


Testing
---

Tested add, remove include and exclude directories.


Thanks,

Ovidiu-Florin BOGDAN

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

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

(Updated Sept. 5, 2015, 10:53 p.m.)


Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh Handa.


Repository: plasma-desktop


Description (updated)
---

Added the functionality to add directories to be indexed.
Did some cleaning up.

Screenshot of the new look.


Diffs
-

  kcms/baloo/CMakeLists.txt 7415289 
  kcms/baloo/configwidget.ui 512e4a5 
  kcms/baloo/folderselectionwidget.h 226ab45 
  kcms/baloo/folderselectionwidget.cpp b44d111 
  kcms/baloo/kcm.h 6ff5813 
  kcms/baloo/kcm.cpp 27d93e2 

Diff: https://git.reviewboard.kde.org/r/125058/diff/


Testing
---

Tested add, remove include and exclude directories.


Thanks,

Ovidiu-Florin BOGDAN

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

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

(Updated Sept. 5, 2015, 11:11 p.m.)


Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh Handa.


Changes
---

Please aprove these, so I can add the rest of the patches.


Repository: plasma-desktop


Description
---

Added the functionality to add directories to be indexed.
Did some cleaning up.

Screenshot of the new look: http://i.imgur.com/RBWxutv.png


Diffs
-

  kcms/baloo/CMakeLists.txt 7415289 
  kcms/baloo/configwidget.ui 512e4a5 
  kcms/baloo/folderselectionwidget.h 226ab45 
  kcms/baloo/folderselectionwidget.cpp b44d111 
  kcms/baloo/kcm.h 6ff5813 
  kcms/baloo/kcm.cpp 27d93e2 

Diff: https://git.reviewboard.kde.org/r/125058/diff/


Testing
---

Tested add, remove include and exclude directories.


Thanks,

Ovidiu-Florin BOGDAN

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/folderselectionwidget.cpp, line 306
> > 
> >
> > Coding style. The { should not be on the next line. Please fix all 
> > instances of this.

This is KDE (maybe Qt) specific. I wrote it like this due to force of habbit. 
I'll fix them all, including the ones I didn't write.


> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/CMakeLists.txt, line 20
> > 
> >
> > You seem to be using 'QStorageInfo' instead of Solid. Why?
> > 
> > Also, if we decide that is what we want to do, it should in a separate 
> > patch. This patch is large and should be split.

It is simpler to get the desired information with QStorageInfo, and it is less 
error prone. The risk for errors is smaller.

Also Solid is another dependency. Qt is already there.


> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/configwidget.ui, line 43
> > 
> >
> > Moving the checkBox from the top to bottom? Why? Also, if we want this 
> > it goes in a seperate patch.
> 
> Ovidiu-Florin BOGDAN wrote:
> I moved it from the bottom to the top.
> 
> Vishesh Handa wrote:
> Right, bottom to top. Why?

It seems to me more logical this way. Here's the screenshot: 
http://i.imgur.com/RBWxutv.png


- Ovidiu-Florin


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


On Sept. 5, 2015, 2:33 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 2:33 p.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Luca Beltrame]

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


Please add the usability group to review UI changes.

- Luca Beltrame


On Set. 5, 2015, 11:33 a.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Set. 5, 2015, 11:33 a.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Martin Klapetek]

> On Sept. 5, 2015, 8:49 p.m., Vishesh Handa wrote:
> > kcms/baloo/folderselectionwidget.cpp, line 373
> > 
> >
> > `QStringLiteral`
> 
> Ovidiu-Florin BOGDAN wrote:
> I didn't change that part.
> 
> Why QStringLiteral instead of QLatin1String?

https://blog.qt.io/blog/2014/06/13/qt-weekly-13-qstringliteral/ and 
http://woboq.com/blog/qstringliteral.html


- Martin


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


On Sept. 5, 2015, 9:18 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 9:18 p.m.)
> 
> 
> Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh 
> Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

> On Sept. 5, 2015, 12:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/configwidget.ui, line 43
> > 
> >
> > Moving the checkBox from the top to bottom? Why? Also, if we want this 
> > it goes in a seperate patch.
> 
> Ovidiu-Florin BOGDAN wrote:
> I moved it from the bottom to the top.

Right, bottom to top. Why?


> On Sept. 5, 2015, 12:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/folderselectionwidget.cpp, line 130
> > 
> >
> > Is this change required?
> 
> Ovidiu-Florin BOGDAN wrote:
> The name is more descriptive like this. It is less likely to cause 
> confusion.

It is only used in one place, which is **one line** below this.

Anyway, if you still want to commit this, it goes in a seperate patch.


- Vishesh


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


On Sept. 5, 2015, 11:33 a.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 11:33 a.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

> On Sept. 5, 2015, 9:49 p.m., Vishesh Handa wrote:
> > kcms/baloo/folderselectionwidget.cpp, line 373
> > 
> >
> > `QStringLiteral`

I didn't change that part.

Why QStringLiteral instead of QLatin1String?


- Ovidiu-Florin


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


On Sept. 5, 2015, 2:33 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 2:33 p.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

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



kcms/baloo/folderselectionwidget.cpp (line 273)


const QStorageInfo& si



kcms/baloo/folderselectionwidget.cpp (line 280)


Maybe 'startsWith' instead of contains?



kcms/baloo/folderselectionwidget.cpp (line 282)


`QStringLiteral`


- Vishesh Handa


On Sept. 5, 2015, 11:33 a.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 11:33 a.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Vishesh Handa]

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


Can you please attach some images of how the KCM now looks?


kcms/baloo/CMakeLists.txt 


You seem to be using 'QStorageInfo' instead of Solid. Why?

Also, if we decide that is what we want to do, it should in a separate 
patch. This patch is large and should be split.



kcms/baloo/configwidget.ui (line 43)


Moving the checkBox from the top to bottom? Why? Also, if we want this it 
goes in a seperate patch.



kcms/baloo/folderselectionwidget.cpp (line 97)


Is this change required?



kcms/baloo/folderselectionwidget.cpp (line 132)


In the Qt coding style, getters do not have the word 'get'



kcms/baloo/folderselectionwidget.cpp (line 238)


Coding style. The { should not be on the next line. Please fix all 
instances of this.


- Vishesh Handa


On Sept. 5, 2015, 11:33 a.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 11:33 a.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/configwidget.ui, line 43
> > 
> >
> > Moving the checkBox from the top to bottom? Why? Also, if we want this 
> > it goes in a seperate patch.

I moved it from the bottom to the top.


> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/folderselectionwidget.cpp, line 130
> > 
> >
> > Is this change required?

The name is more descriptive like this. It is less likely to cause confusion.


- Ovidiu-Florin


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


On Sept. 5, 2015, 2:33 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 2:33 p.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Thomas Pfeiffer]

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


Have you thought of the corner cases?
These come to my mind:
- Is it possible to add the same folder to both lists? If so: Which one wins?
- It ist possible to add a folder to index within an excluded folder? If so: 
what happens?
- Is it possible to exclude a folder which is _not_ within an included folder 
(not strictly a problem as it would simply have no effect, but strange 
nonetheless)?

- Thomas Pfeiffer


On Sept. 5, 2015, 8:11 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 8:11 p.m.)
> 
> 
> Review request for Baloo, Plasma, KDE Usability, Pinak Ahuja, and Vishesh 
> Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> Screenshot of the new look: http://i.imgur.com/RBWxutv.png
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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

Re: Review Request 125058: Cleanup and Add directories to index list

[Ovidiu-Florin BOGDAN]

> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > Can you please attach some images of how the KCM now looks?

Should I make a new review request with each patch? Or a single one with 3 
diffs?


> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/CMakeLists.txt, line 20
> > 
> >
> > You seem to be using 'QStorageInfo' instead of Solid. Why?
> > 
> > Also, if we decide that is what we want to do, it should in a separate 
> > patch. This patch is large and should be split.
> 
> Ovidiu-Florin BOGDAN wrote:
> It is simpler to get the desired information with QStorageInfo, and it is 
> less error prone. The risk for errors is smaller.
> 
> Also Solid is another dependency. Qt is already there.

I cannot separate this cleanly into a separate patch, because this is part of a 
biger refactorization.


> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/configwidget.ui, line 43
> > 
> >
> > Moving the checkBox from the top to bottom? Why? Also, if we want this 
> > it goes in a seperate patch.
> 
> Ovidiu-Florin BOGDAN wrote:
> I moved it from the bottom to the top.
> 
> Vishesh Handa wrote:
> Right, bottom to top. Why?
> 
> Ovidiu-Florin BOGDAN wrote:
> It seems to me more logical this way. Here's the screenshot: 
> http://i.imgur.com/RBWxutv.png

Separated into a different patch.


> On Sept. 5, 2015, 3:12 p.m., Vishesh Handa wrote:
> > kcms/baloo/folderselectionwidget.cpp, line 130
> > 
> >
> > Is this change required?
> 
> Ovidiu-Florin BOGDAN wrote:
> The name is more descriptive like this. It is less likely to cause 
> confusion.
> 
> Vishesh Handa wrote:
> It is only used in one place, which is **one line** below this.
> 
> Anyway, if you still want to commit this, it goes in a seperate patch.

Separated into different patch.


- Ovidiu-Florin


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


On Sept. 5, 2015, 2:33 p.m., Ovidiu-Florin BOGDAN wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125058/
> ---
> 
> (Updated Sept. 5, 2015, 2:33 p.m.)
> 
> 
> Review request for Baloo, Plasma, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> ---
> 
> Added the functionality to add directories to be indexed.
> Did some cleaning up.
> 
> 
> Diffs
> -
> 
>   kcms/baloo/CMakeLists.txt 7415289 
>   kcms/baloo/configwidget.ui 512e4a5 
>   kcms/baloo/folderselectionwidget.h 226ab45 
>   kcms/baloo/folderselectionwidget.cpp b44d111 
>   kcms/baloo/kcm.h 6ff5813 
>   kcms/baloo/kcm.cpp 27d93e2 
> 
> Diff: https://git.reviewboard.kde.org/r/125058/diff/
> 
> 
> Testing
> ---
> 
> Tested add, remove include and exclude directories.
> 
> 
> Thanks,
> 
> Ovidiu-Florin BOGDAN
> 
>

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