Review Request 124919: Re-organize D-Bus interfaces

2015-08-25 Thread Pinak Ahuja 

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

Review request for Baloo and Vishesh Handa.


Repository: baloo


Description
---

* Previously mainhub class was being used to forward D-Bus calls to relevant 
objects
* Now each object that needs D-Bus communication is registered as a separate 
D-Bus object and communication takes place directly.
* FileContentIndexer has been made a long lived class now to register a D-Bus 
object for it.


Diffs
-

  src/dbus/CMakeLists.txt 7c37d94 
  src/file/filecontentindexer.h eeaa93f 
  src/file/filecontentindexer.cpp 26f98a3 
  src/file/fileindexscheduler.h 90c23c9 
  src/file/fileindexscheduler.cpp 89587bc 
  src/file/indexerstate.h 2ed8ec9 
  src/file/mainhub.h 08993c6 
  src/file/mainhub.cpp dcfac30 
  src/tools/baloo-monitor/CMakeLists.txt 1abb16a 
  src/tools/baloo-monitor/monitor.h 597690a 
  src/tools/baloo-monitor/monitor.cpp 485db87 
  src/tools/balooctl/CMakeLists.txt 2823e65 
  src/tools/balooctl/main.cpp 81bedf6 
  src/tools/balooctl/monitor.h 2a39ba9 
  src/tools/balooctl/monitor.cpp c5d3b50 

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


Testing
---

baloo-monitor and balooctl seem to be working as before.


Thanks,

Pinak Ahuja


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Review Request 124843: Use json metadata in KDED module and fix plugin name

2015-08-25 Thread Vishesh Handa 

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

Ship it!


Looks great!

- Vishesh Handa


On Aug. 20, 2015, 6:43 p.m., Ragnar Thomsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124843/
> ---
> 
> (Updated Aug. 20, 2015, 6:43 p.m.)
> 
> 
> Review request for Baloo.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> This patch ports the kded-module of baloo to use json metadata and disables 
> installation of the desktop file. Additionally, the name of the plugin and 
> desktop file is changed to be similar to the class (baloosearchmodule), and 
> the two parameters in the desktop file (`X-KDE-Library` and 
> `X-KDE-DBus-ModuleName`) are also set to baloosearchmodule. The latter was 
> causing an issue with the kded kcm failing to load the baloo module.
> 
> If the name baloosearchmodule is not descriptive enough we can change it. 
> However, the different components should have the same name to avoid problems 
> with kded.
> 
> 
> Diffs
> -
> 
>   src/kioslaves/kded/baloosearchmodule.desktop 9d84e2b 
>   src/kioslaves/kded/CMakeLists.txt 41f1ef2 
>   src/kioslaves/kded/baloosearchmodule.cpp 93afa79 
> 
> Diff: https://git.reviewboard.kde.org/r/124843/diff/
> 
> 
> Testing
> ---
> 
> The baloo kded module now appears in the kded kcm, and can be 
> enabled/disabled.
> 
> 
> Thanks,
> 
> Ragnar Thomsen
> 
>


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Review Request 124919: Re-organize D-Bus interfaces

2015-08-25 Thread Vishesh Handa 

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


Looks good. The only minor thing I realized is that I'm not sure how QtDbus 
behaves with interfaces which appear and disappear (contentIndexer).


src/file/filecontentindexer.cpp (line 49)


Maybe just 'fileIndexer' or some similar small word? This is quite the 
mouthfull.



src/file/filecontentindexer.cpp (line 133)


This might not be thread safe.

It calls batchTimings which accesses `m_batchTimeBuffer`. This variable is 
also being modified in the `run` method which runs in another thread.



src/file/mainhub.cpp (line 46)


Maybe we can just use '/'? This is getting a bit wordy, and is exposing the 
internal class name/structure.


- Vishesh Handa


On Aug. 25, 2015, 11:28 a.m., Pinak Ahuja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124919/
> ---
> 
> (Updated Aug. 25, 2015, 11:28 a.m.)
> 
> 
> Review request for Baloo and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> * Previously mainhub class was being used to forward D-Bus calls to relevant 
> objects
> * Now each object that needs D-Bus communication is registered as a separate 
> D-Bus object and communication takes place directly.
> * FileContentIndexer has been made a long lived class now to register a D-Bus 
> object for it.
> 
> 
> Diffs
> -
> 
>   src/dbus/CMakeLists.txt 7c37d94 
>   src/file/filecontentindexer.h eeaa93f 
>   src/file/filecontentindexer.cpp 26f98a3 
>   src/file/fileindexscheduler.h 90c23c9 
>   src/file/fileindexscheduler.cpp 89587bc 
>   src/file/indexerstate.h 2ed8ec9 
>   src/file/mainhub.h 08993c6 
>   src/file/mainhub.cpp dcfac30 
>   src/tools/baloo-monitor/CMakeLists.txt 1abb16a 
>   src/tools/baloo-monitor/monitor.h 597690a 
>   src/tools/baloo-monitor/monitor.cpp 485db87 
>   src/tools/balooctl/CMakeLists.txt 2823e65 
>   src/tools/balooctl/main.cpp 81bedf6 
>   src/tools/balooctl/monitor.h 2a39ba9 
>   src/tools/balooctl/monitor.cpp c5d3b50 
> 
> Diff: https://git.reviewboard.kde.org/r/124919/diff/
> 
> 
> Testing
> ---
> 
> baloo-monitor and balooctl seem to be working as before.
> 
> 
> Thanks,
> 
> Pinak Ahuja
> 
>


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Review Request 124919: Re-organize D-Bus interfaces

