----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124699/#review83789 -----------------------------------------------------------
Whoa! Huge patch. But I like the direction we're going on in. src/file/extractor/app.cpp (line 71) <https://git.reviewboard.kde.org/r/124699/#comment58015> I know I'm using `documentUrl` over here when I actually mean file path. I shouldn't be doing that :( src/file/extractorprocess.h (line 40) <https://git.reviewboard.kde.org/r/124699/#comment58010> Could you please rename this? * It would be nice if we could avoid using the term `url` as we're actually dealing with file paths. URLs are supposed to have a scheme, and follow a specific RFC. * How about `finishedIndexingFile(QString filePath)`? src/file/extractorprocess.cpp (line 71) <https://git.reviewboard.kde.org/r/124699/#comment58012> Could you please check if this is internally spawning an event loop? We want to avoid that. src/file/extractorprocess.cpp (line 79) <https://git.reviewboard.kde.org/r/124699/#comment58011> What is this variable used for? src/file/mainhub.h (line 40) <https://git.reviewboard.kde.org/r/124699/#comment58013> Could you please avoid using the word 'url' over here. src/file/mainhub.h (line 53) <https://git.reviewboard.kde.org/r/124699/#comment58014> I'm getting scared how `MainHub` is becoming a dumping ground for everything. Do these APIs need to be exposed over here. Maybe they can be exposed in other specific class. We're mostly just forwarding all the data + signals just for this dbus interface. src/file/mainhub.cpp (line 115) <https://git.reviewboard.kde.org/r/124699/#comment58016> This API is a little cryptic. So `registerMonitor(true)` registers it, and `registerMonitor(false` unregisters it? Maybe a separate function for unregistering? tests/file/CMakeLists.txt (line 22) <https://git.reviewboard.kde.org/r/124699/#comment58017> If you're disabling this temporarily, please add a comment why. - Vishesh Handa On Aug. 11, 2015, 2:55 p.m., Pinak Ahuja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124699/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2015, 2:55 p.m.) > > > Review request for Baloo and Vishesh Handa. > > > Repository: baloo > > > Description > ------- > > * Previously the url beind indexed was exported by baloo_file_extractor which > was not ideal because calls to fetch currently being indexed url were blocked > till the current batch was indexed. > * Now, the extractor writes the url of the file it starts indexing to stdout, > which is read and emitted over dbus by the main thread. There's a long chain > of signals, to get the url to the mainhub right now this should be fixed in > later patches when we register different objects over dbus as discussed > during akademy. > * Also the remaining time test needs to be rewritten completely, and will > need to run baloo_file, instead of only the baloo_file_extractor. > > > Diffs > ----- > > src/dbus/CMakeLists.txt baf0cdf > src/file/extractor/app.h 9df4109 > src/file/extractor/app.cpp 2fb4fd6 > src/file/extractor/iohandler.h fac004c > src/file/extractor/iohandler.cpp 601759a > src/file/extractor/main.cpp 21df5ee > src/file/extractor/org.kde.baloo.extractor.xml 1a7c2c4 > src/file/extractorprocess.h 3106a71 > src/file/extractorprocess.cpp 7ce1936 > src/file/filecontentindexer.h 1e268c0 > src/file/filecontentindexer.cpp b8bdcdc > src/file/fileindexscheduler.h a3cb779 > src/file/fileindexscheduler.cpp 5d86d9e > src/file/mainhub.h bb4b287 > src/file/mainhub.cpp 955e23b > src/file/org.kde.baloo.file.indexer.xml 312ee7d > src/tools/baloo-monitor/CMakeLists.txt 527435d > src/tools/baloo-monitor/monitor.h 926450b > src/tools/baloo-monitor/monitor.cpp 281b7c2 > src/tools/balooctl/CMakeLists.txt 50fa50f > src/tools/balooctl/monitor.h b8e0402 > src/tools/balooctl/monitor.cpp a833f19 > tests/file/CMakeLists.txt 98f5a00 > > Diff: https://git.reviewboard.kde.org/r/124699/diff/ > > > Testing > ------- > > Monitor seems to be working as before. > > > Thanks, > > Pinak Ahuja > >
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