2015-08-25 Thread Pinak Ahuja 


> On Aug. 25, 2015, 4:54 p.m., Vishesh Handa wrote:
> > Looks good. The only minor thing I realized is that I'm not sure how QtDbus 
> > behaves with interfaces which appear and disappear (contentIndexer).

ContentIndexer doesn't appear and disappear now, it's a long lived class, with 
this patch. Handling the monitor registration with an appearing and 
disappearing class would be a pain.


> On Aug. 25, 2015, 4:54 p.m., Vishesh Handa wrote:
> > src/file/mainhub.cpp, line 53
> > 
> >
> > Maybe we can just use '/'? This is getting a bit wordy, and is exposing 
> > the internal class name/structure.

I'll rename the objects to something smaller.


> On Aug. 25, 2015, 4:54 p.m., Vishesh Handa wrote:
> > src/file/filecontentindexer.cpp, line 134
> > 
> >
> > This might not be thread safe.
> > 
> > It calls batchTimings which accesses `m_batchTimeBuffer`. This variable 
> > is also being modified in the `run` method which runs in another thread.

I'll need to check this out.


- Pinak


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


On Aug. 25, 2015, 11:28 a.m., Pinak Ahuja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124919/
> ---
> 
> (Updated Aug. 25, 2015, 11:28 a.m.)
> 
> 
> Review request for Baloo and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> * Previously mainhub class was being used to forward D-Bus calls to relevant 
> objects
> * Now each object that needs D-Bus communication is registered as a separate 
> D-Bus object and communication takes place directly.
> * FileContentIndexer has been made a long lived class now to register a D-Bus 
> object for it.
> 
> 
> Diffs
> -
> 
>   src/dbus/CMakeLists.txt 7c37d94 
>   src/file/filecontentindexer.h eeaa93f 
>   src/file/filecontentindexer.cpp 26f98a3 
>   src/file/fileindexscheduler.h 90c23c9 
>   src/file/fileindexscheduler.cpp 89587bc 
>   src/file/indexerstate.h 2ed8ec9 
>   src/file/mainhub.h 08993c6 
>   src/file/mainhub.cpp dcfac30 
>   src/tools/baloo-monitor/CMakeLists.txt 1abb16a 
>   src/tools/baloo-monitor/monitor.h 597690a 
>   src/tools/baloo-monitor/monitor.cpp 485db87 
>   src/tools/balooctl/CMakeLists.txt 2823e65 
>   src/tools/balooctl/main.cpp 81bedf6 
>   src/tools/balooctl/monitor.h 2a39ba9 
>   src/tools/balooctl/monitor.cpp c5d3b50 
> 
> Diff: https://git.reviewboard.kde.org/r/124919/diff/
> 
> 
> Testing
> ---
> 
> baloo-monitor and balooctl seem to be working as before.
> 
> 
> Thanks,
> 
> Pinak Ahuja
> 
>


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Review Request 124919: Re-organize D-Bus interfaces

2015-08-25 Thread Pinak Ahuja 

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

(Updated Aug. 25, 2015, 5:52 p.m.)


Review request for Baloo and Vishesh Handa.


Repository: baloo


Description
---

* Previously mainhub class was being used to forward D-Bus calls to relevant 
objects
* Now each object that needs D-Bus communication is registered as a separate 
D-Bus object and communication takes place directly.
* FileContentIndexer has been made a long lived class now to register a D-Bus 
object for it.


Diffs (updated)
-

  src/dbus/CMakeLists.txt 7c37d94 
  src/file/filecontentindexer.h eeaa93f 
  src/file/filecontentindexer.cpp 26f98a3 
  src/file/fileindexscheduler.h 90c23c9 
  src/file/fileindexscheduler.cpp 89587bc 
  src/file/indexerstate.h 2ed8ec9 
  src/file/mainhub.h 08993c6 
  src/file/mainhub.cpp dcfac30 
  src/tools/baloo-monitor/CMakeLists.txt 1abb16a 
  src/tools/baloo-monitor/monitor.h 597690a 
  src/tools/baloo-monitor/monitor.cpp 485db87 
  src/tools/balooctl/CMakeLists.txt 2823e65 
  src/tools/balooctl/main.cpp 81bedf6 
  src/tools/balooctl/monitor.h 2a39ba9 
  src/tools/balooctl/monitor.cpp c5d3b50 

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


Testing
---

baloo-monitor and balooctl seem to be working as before.


Thanks,

Pinak Ahuja


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Review Request 124919: Re-organize D-Bus interfaces

2015-08-25 Thread Pinak Ahuja 


> On Aug. 25, 2015, 4:54 p.m., Vishesh Handa wrote:
> > src/file/filecontentindexer.cpp, line 134
> > 
> >
> > This might not be thread safe.
> > 
> > It calls batchTimings which accesses `m_batchTimeBuffer`. This variable 
> > is also being modified in the `run` method which runs in another thread.
> 
> Pinak Ahuja wrote:
> I'll need to check this out.

This is not thread safe, the solution would be to move all the time estimation 
code including the circular buffer to the main thread. Lets do this in a 
separate patch considering this behaviour was present before this patch.


- Pinak


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


On Aug. 25, 2015, 5:52 p.m., Pinak Ahuja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124919/
> ---
> 
> (Updated Aug. 25, 2015, 5:52 p.m.)
> 
> 
> Review request for Baloo and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> * Previously mainhub class was being used to forward D-Bus calls to relevant 
> objects
> * Now each object that needs D-Bus communication is registered as a separate 
> D-Bus object and communication takes place directly.
> * FileContentIndexer has been made a long lived class now to register a D-Bus 
> object for it.
> 
> 
> Diffs
> -
> 
>   src/dbus/CMakeLists.txt 7c37d94 
>   src/file/filecontentindexer.h eeaa93f 
>   src/file/filecontentindexer.cpp 26f98a3 
>   src/file/fileindexscheduler.h 90c23c9 
>   src/file/fileindexscheduler.cpp 89587bc 
>   src/file/indexerstate.h 2ed8ec9 
>   src/file/mainhub.h 08993c6 
>   src/file/mainhub.cpp dcfac30 
>   src/tools/baloo-monitor/CMakeLists.txt 1abb16a 
>   src/tools/baloo-monitor/monitor.h 597690a 
>   src/tools/baloo-monitor/monitor.cpp 485db87 
>   src/tools/balooctl/CMakeLists.txt 2823e65 
>   src/tools/balooctl/main.cpp 81bedf6 
>   src/tools/balooctl/monitor.h 2a39ba9 
>   src/tools/balooctl/monitor.cpp c5d3b50 
> 
> Diff: https://git.reviewboard.kde.org/r/124919/diff/
> 
> 
> Testing
> ---
> 
> baloo-monitor and balooctl seem to be working as before.
> 
> 
> Thanks,
> 
> Pinak Ahuja
> 
>


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Help wanted to evolve KDEs music players

2015-08-25 Thread Pinak Ahuja 
So, what's up with this? Anyone working on something? I'm planning to start
work on a simple player based on the VDG's design using
https://github.com/vlc-qt/vlc-qt for audio playback. The player will not
support any online accounts, will only be for local music playback. IMO if
a user needs online accounts they have other players to choose from
(tomahawk). KDE should at least have a  basic player with a nice
"plasmaish" UI.

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Review Request 124843: Use json metadata in KDED module and fix plugin name

2015-08-25 Thread Ragnar Thomsen 

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

(Updated Aug. 25, 2015, 8:42 p.m.)


Status
--

This change has been marked as submitted.


Review request for Baloo.


Changes
---

Submitted with commit e0f10327d0c487173429b64e62ade539625d88a9 by Ragnar 
Thomsen to branch master.


Repository: baloo


Description
---

This patch ports the kded-module of baloo to use json metadata and disables 
installation of the desktop file. Additionally, the name of the plugin and 
desktop file is changed to be similar to the class (baloosearchmodule), and the 
two parameters in the desktop file (`X-KDE-Library` and 
`X-KDE-DBus-ModuleName`) are also set to baloosearchmodule. The latter was 
causing an issue with the kded kcm failing to load the baloo module.

If the name baloosearchmodule is not descriptive enough we can change it. 
However, the different components should have the same name to avoid problems 
with kded.


Diffs
-

  src/kioslaves/kded/baloosearchmodule.desktop 9d84e2b 
  src/kioslaves/kded/CMakeLists.txt 41f1ef2 
  src/kioslaves/kded/baloosearchmodule.cpp 93afa79 

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


Testing
---

The baloo kded module now appears in the kded kcm, and can be enabled/disabled.


Thanks,

Ragnar Thomsen


>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<