D9712: balooctl status: produce parseable output

[Vishesh Handa]

vhanda added a comment.


  In https://phabricator.kde.org/D9712#191746, @ltoscano wrote:
  
  > May I observe that relying on the command line output is fragile in the 
best case?
  >  Unless the  command can produce the output in some formatted way (like the 
openstack CLI, which can enable a special mode to produce output in various 
formats like csv, table-formatted, or direct values, and select the columns).
  
  
  I agree. Having a special mode which gives json/csv or some other machine 
readable output is a far better option.

REPOSITORY
  R293 Baloo

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

To: michaelh, #frameworks, vhanda, smithjd
Cc: ltoscano, lbeltrame, elvisangelaccio, lueck, dfaure, ngraham

D8007: popplerextractor: don't try to guess the title if there isn't one.

[Vishesh Handa]

vhanda added a comment.


  To give some history - Strigi used to do this guessing, and when I 
implemented KFileMetaData, I decided to do the same. I don't have an opinion on 
whether it is a good idea or not.

REPOSITORY
  R286 KFileMetaData

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

To: flameeyes, #frameworks, mgallien
Cc: mgallien, vhanda, ngraham, #frameworks

D8007: popplerextractor: don't try to guess the title if there isn't one.

[Vishesh Handa]

vhanda added a comment.


  > @vhanda do you know if there is something to do to update Baloo database 
when metadata returned by KFileMetaData are changed even if the file itself did 
not change ?
  
  I'm starting to forget the code base and what all I implemented. But I don't 
think such a thing was implemented.

REPOSITORY
  R286 KFileMetaData

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

To: flameeyes, #frameworks, mgallien
Cc: mgallien, vhanda, ngraham, #frameworks

D8527: Display "Downloaded From" by default, if file supports it

[Vishesh Handa]

vhanda accepted this revision.
vhanda added a comment.
This revision is now accepted and ready to land.


  Looks good.
  
  Maybe we should wait and see if someone who works on Dolphin has an opinion. 
This widget is only used in Dolphin.

REPOSITORY
  R824 Baloo Widgets

BRANCH
  origin_url_386261

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

To: ngraham, #dolphin, #frameworks, broulik, dfaure, markg, vhanda
Cc: vhanda, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp

D8528: Consider DjVu files to be documents

[Vishesh Handa]

vhanda added a comment.


  Since I was added as a reviewer, I thought I'll comment.
  
  I am not currently maintaining Baloo or using it, so I don't want to really 
discuss the specifics. Though from a technical point of view this change will 
work. If nobody has any objections, I would say go for it.

REPOSITORY
  R293 Baloo

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

To: ngraham, vhanda, #frameworks, rkflx
Cc: rkflx, #frameworks

D4995: Fix DB inconsistency due to some docterms appearing with uppercase symbols

[Vishesh Handa]

vhanda accepted this revision.
vhanda added a comment.
This revision is now accepted and ready to land.


  This is awesome. Good work.
  
  Ship it! (If you don't have commit access, please ask for it, you can add me 
as a reference)
  
  (Optionally, one could even add a unit test)

REPOSITORY
  R293 Baloo

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

To: poboiko, pinakahuja, vhanda
Cc: #frameworks

D4911: add Baloo DBus signals for moved or removed files

[Vishesh Handa]

vhanda added a comment.


  I'm not the maintainer of Baloo any more, so I don't want to give it a clear 
Yes / No.
  
  This patch is going to be a big CPU hog. For files this will barely have an 
impact, but for folders of a large enough size, it's going to result in tons of 
dbus signals, and more importantly, tons of database lookups and full path 
constructions. It really will add up. In an earlier version of Baloo, we used 
to store the full file paths, which meant when a folder moved, every 
sub-file/folder's URL had to be updated. That was a huge CPU burner. This patch 
isn't that bad, but it's half way there.
  
  I cannot see what advantage these signals add, apart from possibly making 
Baloo more introspect-able. If it's to build applications on top of Baloo, I 
would recommend against it. The kernel provides file system monitoring APIs 
which should be used instead.

REPOSITORY
  R293 Baloo

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

To: mgallien, vhanda
Cc: apol, #frameworks

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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

(Updated Feb. 26, 2017, 8:47 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks.


Bugs: 340819
https://bugs.kde.org/show_bug.cgi?id=340819


Repository: kunitconversion


Description
---

Currency: Fetch the currency file properly

Properly run an event loop and wait for the file to be fetched.

Also add a test to make sure currency conversion is working.

This patch also contains -
* https://git.reviewboard.kde.org/r/122182/
* https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/

This is because reviewboard refuses to upload only a part of the diff. Please 
only look at currency.cpp w.r.t the EventLoop.


Diffs
-

  autotests/convertertest.h 8129a48 
  autotests/convertertest.cpp ae4298e 
  src/currency.cpp 715233c 

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


Testing
---

Test now passes.


Thanks,

Vishesh Handa

Re: Scrap Baloo Thread Feedback

[Vishesh Handa]

On Sun, Oct 16, 2016 at 2:16 PM, Christoph Cullmann <cullm...@absint.com> wrote:
> Hi,
>
> (evil top posting)
>
> given the silence, I assume any interest in baloo has stopped once more, or?
> Or are there any plans how to fixup the current situation?

I'm not going to be very involved with this.

I've already expressed my opinion. You all are free to make a decision.

--
Vishesh Handa

Re: Scrap Baloo Thread Feedback

[Vishesh Handa]

On Fri, Oct 7, 2016 at 8:08 PM, Christoph Cullmann  wrote:
>
> I did experiments and search works with tracker, but yes, a problem is 
> tagging,+
> which ATM doesn't work. Nor do I say that is a ready solution now, just a 
> possibility
> to avoid having to maintain low level code with at most 1 person (how it 
> looks ATM).
>
> And I don't propose to go that road now, but ATM I see nobody doing any other 
> experiments.
>
> Besides, tracker is constantly maintained and used since >> 5 years:
>
> https://github.com/GNOME/tracker/graphs/contributors

ok. Baloo clearly isn't being maintained.

>
>>
>>>
>>> => That is good that we agree, but I find it very astonishing that we use 
>>> baloo
>>> in its
>>> current state more or less mandatory on all that systems were it by design 
>>> will
>>> fail.
>>>
>>> (and it fails if you read the bugs)
>>>
>>
>> There is a certain amount of failure, but it's not "by-design". But
>> maybe I'm not seeing things clearly.
> You yourself stated that neither 32-bit issues nor NFS nor > 32-bit inodes 
> have any
> error handling. And that seems to have been known even during design and still
> we have this now as a framework per default used by any Plasma installation on
> systems exactly featuring that without error checking.
>
>>

>>
>> How about requirements such as resource consumption, ease of
>> integration, search speed are taken into consideration? Come on guys.
>> We're engineers over here.
>>
> What is the argument here? If you take a look at bugs.kde.org, you see 
> that
> people are complaining about all
> of that with baloo. I see no evidence nowhere that e.g. baloo is 
> "superior" to
> what GNOME uses
> or any other solution (perhaps beside nepomuk, ok...).
>>
>> What tests have been to obtain the evidence?
> What tests have been done to obtain the inverse evidence? I only hear here 
> the complaint
> about not taking requirements like resource consumption or speed into 
> account, but
> there is ATM zero evidence that e.g. tracker is slower.
>

I did do a lot of tests during the design of Baloo. I don't have hard numbers.

Even if I didn't, that doesn't mean a decision should be made without
gathering proper data.

> And the typical error check is:
>
> void MTimeDB::put(quint32 mtime, quint64 docId)
> {
> Q_ASSERT(mtime > 0);
> Q_ASSERT(docId > 0);
>
> MDB_val key;
> key.mv_size = sizeof(quint32);
> key.mv_data = static_cast();
>
> MDB_val val;
> val.mv_size = sizeof(quint64);
> val.mv_data = static_cast();
>
> int rc = mdb_put(m_txn, m_dbi, , , 0);
> Q_ASSERT_X(rc == 0, "MTimeDB::put", mdb_strerror(rc));
> }
>
> without any way to pass an error to the outside, nor any error handling code 
> at the outside,
> as no error can ever occur that is non-fatal.
>

ok. The API isn't exported. It can be changed.

But we both seem to have different opinions of how much work this would be.

>
> Besides I don't see any documentation of the DB format, but I could miss that.
> (at least not in the git nor https://community.kde.org/Baloo)
>

There isn't any.

>
> What would be highly appreciated would be a bit of documentation what the
> different pieces do and stuff like that, even if you have no time to code.
>

If you can send me specific questions about different parts I can
answer them. For "general documentation", I don't know where to start.
I usually much prefer just going through the code.


> Greetings
> Christoph
>

Re: Scrap Baloo Thread Feedback

[Vishesh Handa]

Hey

On Fri, Oct 7, 2016 at 6:34 PM, Christoph Cullmann <cullm...@absint.com> wrote:
> Hi,
>
>> On Fri, Oct 7, 2016 at 5:58 PM, Christoph Cullmann <cullm...@absint.com> 
>> wrote:
>>>
>
> 1) No handling of DB errors beside asserting
> 2) No handling of errors in the extractors (e.g. see the fixes I did, all 
> extractors will need more of that)
> 3) No handling of NFS/large inodes/inconsistencies => crash
>
> In the end, in my opinion, you can rewrite close to all parts dealing with 
> the DB or
> any other thing internally. If ever any thing gots inconsistent, ATM you are 
> doomed, forever,
> if not by luck my new startup code deletes the index, then you live again 
> until it is reindexed.
>
>>
> I am not sure, I am all for removing complete indexing and use a other indexer
> like tracker to exactly avoid the excurse into DB world and how to handle it
> in a safe way with close to zero person manpower.
>

It's avoiding the problem and hoping for the best, without any experiments.

>
> => That is good that we agree, but I find it very astonishing that we use 
> baloo in its
> current state more or less mandatory on all that systems were it by design 
> will fail.
>
> (and it fails if you read the bugs)
>

There is a certain amount of failure, but it's not "by-design". But
maybe I'm not seeing things clearly.

>>
>>>>
>>>> How about requirements such as resource consumption, ease of
>>>> integration, search speed are taken into consideration? Come on guys.
>>>> We're engineers over here.

>>> What is the argument here? If you take a look at bugs.kde.org, you see that
>>> people are complaining about all
>>> of that with baloo. I see no evidence nowhere that e.g. baloo is "superior" 
>>> to
>>> what GNOME uses
>>> or any other solution (perhaps beside nepomuk, ok...).

What tests have been to obtain the evidence?

>
>>
>> Yup, you have. It's awesome. I no longer have the motivation to work on 
>> Baloo.
> Thanks, but that makes me very sad, btw.
> Baloo came up to replace nepomuk, which was dead because it had too many bugs 
> and all maintainers left.
> Now we have baloo, which has many bugs, some even by design, and the 
> maintainer left, too.
>

Actually, Nepomuk was not dead. I was maintaining it. I killed it
because it had too many structural problems.

This is how the open source world works. People work on projects and
when it no longer scratches their itch (I no longer use Baloo), they
loose interest. This is "supposed" to be a hobby.

>
>> (This is why they run on a separate process)
> That doesn't help, it just OOMs your system => dead, it needs resource 
> restrictions,
> which is tricky to get right.
>

You're right. It needs a better thought out solution. A separate
process is the bare minimum.

Btw, have you looked if Tracker actually does any of this?

>> My hostility was because the proposal ignores key points such as -
>>
>> * Indexing Speed
>> * Search speed
>> * Database size
> => If you look at the bugs, people complain we are inferior and I see not
> that the proposal ignores it, I just see not how to compare, given there are 
> no
> hard facts that we are faster than e.g. tracker in any way.
>

Data can be gathered about it. Not all data is publicly available.

>> * Ease of use with our existing components
> My proposal did not change the interface at all, it has zero impact on "ease 
> of use".
>
>> * Ease of fixing problems in the code
> My estimate would be: rewrite close to everything. Even the basic 64-bit int 
> id won't work
> with 64-bit inodes, each DB call must be touched to check for errors, at each 
> place
> one will need to check for potential inconsistencies and exit gracefully...
>

I don't follow why everything needs to be re-written? Am I missing
something or do we just need to check for more errors and use a higher
integer id? This certainly doesn't seem super trivial, but it sounds
like less work than implementing a shim on top of Tracker.

I could be wrong.

>>
>> Baloo has certain speed requirements if it is to be used with krunner,
>> and we want instant feedback. This was an integral requirement.
> I doubt e.g. tracker has different requirements, as it is used in similar 
> places by GNOME.
>
> But all that left besides, have you an proposal how to fixup the current 
> situation?
> Are you willing to invest some work to fix the current issues or an idea what
> would be a good way to tackle them?
>

I probably will not work more in Baloo.

I'll have to investigate the problems a bit more. From the cursory
look of this thread, it doesn't seem that the problems are that dire.
But I may not be reading into it correctly.


--
Vishesh Handa

Re: Scrap Baloo Thread Feedback

[Vishesh Handa]

On Fri, Oct 7, 2016 at 6:20 PM, Aleix Pol <aleix...@kde.org> wrote:
>>>
>>
>> I don't understand why all framework discussions must happen on the
>> same list. It just adds to a crazy amount of noise, which one then
>> needs to parse through.
>
> Arguing that it should be elsewhere because you'd like to ignore the
> rest of the traffic in kde-frameworks doesn't sound very constructive,
> especially considering how they're the "noise" that actually improves
> the frameworks.
>
> Maybe you can better configure your e-mail client differently so we
> can focus on the issue at matter?

This is not about how it should be. I'm informing them why it was
chosen to be somewhere else. This decision can be changed.

Frameworks collectively may or may not improve by having everything in
one place. Lets not treat it as a axiom.

An analogy could be that we get commit emails, but we get to choose
which projects we are interested in. We don't make everyone subscribe
to kde-commits, and then put their own complex filters on top.

--
Vishesh Handa

Re: Scrap Baloo Thread Feedback

[Vishesh Handa]

On Fri, Oct 7, 2016 at 6:14 PM, Christoph Cullmann <cullm...@absint.com> wrote:
>>>
>>
>> I don't understand why all framework discussions must happen on the
>> same list. It just adds to a crazy amount of noise, which one then
>> needs to parse through.
>
> If you would have baloo-devel I could understand that point,
> but not with some other generic mailing list like kde-devel which
> has the same amount of noise and is not even dedicated to 'frameworks'
> or 'baloo'.

If you guys plans to use frameworks devel, then please change the
review requests.

It was just too much noise for me, and I found the noise/signal ratio
way lower in kde-devel. Baloo-devel was specifically not chosen as it
would just an another silo in kde. Nepomuk used to suffer from that.

--
Vishesh Handa

Re: Scrap Baloo Thread Feedback

[Vishesh Handa]

On Fri, Oct 7, 2016 at 5:57 PM, Kevin Funk <kf...@kde.org> wrote:
> On Friday, 7 October 2016 17:24:26 CEST Vishesh Handa wrote:
>> Hey guys
>>
>> I was told there is a thread about scrapping Baloo. All Baloo
>> discussion used to happen on kde-devel and that's where the review
>> requests go. It's the only reason I am still subscribed to kde-devel.
>
> Heya,
>
> Baloo is a framework nowadays, therefore it totally makes sense to have the
> discussion on kde-framework-devel.
>
> There's been tons of discussion around Baloo on kde-framework-devel already.
> kde-frameworks-devel is also where the CI messages for the baloo repo go to.
>
> It likely makes sense for you to subscribe, no?
>

I don't understand why all framework discussions must happen on the
same list. It just adds to a crazy amount of noise, which one then
needs to parse through.

> Cheers,
> Kevin
>
>> (snip)
>
>
> --
> Kevin Funk | kf...@kde.org | http://kfunk.org

Scrap Baloo Thread Feedback

[Vishesh Handa]

Hey guys

I was told there is a thread about scrapping Baloo. All Baloo
discussion used to happen on kde-devel and that's where the review
requests go. It's the only reason I am still subscribed to kde-devel.

I must say, the thread is overall quite disappointing. There seems to
be no scientific or rationale cost based analysis of this. How about a
list of requirements and priorities are drawn up and then possible
solutions are evaluated according to it?

Right now, random requirements such as NFS and 32bit systems are
coming up. Are these really that important? I specifically designed
Baloo to not care about both network mounts and 32-bit systems. Yes,
Baloo has bugs and it won't handle more than 32bit-inodes. These
things, as all others, can be fixed. It's really a question of what is
important. Lets not target the outliers. Many of these decisions were
deliberately taken.

How about requirements such as resource consumption, ease of
integration, search speed are taken into consideration? Come on guys.
We're engineers over here.

(If the discussion continues on kde-frameworks-devel, I probably won't see it)

--
Vishesh Handa

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Vishesh Handa]

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


Ship it!




This is awesome. Ship it.

- Vishesh Handa


On Sept. 11, 2016, 7:19 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 11, 2016, 7:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Find out whether a file is a remote file or not

[Vishesh Handa]

On Sun, May 1, 2016 at 6:28 PM, Dominik Haumann <dhaum...@kde.org> wrote:
>
> CC: Vishesh, since maybe baloo also had to solve these issues

We mostly just use Solid to query the list of mount points and ignore
non local stuff.

baloo/src/file/storagedevices.h

--
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Please add new versions on bugs.kde.org products on KF5 releases

[Vishesh Handa]

On Sat, Apr 9, 2016 at 12:09 PM, David Faure <fa...@kde.org> wrote:
> One following issue remains:
> there is no frameworks-krunner bugzilla product. There is a krunner product, 
> but it's not the same
> thing, it's not KF5-versionned. Should we add one? Who would be the 
> maintainer?
> (the yaml file points to Vishesh).

Whatever is easier.

I am the maintainer, but I haven't been doing anything, so someone
else should take over. I'll write an email announcing that soon.

--
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127023: [KFileMetadata] Support Origin Email subject/sender/id

[Vishesh Handa]

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


Fix it, then Ship it!





src/propertyinfo.cpp (line 421)
<https://git.reviewboard.kde.org/r/127023/#comment63024>

I'm not too keen on indexing this unless there is a clear use case. It just 
increases the size of the index.

During the Nepomuk days we used to index and collect way too much 
information with the hope that we would be find amazing creative uses for the 
data. It did not end well.


- Vishesh Handa


On Feb. 9, 2016, 9:09 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127023/
> ---
> 
> (Updated Feb. 9, 2016, 9:09 p.m.)
> 
> 
> Review request for KDE Frameworks, KDEPIM, Daniel Vrátil, Sebastian Kügler, 
> and Vishesh Handa.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> This adds support for the user.xdg.origin.email.subject, 
> user.xdg.origin.email.sender, user.xdg.origin.email.message-id xattrs [1] to 
> KFileMetadata.
> 
> This can (should :P) be populated by KMail when you save an attachment.
> 
> Not too happy about the "displayName" I chose. Also we'll need to figure out 
> what to index and how we can relate this information and present it to the 
> user in a meaningful way. But at least let's collect the information first 
> and then we can think about ways to handle this.
> 
> [1] https://wiki.freedesktop.org/www/CommonExtendedAttributes/
> 
> 
> Diffs
> -
> 
>   src/properties.h 6ceaca5 
>   src/propertyinfo.cpp 4d1fac4 
>   src/usermetadata.h 9e10d2a 
>   src/usermetadata.cpp 5485d0e 
> 
> Diff: https://git.reviewboard.kde.org/r/127023/diff/
> 
> 
> Testing
> ---
> 
> Not really. Tests pass, though.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125762: External extractor plugin support for KFileMetaData

[Vishesh Handa]

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


If you're planning on pushing this please push it to a testing branch. Untill 
we actually have some plugins using this we might not be sure of the API. My 
points below are quite negative and I was hoping on writing a more positive 
email about the different ways to go about this, but I seem to have been 
procrastinating and have not written it. Sorry.

My main objections with this -

1. One cannot choose between external plugins. Since all of them are in 
1-plugin (called ExternalPlugin), and all the mimtypes are just combined. This 
matters less in the case of Baloo, but when someone else wants to choose 
specific extractors. It's impossible.

2. Extraction of plain text is simply not supported.

3. No way to differentiate in the implementation of a plugin. We may get 
plugins for the same mimetype using different technologies, perhaps we need a 
way to identify which one to choose. Maybe this will never be a problem, but 
I'm skeptical.

4. Contamination of the PATH, maybe we could put them in a different place 
which makes it obvious that users should never execute them.

5. Getting the list of extractors now means running a possibly large number of 
executables with possibly bad implementations. They could just get stuck, and 
all other plugins will suffer. Perhaps the list of mimetypes supported could be 
in a desktop file?

- Vishesh Handa


On Oct. 24, 2015, 12:19 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125762/
> ---
> 
> (Updated Oct. 24, 2015, 12:19 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> This patch introduces support for external metadata extractors in 
> KFileMetaData
> 
> The external extractors themselves can be written in any language, provided 
> that it can be executed as a standalone executable (compiled or script with a 
> hashbang), with command line arguments, and can output data to stdout.
> 
> The extractors are executed like so:
> 
> * `extractor --mimetypes` - outputs a list of mimetypes supported by the 
> extractor, one per line.
> * `extractor filename` - outputs a json document with the metadata. The keys 
> are such that they can be directly used with PropertyInfo::fromName().
> 
> At the KFileMetaData end, an additional internal plugin (ExternalExtractor) 
> is provided that forms a conduit between external extractors and the internal 
> API. This plugin looks for executables called 
> kfilemetadata_extractor_ in /usr/bin to find external extractors, 
> and executes them with the --mimetypes arg to find the list of mimetypes each 
> extractor supports. ExternalExtractor then claims to support all of these 
> mimetypes, and then delegates to the extractor executable when doing the 
> actual extraction.
> 
> 
> Diffs
> -
> 
>   README.md 19b1a26 
>   src/extractors/CMakeLists.txt 5dd223e 
>   src/extractors/externalextractor.h PRE-CREATION 
>   src/extractors/externalextractor.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125762/diff/
> 
> 
> Testing
> ---
> 
> Tested with the sample executable file extractor (as attched, written in 
> python) with the dump manual test in KFileMetaData. Works.
> 
> 
> File Attachments
> 
> 
> kfilemetadata_extractor_executable
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/23/146b657f-31d9-4117-a82f-ef966a6339d4__kfilemetadata_extractor_executable
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126110: Clean up and armour Baloo::Database::open()

[Vishesh Handa]

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

Ship it!



src/engine/database.cpp (line 73)
<https://git.reviewboard.kde.org/r/126110/#comment60854>

Please remove the comment. It's not longer valid.



src/engine/database.cpp (line 192)
<https://git.reviewboard.kde.org/r/126110/#comment60855>

Perhaps you want to close the env here as well? The assert can fail in 
production.


- Vishesh Handa


On Nov. 19, 2015, 11:39 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126110/
> ---
> 
> (Updated Nov. 19, 2015, 11:39 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Bugs: 353757
> http://bugs.kde.org/show_bug.cgi?id=353757
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> * Add checks for failures
> * Add manual checks after Q_ASSERT* (they're not compiled in Release mode)
> * Clean up m_env after failure, not just set it to 0
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp e39eb86 
> 
> Diff: https://git.reviewboard.kde.org/r/126110/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, does not crash.
> make test succeeds at 100%
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Vishesh Handa]

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

Ship it!


I'm a little hesitant about this, but ship it.

A couple of things though -
1. Dolphin should probably not be sending Baloo so many requests if Baloo is 
disabled. Perhaps we should fix this in Dolphin.
2. This patch is mostly only for the Baloo::File class which causes the db to 
be opened and closed for each operation. It's expensive, but the main use case 
that I focussed on was searching. Over there the db is only opened once per 
application.

- Vishesh Handa


On Nov. 18, 2015, 7:40 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125762: External extractor plugin support for KFileMetaData

[Vishesh Handa]

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


I still have many thoughts regarding this particular approach, and third party 
plugins in general. I'm trying to write them into a cohesive blob.


src/extractors/externalextractor.cpp (line 69)
<https://git.reviewboard.kde.org/r/125762/#comment59973>

Why move from the C++ for syntax to Q_FOREACH? There doesn't seem to be any 
advantage in this case.


- Vishesh Handa


On Oct. 24, 2015, 12:19 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125762/
> ---
> 
> (Updated Oct. 24, 2015, 12:19 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> This patch introduces support for external metadata extractors in 
> KFileMetaData
> 
> The external extractors themselves can be written in any language, provided 
> that it can be executed as a standalone executable (compiled or script with a 
> hashbang), with command line arguments, and can output data to stdout.
> 
> The extractors are executed like so:
> 
> * `extractor --mimetypes` - outputs a list of mimetypes supported by the 
> extractor, one per line.
> * `extractor filename` - outputs a json document with the metadata. The keys 
> are such that they can be directly used with PropertyInfo::fromName().
> 
> At the KFileMetaData end, an additional internal plugin (ExternalExtractor) 
> is provided that forms a conduit between external extractors and the internal 
> API. This plugin looks for executables called 
> kfilemetadata_extractor_ in /usr/bin to find external extractors, 
> and executes them with the --mimetypes arg to find the list of mimetypes each 
> extractor supports. ExternalExtractor then claims to support all of these 
> mimetypes, and then delegates to the extractor executable when doing the 
> actual extraction.
> 
> 
> Diffs
> -
> 
>   README.md 19b1a26 
>   src/extractors/CMakeLists.txt 5dd223e 
>   src/extractors/externalextractor.h PRE-CREATION 
>   src/extractors/externalextractor.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125762/diff/
> 
> 
> Testing
> ---
> 
> Tested with the sample executable file extractor (as attched, written in 
> python) with the dump manual test in KFileMetaData. Works.
> 
> 
> File Attachments
> 
> 
> kfilemetadata_extractor_executable
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/23/146b657f-31d9-4117-a82f-ef966a6339d4__kfilemetadata_extractor_executable
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120393: [kdelibs4support] Kill dead code

[Vishesh Handa]

> On May 26, 2015, 8:05 p.m., Hrvoje Senjan wrote:
> > Ok. Will update diff to kill nepomuk only. What about forwarding headers? 
> > They are still installed

Ping. What's the status on this?


- Vishesh


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


On March 18, 2015, 6:24 p.m., Hrvoje Senjan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120393/
> ---
> 
> (Updated March 18, 2015, 6:24 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Vishesh Handa.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> Strigi check has been removed in commit 
> c8f4c69650c71276b2a2263212addde63764e58b, and soprano wasn't even ported to 
> Qt5 (afaik), so this was never compiled.
> 
> 
> Diffs
> -
> 
>   autotests/kfilemetainfotest.cpp c751cdd 
>   src/CMakeLists.txt b662893 
>   src/config-kdelibs4support.h.cmake 1af3ee0 
>   src/kio/kfilemetadataconfigurationwidget.cpp 259b205 
>   src/kio/kfilemetadataprovider.cpp 3468546 
>   src/kio/kfilemetadataprovider_p.h 31137b2 
>   src/kio/kfilemetadatawidget.cpp 1edb069 
>   src/kio/kfilemetainfo.cpp eae1295 
>   src/kio/kfilemetainfoitem.cpp 62f760d 
>   src/kio/kfilemetainfoitem_p.h 8929e46 
>   src/kio/knfotranslator.cpp 8eec6a1 
> 
> Diff: https://git.reviewboard.kde.org/r/120393/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hrvoje Senjan
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124876: KSycoca: check timestamps and run kbuildsycoca if needed. No kded needed anymore.

[Vishesh Handa]

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



src/sycoca/ksycoca.cpp (line 574)
https://git.reviewboard.kde.org/r/124876/#comment58543

Does the added `(void)` disable some kind of warning?



src/sycoca/ksycoca.cpp (line 592)
https://git.reviewboard.kde.org/r/124876/#comment58544

random thought: Is the return value given by kbuildsyscoca useful?


- Vishesh Handa


On Aug. 28, 2015, 9:41 a.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124876/
 ---
 
 (Updated Aug. 28, 2015, 9:41 a.m.)
 
 
 Review request for KDE Frameworks, Boudewijn Rempt and Vishesh Handa.
 
 
 Repository: kservice
 
 
 Description
 ---
 
 If 1.5s have passed since the last time we checked, we compare the mtime of
 the directories (12 dirs on my system) with the timestamp stored in the
 ksycoca database (which indicates that all changes prior to that time
 are in the DB).
 
 Note that we only check the mtime of dirs, not files. Therefore manually
 editing an installed .desktop file will require touching the dir, or
 running kbuildsycoca5.
 
 
 Diffs
 -
 
   src/sycoca/ksycoca.cpp b7f7abc88db90d784851d91036069e0647fdcbf6 
   src/sycoca/ksycoca_p.h 9f403d6f4be2b406f4985f668176cfa56a5898ea 
 
 Diff: https://git.reviewboard.kde.org/r/124876/diff/
 
 
 Testing
 ---
 
 the kservice unittests still pass
 
 
 Thanks,
 
 David Faure
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124610: do not install namelink for private library

[Vishesh Handa]

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


I have no idea what this does. Someone else will need to give it a ShipIt. 
Also, you probably want to get this in today before the tagging.

- Vishesh Handa


On Aug. 4, 2015, 8:04 a.m., Harald Sitter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124610/
 ---
 
 (Updated Aug. 4, 2015, 8:04 a.m.)
 
 
 Review request for Baloo, Build System and KDE Frameworks.
 
 
 Repository: baloo
 
 
 Description
 ---
 
 please note that the private target is appearing in the resulting cmake
 config all the same and we cannot prevent this because of a cmake oddity
 Alex Merry was kind enough to track down [1].
 testing suggests that this is however not breaking anything (what with the
 library being private).
 
 [1] http://public.kitware.com/pipermail/cmake-developers/2015-July/025798.html
 
 
 Diffs
 -
 
   src/engine/CMakeLists.txt 8e2b5b9c0294a08142f4dc486eecab442167b1ec 
 
 Diff: https://git.reviewboard.kde.org/r/124610/diff/
 
 
 Testing
 ---
 
 make  make install
   no .so symlink installed
 
 made sure there is no symlink on the system and rebuilt gwenview:
   gwenview builds just fine
 
 
 Thanks,
 
 Harald Sitter
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Making Baloo and KFileMetaData a framework

[Vishesh Handa]

Hey guys

Sorry. I should have sent this email earlier.

Would it be all right if both Baloo and KFileMetaData joined
Frameworks for the next release? Currently they are both part of
Plasma.

I had tried to do this last year, but Baloo was GPL because of its
dependency on Xapian. Since then I've replaced Xapian with a custom
full-text-engine, so Baloo is now finally LGPL, and meets the
frameworks criteria.

The public API hasn't really changed since the last time.

--
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124128: KDirWatch: Only establish a connection to FAM if requested

[Vishesh Handa]

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

(Updated July 2, 2015, 5:23 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Edmundson.


Changes
---

Submitted with commit 9319a163b7f8360949bdd1cde12aaf3d7b033132 by Vishesh Handa 
to branch master.


Repository: kcoreaddons


Description
---

KDirWatch by default initializes a connection to both inotify and FAM,
even if we aren't going to be using either. This is unnecessary and the
FAM backend has caused it to block for a while on running FAMOpen.


Diffs
-

  src/lib/io/kdirwatch.cpp 246be82 

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


Testing
---

Unit tests pass


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124128: KDirWatch: Only establish a connection to FAM if requested

[Vishesh Handa]

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

(Updated June 30, 2015, 2:50 p.m.)


Review request for KDE Frameworks and David Edmundson.


Repository: kcoreaddons


Description
---

KDirWatch by default initializes a connection to both inotify and FAM,
even if we aren't going to be using either. This is unnecessary and the
FAM backend has caused it to block for a while on running FAMOpen.


Diffs (updated)
-

  src/lib/io/kdirwatch.cpp 246be82 

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


Testing
---

Unit tests pass


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124128: KDirWatch: Only establish a connection to FAM if requested

[Vishesh Handa]

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

(Updated June 29, 2015, 4:40 p.m.)


Review request for KDE Frameworks and David Edmundson.


Changes
---

Call FAMOpen on first use.


Repository: kcoreaddons


Description
---

KDirWatch by default initializes a connection to both inotify and FAM,
even if we aren't going to be using either. This is unnecessary and the
FAM backend has caused it to block for a while on running FAMOpen.


Diffs (updated)
-

  src/lib/io/kdirwatch.cpp 246be82 

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


Testing
---

Unit tests pass


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124134: Fixing KDED string references

[Vishesh Handa]

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

Ship it!


Looks good.

- Vishesh Handa


On June 20, 2015, 5:26 a.m., David Narváez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124134/
 ---
 
 (Updated June 20, 2015, 5:26 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 Changed to kded5.
 
 
 Diffs
 -
 
   src/kcms/kio/kcookiesmain.cpp c4e36ac69e64f0540167e2a68cd8dadde4333450 
   src/kcms/kio/kcookiesmanagement.cpp 
 c041fc2f2e1a162ced43c944baeec1784e0bcca9 
   src/kcms/kio/kcookiespolicies.cpp 7470616a130decb4261698aa4d4dc3da5c68ec22 
   src/kcms/kio/ksaveioconfig.cpp 82ca6d9c90a58190d912f0f0c791c5e229254ec6 
 
 Diff: https://git.reviewboard.kde.org/r/124134/diff/
 
 
 Testing
 ---
 
 Notice that I was only able to test this with the 5.10 tag since I don't have 
 the rest of the 5.11 frameworks.
 
 Before this patch, if you made sure kded4 was not running and you tried 
 
 $ kcmshell5 cookies
 
 a message would pop up saying the cookies service was not available. After 
 this patch you are able to view and edit your cookies. After this patch I 
 also have ~/.config/kcookiejarrc.
 
 
 Thanks,
 
 David Narváez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 124128: KDirWatch: Only establish a connection to FAM if requested

[Vishesh Handa]

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

Review request for KDE Frameworks and David Edmundson.


Repository: kcoreaddons


Description
---

KDirWatch by default initializes a connection to both inotify and FAM,
even if we aren't going to be using either. This is unnecessary and the
FAM backend has caused it to block for a while on running FAMOpen.


Diffs
-

  src/lib/io/kdirwatch.cpp 246be82929cc08e40bd9eceec017036ec4686c26 

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


Testing
---

Unit tests pass


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124065: KMimeType mimeTypeForNameAndData() - mimeTypeForFileNameAndData()

[Vishesh Handa]

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

Ship it!


- Vishesh Handa


On June 10, 2015, 7:51 p.m., Jarosław Staniek wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124065/
 ---
 
 (Updated June 10, 2015, 7:51 p.m.)
 
 
 Review request for KDE Frameworks and Laurent Montel.
 
 
 Repository: kde-dev-scripts
 
 
 Description
 ---
 
 There's apparent mistake; mimeTypeForNameAndData() does not exist, shall be 
 mimeTypeForFileNameAndData().
 
 
 Diffs
 -
 
   kf5/convert-kmimetype.pl 363ff21cd46331efe209a5051feca58e971739b2 
 
 Diff: https://git.reviewboard.kde.org/r/124065/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jarosław Staniek
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123735: version of QmlObject with a static engine

[Vishesh Handa]

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

Ship it!


Awesome. I love the test!


autotests/quickviewsharedengine.cpp (line 189)
https://git.reviewboard.kde.org/r/123735/#comment55325

Very minor thing -

QCOMPARE(view-status(), QQcmlComponent::Error);
QCOMPARE(view-errors().count(), 1);



src/kdeclarative/qmlobjectsharedengine.cpp (line 48)
https://git.reviewboard.kde.org/r/123735/#comment55326

Could you please add a comment specifying why 2 refs is the magic number.

I understand that it's our own engineRef and s_engine, but it took me a bit 
to figure it out.

How about -
// Delete when we along with the static shared pointer are the only 
references to it.


- Vishesh Handa


On May 21, 2015, 10:52 a.m., Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123735/
 ---
 
 (Updated May 21, 2015, 10:52 a.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 to make easier doing applications like plasma that use a lot of qml to have a 
 single engine make a subclass of QmlObject called QmlObjectSharedEngine that 
 has a single, static QQmlEngine
 
 Adds a class called QuickViewSharedEngine that has the same behavior as 
 QmlObjectSharedEngine(using it): static QQmlEngine, separed rootContexts() 
 for each instance.
 This is used by desktopviews and panelviews to share their engine.
 
 Unfortunately it may not be possible to get the applet configuration dialogs 
 to use this, since they still need a separed engine in order to have a 
 different controls style (qstyle based) than the stuff in the desktop/panel
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt adc1102 
   autotests/quickviewsharedengine.cpp PRE-CREATION 
   autotests/util.h PRE-CREATION 
   autotests/util.cpp PRE-CREATION 
   src/kdeclarative/CMakeLists.txt d73bff0 
   src/kdeclarative/kdeclarative.cpp b3906e2 
   src/kdeclarative/qmlobject.h f26b67d 
   src/kdeclarative/qmlobject.cpp c483665 
   src/kdeclarative/qmlobjectsharedengine.h PRE-CREATION 
   src/kdeclarative/qmlobjectsharedengine.cpp PRE-CREATION 
   src/quickaddons/CMakeLists.txt 777d07c 
   src/quickaddons/quickviewsharedengine.h PRE-CREATION 
   src/quickaddons/quickviewsharedengine.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/123735/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Marco Martin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123735: version of QmlObject with a static engine

[Vishesh Handa]

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



src/quickaddons/quickviewsharedengine.h (line 41)
https://git.reviewboard.kde.org/r/123735/#comment55306

It would be awesome if we could have some tests for this class. 

Maybe we can just try and copy the QQuickView tests? 
http://code.qt.io/cgit/qt/qtdeclarative.git/tree/tests/auto/quick/qquickview/tst_qquickview.cpp


- Vishesh Handa


On May 18, 2015, 8 p.m., Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123735/
 ---
 
 (Updated May 18, 2015, 8 p.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 to make easier doing applications like plasma that use a lot of qml to have a 
 single engine make a subclass of QmlObject called QmlObjectSharedEngine that 
 has a single, static QQmlEngine
 
 Adds a class called QuickViewSharedEngine that has the same behavior as 
 QmlObjectSharedEngine(using it): static QQmlEngine, separed rootContexts() 
 for each instance.
 This is used by desktopviews and panelviews to share their engine.
 
 Unfortunately it may not be possible to get the applet configuration dialogs 
 to use this, since they still need a separed engine in order to have a 
 different controls style (qstyle based) than the stuff in the desktop/panel
 
 
 Diffs
 -
 
   src/kdeclarative/CMakeLists.txt d73bff0 
   src/kdeclarative/kdeclarative.cpp b3906e2 
   src/kdeclarative/qmlobject.h f26b67d 
   src/kdeclarative/qmlobject.cpp c483665 
   src/kdeclarative/qmlobjectsharedengine.h PRE-CREATION 
   src/kdeclarative/qmlobjectsharedengine.cpp PRE-CREATION 
   src/quickaddons/CMakeLists.txt 777d07c 
   src/quickaddons/quickviewsharedengine.h PRE-CREATION 
   src/quickaddons/quickviewsharedengine.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/123735/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Marco Martin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123735: version of QmlObject with a static engine

[Vishesh Handa]

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



src/kdeclarative/qmlobjectsharedengine.cpp (line 41)
https://git.reviewboard.kde.org/r/123735/#comment55280

On first run `s_engine` is 0, so this will not actually clean up the 
`QQmlEngine`



src/kdeclarative/qmlobjectsharedengine.cpp (line 60)
https://git.reviewboard.kde.org/r/123735/#comment55281

I'm probably missing some parts of the picture. Could you please explain 
why this needs to be static?

Also, you could just combine the QSharedPointer and the normal pointer.


- Vishesh Handa


On May 18, 2015, 8 p.m., Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123735/
 ---
 
 (Updated May 18, 2015, 8 p.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 to make easier doing applications like plasma that use a lot of qml to have a 
 single engine make a subclass of QmlObject called QmlObjectSharedEngine that 
 has a single, static QQmlEngine
 
 Adds a class called QuickViewSharedEngine that has the same behavior as 
 QmlObjectSharedEngine(using it): static QQmlEngine, separed rootContexts() 
 for each instance.
 This is used by desktopviews and panelviews to share their engine.
 
 Unfortunately it may not be possible to get the applet configuration dialogs 
 to use this, since they still need a separed engine in order to have a 
 different controls style (qstyle based) than the stuff in the desktop/panel
 
 
 Diffs
 -
 
   src/kdeclarative/CMakeLists.txt d73bff0 
   src/kdeclarative/kdeclarative.cpp b3906e2 
   src/kdeclarative/qmlobject.h f26b67d 
   src/kdeclarative/qmlobject.cpp c483665 
   src/kdeclarative/qmlobjectsharedengine.h PRE-CREATION 
   src/kdeclarative/qmlobjectsharedengine.cpp PRE-CREATION 
   src/quickaddons/CMakeLists.txt 777d07c 
   src/quickaddons/quickviewsharedengine.h PRE-CREATION 
   src/quickaddons/quickviewsharedengine.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/123735/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Marco Martin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123735: version of QmlObject with a static engine

[Vishesh Handa]

 On May 19, 2015, 3:04 p.m., Vishesh Handa wrote:
  src/kdeclarative/qmlobjectsharedengine.cpp, line 60
  https://git.reviewboard.kde.org/r/123735/diff/8/?file=370102#file370102line60
 
  I'm probably missing some parts of the picture. Could you please 
  explain why this needs to be static?
  
  Also, you could just combine the QSharedPointer and the normal pointer.
 
 Marco Martin wrote:
 the whole point is to have a single instance per process...
 or could be done differently, like with a Q_GLOBAL_STATIC...

So does each applet construct its own `QmlObjectSharedEngine`?

Maybe we can disable this class's constructor and just have a `static 
QQmlObjectSharedEngine* instance()` method? It'll make the semantics much 
clearer.


- Vishesh


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


On May 18, 2015, 8 p.m., Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123735/
 ---
 
 (Updated May 18, 2015, 8 p.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 to make easier doing applications like plasma that use a lot of qml to have a 
 single engine make a subclass of QmlObject called QmlObjectSharedEngine that 
 has a single, static QQmlEngine
 
 Adds a class called QuickViewSharedEngine that has the same behavior as 
 QmlObjectSharedEngine(using it): static QQmlEngine, separed rootContexts() 
 for each instance.
 This is used by desktopviews and panelviews to share their engine.
 
 Unfortunately it may not be possible to get the applet configuration dialogs 
 to use this, since they still need a separed engine in order to have a 
 different controls style (qstyle based) than the stuff in the desktop/panel
 
 
 Diffs
 -
 
   src/kdeclarative/CMakeLists.txt d73bff0 
   src/kdeclarative/kdeclarative.cpp b3906e2 
   src/kdeclarative/qmlobject.h f26b67d 
   src/kdeclarative/qmlobject.cpp c483665 
   src/kdeclarative/qmlobjectsharedengine.h PRE-CREATION 
   src/kdeclarative/qmlobjectsharedengine.cpp PRE-CREATION 
   src/quickaddons/CMakeLists.txt 777d07c 
   src/quickaddons/quickviewsharedengine.h PRE-CREATION 
   src/quickaddons/quickviewsharedengine.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/123735/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Marco Martin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123341: Optimize reading Sonnet settings by minimizing the cals to save()

[Vishesh Handa]

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



autotests/test_settings.cpp (line 43)
https://git.reviewboard.kde.org/r/123341/#comment55282

Minor thing, which is up to you.

In this test you're allocating the `Speller` off the heap and having to 
delete it. In the other test you're allocating it off the stack.


- Vishesh Handa


On May 12, 2015, 6:03 p.m., Kåre Särs wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123341/
 ---
 
 (Updated May 12, 2015, 6:03 p.m.)
 
 
 Review request for KDE Frameworks, Laurent Montel and Martin Tobias Holmedahl 
 Sandsmark.
 
 
 Repository: sonnet
 
 
 Description
 ---
 
 The curent implementation can causes ~25 cals to Settings::save() for every 
 created Speller instance. 
 
 The Settings::restore() function ends with setQuietIgnoreList(...) which 
 would call save() for evey ignore-word.
 
 This patch removes the save() cals in the setFoo() functions of Settings and 
 replace it by a boolean that indicates if the settign is changed or not. Then 
 in the Speller class save() is called when needed.
 
 The reason for this patch is that it took Kate ~2 minutes to open a kate 
 session with ~340 files. The implementation in Kate called reload() for every 
 file. There is a RR for Kate to remove that unneeded reload.
 
 This patch also prepares Sonnet::Settings for being used in the public API to 
 set aplication specific Sonnet settings without saving them in the global 
 settings.
 
 The Settings class is exported but the header is private. This change is BIC, 
 but since the class is private, my interpretation is that it should not mater.
 
 If accepted I will also add a Review Request to make the Settings class 
 public to enable application specific settings.
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt 8ade413 
   autotests/test_settings.h PRE-CREATION 
   autotests/test_settings.cpp PRE-CREATION 
   src/core/settings.cpp b5c4198 
   src/core/settings_p.h 0d48889 
   src/core/speller.cpp 3903b42 
   src/ui/configwidget.cpp 02f2a26 
 
 Diff: https://git.reviewboard.kde.org/r/123341/diff/
 
 
 Testing
 ---
 
 Tested with Kate and with the patch the startup time was back to normal. 
 Two unit tests added.
 
 
 Thanks,
 
 Kåre Särs
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Kross module loading is broken since KF5.0.0

[Vishesh Handa]

On Wed, May 13, 2015 at 2:42 PM, Alexander Potashev aspotas...@gmail.com
wrote:

 But nobody will ever link to a Kross plugin bypassing the standard loader
 for Kross modules.

 That said, I think it should be absolutely fine if we rename all Kross
 modules back to lowercase spelling and be happy again.

 What do you think?


Ok. Though I was hoping someone else would answer.

We currently don't have a maintainer for Kross. So please make a review
request with the changes, you can wait a week or so for someone to object,
and then just ship it.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Kross module loading is broken since KF5.0.0

[Vishesh Handa]

On Mon, May 11, 2015 at 6:37 PM, Alexander Potashev aspotas...@gmail.com
wrote:

  3. Make module names case insensitive. This can be implemented in
 Kross framework by listing and filtering directory contents instead of
 checking for a specific file. It will be slower and this looks like a
 hack to me.


I would go for this one. It's trivial to implement and the speed difference
would be minuscule.




-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Vishesh Handa]

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



autotests/kconfig_compiler/test_signal.h.ref (line 121)
https://git.reviewboard.kde.org/r/123367/#comment54187

Is the move required?



src/kconfig_compiler/kconfig_compiler.cpp (line 1722)
https://git.reviewboard.kde.org/r/123367/#comment54188

It might be more readable, if you created a new variable called 
'requiresMoc' or 'requiresQObjectMacro'.


- Vishesh Handa


On April 15, 2015, 3:38 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 15, 2015, 3:38 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Vishesh Handa]

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

Ship it!


- Vishesh Handa


On April 22, 2015, 2:51 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 2:51 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123402: Moves away from KService

[Vishesh Handa]

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



src/widgets/actions.cpp (line 34)
https://git.reviewboard.kde.org/r/123402/#comment54080

If you want you can also change this to the new for syntax.


- Vishesh Handa


On April 17, 2015, 3:45 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123402/
 ---
 
 (Updated April 17, 2015, 3:45 p.m.)
 
 
 Review request for KDE Frameworks, KDEPIM and Martin Klapetek.
 
 
 Repository: kpeople
 
 
 Description
 ---
 
 Using KPluginFactory for instantiating the plugins, maybe we could do a bit 
 better there and just use QPluginLoader which is what we seem to need on most 
 cases. See discussion on depending patch.
 
 Tier 3 - Tier 2
 
 
 Diffs
 -
 
   CMakeLists.txt 11e2aa6 
   metainfo.yaml 9795646 
   src/CMakeLists.txt 225cee5 
   src/personpluginmanager.cpp 9dd3f6e 
   src/widgets/actions.cpp ed7e02c 
   src/widgets/persondetailsview.cpp 8a2ef5c 
 
 Diff: https://git.reviewboard.kde.org/r/123402/diff/
 
 
 Testing
 ---
 
 My KTP contact list still works, tests still pass and example applications 
 still work as well.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120393: [kdelibs4support] Kill dead code

[Vishesh Handa]

 On March 18, 2015, 11:23 p.m., Vishesh Handa wrote:
  I'm all for getting rid of the Nepomuk code. However, I'm not too sure 
  about the strigi part. That should still work.
 
 Hrvoje Senjan wrote:
 It does not ;-)
 Originally, this review added back the find_package(Strigi) call which 
 was removed a while back (at least before 5.0.0), so this code was/is never 
 compiled.
 
 Vishesh Handa wrote:
 I still cannot give it a ship it.
 
 
 We need to collectively decide if we want to let the Strigi integration 
 be broken and remove the code. Or add the dependency again and see why it 
 doesn't work.
 
 Albert Astals Cid wrote:
 Agreeing with Vishesh here, can you send a new email to kde-core-devel 
 mailing list mentioning what should we do, if stop supporting strigi or not 
 in kdelibs4support? This way we can get a more project wide discussion about 
 it.
 
 Hrvoje Senjan wrote:
 Maybe there was a misunderstanding - i do not know do strigi related code 
 works if compiled - problem was/is they where never compiled since, and 
 before 5.0.0.
 Anyway, i'll compose a k-c-d mail laters...

Ping.

If you can split up just the Nepomuk parts, it's a ship it from me.


- Vishesh


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


On March 18, 2015, 6:24 p.m., Hrvoje Senjan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120393/
 ---
 
 (Updated March 18, 2015, 6:24 p.m.)
 
 
 Review request for KDE Frameworks, David Faure and Vishesh Handa.
 
 
 Repository: kdelibs4support
 
 
 Description
 ---
 
 Strigi check has been removed in commit 
 c8f4c69650c71276b2a2263212addde63764e58b, and soprano wasn't even ported to 
 Qt5 (afaik), so this was never compiled.
 
 
 Diffs
 -
 
   autotests/kfilemetainfotest.cpp c751cdd 
   src/CMakeLists.txt b662893 
   src/config-kdelibs4support.h.cmake 1af3ee0 
   src/kio/kfilemetadataconfigurationwidget.cpp 259b205 
   src/kio/kfilemetadataprovider.cpp 3468546 
   src/kio/kfilemetadataprovider_p.h 31137b2 
   src/kio/kfilemetadatawidget.cpp 1edb069 
   src/kio/kfilemetainfo.cpp eae1295 
   src/kio/kfilemetainfoitem.cpp 62f760d 
   src/kio/kfilemetainfoitem_p.h 8929e46 
   src/kio/knfotranslator.cpp 8eec6a1 
 
 Diff: https://git.reviewboard.kde.org/r/120393/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Hrvoje Senjan
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120393: [kdelibs4support] Kill dead code

[Vishesh Handa]

 On March 18, 2015, 11:23 p.m., Vishesh Handa wrote:
  I'm all for getting rid of the Nepomuk code. However, I'm not too sure 
  about the strigi part. That should still work.
 
 Hrvoje Senjan wrote:
 It does not ;-)
 Originally, this review added back the find_package(Strigi) call which 
 was removed a while back (at least before 5.0.0), so this code was/is never 
 compiled.

I still cannot give it a ship it.


We need to collectively decide if we want to let the Strigi integration be 
broken and remove the code. Or add the dependency again and see why it doesn't 
work.


- Vishesh


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


On March 18, 2015, 6:24 p.m., Hrvoje Senjan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120393/
 ---
 
 (Updated March 18, 2015, 6:24 p.m.)
 
 
 Review request for KDE Frameworks, David Faure and Vishesh Handa.
 
 
 Repository: kdelibs4support
 
 
 Description
 ---
 
 Strigi check has been removed in commit 
 c8f4c69650c71276b2a2263212addde63764e58b, and soprano wasn't even ported to 
 Qt5 (afaik), so this was never compiled.
 
 
 Diffs
 -
 
   autotests/kfilemetainfotest.cpp c751cdd 
   src/CMakeLists.txt b662893 
   src/config-kdelibs4support.h.cmake 1af3ee0 
   src/kio/kfilemetadataconfigurationwidget.cpp 259b205 
   src/kio/kfilemetadataprovider.cpp 3468546 
   src/kio/kfilemetadataprovider_p.h 31137b2 
   src/kio/kfilemetadatawidget.cpp 1edb069 
   src/kio/kfilemetainfo.cpp eae1295 
   src/kio/kfilemetainfoitem.cpp 62f760d 
   src/kio/kfilemetainfoitem_p.h 8929e46 
   src/kio/knfotranslator.cpp 8eec6a1 
 
 Diff: https://git.reviewboard.kde.org/r/120393/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Hrvoje Senjan
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120393: [kdelibs4support] Kill dead code

[Vishesh Handa]

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


I'm all for getting rid of the Nepomuk code. However, I'm not too sure about 
the strigi part. That should still work.

- Vishesh Handa


On March 18, 2015, 6:24 p.m., Hrvoje Senjan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120393/
 ---
 
 (Updated March 18, 2015, 6:24 p.m.)
 
 
 Review request for KDE Frameworks, David Faure and Vishesh Handa.
 
 
 Repository: kdelibs4support
 
 
 Description
 ---
 
 Strigi check has been removed in commit 
 c8f4c69650c71276b2a2263212addde63764e58b, and soprano wasn't even ported to 
 Qt5 (afaik), so this was never compiled.
 
 
 Diffs
 -
 
   autotests/kfilemetainfotest.cpp c751cdd 
   src/CMakeLists.txt b662893 
   src/config-kdelibs4support.h.cmake 1af3ee0 
   src/kio/kfilemetadataconfigurationwidget.cpp 259b205 
   src/kio/kfilemetadataprovider.cpp 3468546 
   src/kio/kfilemetadataprovider_p.h 31137b2 
   src/kio/kfilemetadatawidget.cpp 1edb069 
   src/kio/kfilemetainfo.cpp eae1295 
   src/kio/kfilemetainfoitem.cpp 62f760d 
   src/kio/kfilemetainfoitem_p.h 8929e46 
   src/kio/knfotranslator.cpp 8eec6a1 
 
 Diff: https://git.reviewboard.kde.org/r/120393/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Hrvoje Senjan
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122621: Implemented SlaveBase::GetFileSystemFreeSpace in sftp ioslave

[Vishesh Handa]

 On Feb. 25, 2015, 10:24 a.m., Vishesh Handa wrote:
  sftp/kio_sftp.cpp, line 247
  https://git.reviewboard.kde.org/r/122621/diff/1/?file=350141#file350141line247
 
  The code indentation seems wrong.
 
 Emmanuel Pescosta wrote:
 Just followed the coding style (1 tab - 2 spaces) from kio_sftp.cpp
 
 Should I use the frameworks coding style instead?

Ah. I didn't see the rest of the file. Probbaly best to follow it, like you 
have :)


- Vishesh


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


On Feb. 18, 2015, 11:14 a.m., Emmanuel Pescosta wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122621/
 ---
 
 (Updated Feb. 18, 2015, 11:14 a.m.)
 
 
 Review request for KDE Frameworks and Andreas Schneider.
 
 
 Repository: kio-extras
 
 
 Description
 ---
 
 Implemented SlaveBase::GetFileSystemFreeSpace to enable free space 
 information in sftp ioslave.
 
 
 Diffs
 -
 
   sftp/kio_sftp.h e088472 
   sftp/kio_sftp.cpp 057fcd4 
 
 Diff: https://git.reviewboard.kde.org/r/122621/diff/
 
 
 Testing
 ---
 
 Works
 
 
 Thanks,
 
 Emmanuel Pescosta
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KDE Telepathy has an unreleased dependency

[Vishesh Handa]

On Wed, Feb 25, 2015 at 6:39 PM, Martin Klapetek martin.klape...@gmail.com
wrote:

 As I said it's not even being used right now, so imho would be fine if it
 got
 removed/disabled altogether.


Also, it will never work. Baloo KF5, has no knowledge about emails. That
code also uses Akonadi APIs.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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



src/currency.cpp
https://git.reviewboard.kde.org/r/122183/#comment52534

I'm really not sure what to do if we cannot open the file. Abort? Print out 
an error message? Or just silently ignore it like right now.


- Vishesh Handa


On Feb. 17, 2015, 11:22 a.m., Vishesh Handa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122183/
 ---
 
 (Updated Feb. 17, 2015, 11:22 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 340819
 https://bugs.kde.org/show_bug.cgi?id=340819
 
 
 Repository: kunitconversion
 
 
 Description
 ---
 
 Currency: Fetch the currency file properly
 
 Properly run an event loop and wait for the file to be fetched.
 
 Also add a test to make sure currency conversion is working.
 
 This patch also contains -
 * https://git.reviewboard.kde.org/r/122182/
 * https://git.reviewboard.kde.org/r/122181/
 * https://git.reviewboard.kde.org/r/122180/
 
 This is because reviewboard refuses to upload only a part of the diff. Please 
 only look at currency.cpp w.r.t the EventLoop.
 
 
 Diffs
 -
 
   src/currency.cpp 715233c 
   autotests/convertertest.h 8129a48 
   autotests/convertertest.cpp ae4298e 
 
 Diff: https://git.reviewboard.kde.org/r/122183/diff/
 
 
 Testing
 ---
 
 Test now passes.
 
 
 Thanks,
 
 Vishesh Handa
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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

(Updated Feb. 17, 2015, 11:22 a.m.)


Review request for KDE Frameworks.


Bugs: 340819
https://bugs.kde.org/show_bug.cgi?id=340819


Repository: kunitconversion


Description
---

Currency: Fetch the currency file properly

Properly run an event loop and wait for the file to be fetched.

Also add a test to make sure currency conversion is working.

This patch also contains -
* https://git.reviewboard.kde.org/r/122182/
* https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/

This is because reviewboard refuses to upload only a part of the diff. Please 
only look at currency.cpp w.r.t the EventLoop.


Diffs (updated)
-

  src/currency.cpp 715233c 
  autotests/convertertest.h 8129a48 
  autotests/convertertest.cpp ae4298e 

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


Testing
---

Test now passes.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122562: Add method for converting from QVariant to base64

[Vishesh Handa]

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



src/qmlcontrols/kquickcontrolsaddons/clipboard.cpp
https://git.reviewboard.kde.org/r/122562/#comment52519

Coding style



src/qmlcontrols/kquickcontrolsaddons/clipboard.cpp
https://git.reviewboard.kde.org/r/122562/#comment52520

huh?

The `case QVariant::String` does not matter.


- Vishesh Handa


On Feb. 13, 2015, 6:16 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122562/
 ---
 
 (Updated Feb. 13, 2015, 6:16 p.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 I need some method like that over at the QuickShare plasmoid.
 
 I'm unsure where to put it, anybody knows of a good place?
 Otherwise I guess it works fine with the clipboard, that returns QVariant.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/clipboard.cpp 23ce29b 
   src/qmlcontrols/kquickcontrolsaddons/clipboard.h ac89b9d 
 
 Diff: https://git.reviewboard.kde.org/r/122562/diff/
 
 
 Testing
 ---
 
 Adopted locally in my QuickShare plasmoid implementation
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122453: Improve KPeople documentation, in order to become a Framework

[Vishesh Handa]

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



metainfo.yaml
https://git.reviewboard.kde.org/r/122453/#comment52234

Perhaps you want to add your name over here?


- Vishesh Handa


On Feb. 6, 2015, 2:14 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122453/
 ---
 
 (Updated Feb. 6, 2015, 2:14 p.m.)
 
 
 Review request for KDE Frameworks, KDEPIM and Telepathy.
 
 
 Repository: libkpeople
 
 
 Description
 ---
 
 Adds a README.md and a metainfo.yaml.
 It also documents some classes and methods that didn't have any documentation.
 
 
 Diffs
 -
 
   src/widgets/CMakeLists.txt bd2cd7e 
   src/widgets/mergedialog.h 305d07b 
   src/widgets/persondetailsview.h 0769d51 
   CMakeLists.txt 00177f3 
   README.md PRE-CREATION 
   metainfo.yaml PRE-CREATION 
   src/CMakeLists.txt 7e01976 
   src/duplicatesfinder_p.h 05f0063 
   src/persondata.h 0fab902 
   src/personsmodel.h 32dc1f5 
 
 Diff: https://git.reviewboard.kde.org/r/122453/diff/
 
 
 Testing
 ---
 
 Unit tests still pass ;).
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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

(Updated Feb. 3, 2015, 12:49 p.m.)


Review request for KDE Frameworks.


Changes
---

Fixed the minor problems


Bugs: 340819
https://bugs.kde.org/show_bug.cgi?id=340819


Repository: kunitconversion


Description
---

Currency: Fetch the currency file properly

Properly run an event loop and wait for the file to be fetched.

Also add a test to make sure currency conversion is working.

This patch also contains -
* https://git.reviewboard.kde.org/r/122182/
* https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/

This is because reviewboard refuses to upload only a part of the diff. Please 
only look at currency.cpp w.r.t the EventLoop.


Diffs (updated)
-

  autotests/convertertest.h 8129a48 
  autotests/convertertest.cpp ae4298e 
  src/currency.cpp 715233c 

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


Testing
---

Test now passes.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

 On Jan. 23, 2015, 9:02 p.m., David Faure wrote:
  src/currency.cpp, line 673
  https://git.reviewboard.kde.org/r/122183/diff/2/?file=344361#file344361line673
 
  pass 'this' as 3rd argument, just in case 'this' gets deleted while the 
  job is running?

'this' isn't a QObject in this case, so I cannot do that.


- Vishesh


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


On Jan. 23, 2015, 2:17 p.m., Vishesh Handa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122183/
 ---
 
 (Updated Jan. 23, 2015, 2:17 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 340819
 https://bugs.kde.org/show_bug.cgi?id=340819
 
 
 Repository: kunitconversion
 
 
 Description
 ---
 
 Currency: Fetch the currency file properly
 
 Properly run an event loop and wait for the file to be fetched.
 
 Also add a test to make sure currency conversion is working.
 
 This patch also contains -
 * https://git.reviewboard.kde.org/r/122182/
 * https://git.reviewboard.kde.org/r/122181/
 * https://git.reviewboard.kde.org/r/122180/
 
 This is because reviewboard refuses to upload only a part of the diff. Please 
 only look at currency.cpp w.r.t the EventLoop.
 
 
 Diffs
 -
 
   autotests/convertertest.h 8129a48 
   autotests/convertertest.cpp ae4298e 
   src/currency.cpp 715233c 
 
 Diff: https://git.reviewboard.kde.org/r/122183/diff/
 
 
 Testing
 ---
 
 Test now passes.
 
 
 Thanks,
 
 Vishesh Handa
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Build failed in Jenkins: kde-baseapps_master_qt5 #213

[Vishesh Handa]

On Wed, Jan 28, 2015 at 4:24 PM, David Faure fa...@kde.org wrote:

 On Wednesday 28 January 2015 14:41:04 KDE CI System wrote:
  [ 74%] Building CXX object
 
 dolphin/src/CMakeFiles/kcm_dolphinnavigation.dir/dolphin_generalsettings.cp
  p.o
  
 http://build.kde.org/job/kde-baseapps_master_qt5/ws/dolphin/src/search/dol
  phinsearchbox.cpp:42:44: fatal error: Baloo/NaturalFileQueryParser: No
 such
  file or directory #include Baloo/NaturalFileQueryParser
  ^
  compilation terminated.

 Hi Vishesh,

 Any idea what changed in baloo that causes this CI error?

 Missing dependency maybe?


Yes. I moved the file from baloo to baloo-widgets. Let me try to fix it.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Vishesh Handa]

 On Jan. 23, 2015, 8:58 p.m., David Faure wrote:
  Not sure why this is suddenly triggering many philosophical discussions 
  about what users should be doing (IMHO, give them choice, this is 
  opensource).
  Similar changes have been done in most other frameworks long ago, this is 
  most certainly consistent.
  (see e.g. kcoreaddons - the find_package(Qt5Test) is being done in the 
  autotests subfolder, the same could be done here).
 
 Vishesh Handa wrote:
 It's not about users, it's about developers/packagers. We're not forcing 
 them to run the tests, we're telling them to compile them. They always have a 
 choice by modifying the code, we as upstream shouldn't be making it easier 
 for them to skip the most of basic of quality tests.
 
 Martin Gräßlin wrote:
 @Vishesh: as I went through the same thing I point you to 
 https://git.reviewboard.kde.org/r/117393/ - it was a discussion for the same 
 on KWin and now I think it was good that we accepted the solution in the end. 
 Maybe the reasoning provided there helps to understand the packagers needs.

@Martin: I do prefer the solution in the kwin patch. This one is really messy.

Though I still don't agree with their reasons.


- Vishesh


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


On Jan. 22, 2015, 7:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 7:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Vishesh Handa]

 On Jan. 23, 2015, 8:58 p.m., David Faure wrote:
  Not sure why this is suddenly triggering many philosophical discussions 
  about what users should be doing (IMHO, give them choice, this is 
  opensource).
  Similar changes have been done in most other frameworks long ago, this is 
  most certainly consistent.
  (see e.g. kcoreaddons - the find_package(Qt5Test) is being done in the 
  autotests subfolder, the same could be done here).

It's not about users, it's about developers/packagers. We're not forcing them 
to run the tests, we're telling them to compile them. They always have a choice 
by modifying the code, we as upstream shouldn't be making it easier for them to 
skip the most of basic of quality tests.


- Vishesh


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


On Jan. 22, 2015, 7:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 7:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: backwards compatibility how namespacing might help or not

[Vishesh Handa]

On Tue, Jan 27, 2015 at 10:28 AM, Harald Sitter sit...@kde.org wrote:

  So yes, a better way of moving thing arounds is needed, or maybe we just
 need
  the dust to settle a bit so that we realize where each thing really
 belongs
  and it'll hopefully fix itself?

 Waiting for the dust to settle is what I have been told for almost a
 year now.


Frameworks hasn't even been out for a year. 6 months isn't a lot of time.


 How much more dust can there possibly be? :P
 The problem IMO is that supposedly the existing things will settle at
 some point, that doesn't mean a new thing couldn't end up in some
 plasma repo for 5.7 and then be moved into a framework 5.130 because
 it suddenly fits better into frameworks.


Are you trying to say that if something is once released part of Plasma it
should never be part of KDE Frameworks?

Also, this is going to happen again with KF 5.9.

-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Vishesh Handa]

 On Jan. 23, 2015, 1:43 p.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.
 
 Matthew Dawson wrote:
 This is mostly useful on source based distributions (specifically, this 
 patch comes from Gentoo).  While in general running tests everywhere would be 
 great, source distro users may not have the cpu time to compile/run tests.  
 Also, some test suites don't work and users may not care to figure out why 
 (for instance, last time I tried enabling tests in Gentoo, binutils failed 
 its suite).
 
 For binary distriubtions, I agree they should be running tests 
 (especially since we work to keep them green).  But source based distros 
 aren't so clear cut.
 
 Andreas Sturmlechner wrote:
 Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 Vishesh Handa wrote:
 It is even more important for source based distros to be running tests. 
 They generally have very different compile options and flags. What is the 
 point of them running the software and possibly finding bugs, when it could 
 have been caught by just running the tests.
 
 Actually, the more I think about this, the more I realize that everyone 
 should be running the autotests. -2 from my side. But I'm not the maintainer 
 of kio.
 
 Vishesh Handa wrote:
  Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 On a source based distribution you already have a dependnecy on cmake, 
 the compiler, and many other things. These are only required during build 
 time. Qt5Test is the same. Once the pacakge has been built + tests have been 
 run, Qt5Test can be removed.
 
 Matthew Dawson wrote:
 At least on Gentoo, by default build time dependencies are not 
 automatically removed (though you can remove them if you want).  Generally 
 speaking that is the right choice, as you will need cmake/compiler/etc. later 
 to build the package when a version is released.  Also, one of the benefits 
 of Gentoo is that the entire development toolchain sticks around, allowing 
 for easy development/bug triaging.
 
 Anyways, source based distros won't always run tests, because users won't 
 always want to run them.  In a perfect world, I agree that is wrong.  In 
 reality, I don't run any test suites across any of my Gentoo installs.  So 
 forcing the tests to build just burns CPU time, and is easily patched out by 
 downstreams.  I don't think trying to force this will get KDE anywhere.

 Anyways, source based distros won't always run tests, because users won't 
 always want to run them.  In a perfect world, I agree that is wrong.  In 
 reality, I don't run any test suites across any of my Gentoo installs.  So 
 forcing the tests to build just burns CPU time, and is easily patched out by 
 downstreams.  I don't think trying to force this will get KDE anywhere.

- If the user doesn't want to run them, I'm sure Gentoo can provide some 
options for that. Compiling them cannot be such a huge cost.
- They are already burning CPU time by using a source based distro. This way 
they might actually catch some bugs and possibly not waste developers time by 
filing bugs which may have been an issue with their system.

I'm not sure if I will approve such patches on packages I maintain.


- Vishesh


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


On Jan. 22, 2015, 7:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 7:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Vishesh Handa]

 On Jan. 23, 2015, 1:43 p.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.
 
 Matthew Dawson wrote:
 This is mostly useful on source based distributions (specifically, this 
 patch comes from Gentoo).  While in general running tests everywhere would be 
 great, source distro users may not have the cpu time to compile/run tests.  
 Also, some test suites don't work and users may not care to figure out why 
 (for instance, last time I tried enabling tests in Gentoo, binutils failed 
 its suite).
 
 For binary distriubtions, I agree they should be running tests 
 (especially since we work to keep them green).  But source based distros 
 aren't so clear cut.
 
 Andreas Sturmlechner wrote:
 Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.

It is even more important for source based distros to be running tests. They 
generally have very different compile options and flags. What is the point of 
them running the software and possibly finding bugs, when it could have been 
caught by just running the tests.

Actually, the more I think about this, the more I realize that everyone should 
be running the autotests. -2 from my side. But I'm not the maintainer of kio.


- Vishesh


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


On Jan. 22, 2015, 7:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 7:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Vishesh Handa]

 On Jan. 23, 2015, 1:43 p.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.
 
 Matthew Dawson wrote:
 This is mostly useful on source based distributions (specifically, this 
 patch comes from Gentoo).  While in general running tests everywhere would be 
 great, source distro users may not have the cpu time to compile/run tests.  
 Also, some test suites don't work and users may not care to figure out why 
 (for instance, last time I tried enabling tests in Gentoo, binutils failed 
 its suite).
 
 For binary distriubtions, I agree they should be running tests 
 (especially since we work to keep them green).  But source based distros 
 aren't so clear cut.
 
 Andreas Sturmlechner wrote:
 Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 Vishesh Handa wrote:
 It is even more important for source based distros to be running tests. 
 They generally have very different compile options and flags. What is the 
 point of them running the software and possibly finding bugs, when it could 
 have been caught by just running the tests.
 
 Actually, the more I think about this, the more I realize that everyone 
 should be running the autotests. -2 from my side. But I'm not the maintainer 
 of kio.

 Exactly, packagers do build the tests of course, but that does not mean users 
 of source packages should have a permanent dependency on Qt5Test.

On a source based distribution you already have a dependnecy on cmake, the 
compiler, and many other things. These are only required during build time. 
Qt5Test is the same. Once the pacakge has been built + tests have been run, 
Qt5Test can be removed.


- Vishesh


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


On Jan. 22, 2015, 7:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 7:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Vishesh Handa]

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


I'm not against this, but I am curious as to why this is being done. 

I would think that packagers should be building the tests and running them on 
their platform and make sure everything passes. We have a strict policy that 
all tests must always pass.

- Vishesh Handa


On Jan. 22, 2015, 7:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 7:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122180: [KUnitConversion] Unit::setUnitMultiplier: Do not call oneself

[Vishesh Handa]

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

(Updated Jan. 23, 2015, 12:10 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kunitconversion


Description
---

KUnitConversion: Unit::setUnitMultiplier: Do not call oneself


Diffs
-

  src/unit.cpp 013b5d7 

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


Testing
---

Created a test case in another patch. We no longer crash.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122181: [KUnitConversion] UnitCategory convert: Call UnitCategoryPrivate instead of duplicating it

[Vishesh Handa]

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

(Updated Jan. 23, 2015, 12:10 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kunitconversion


Description
---


  This is probably a mistake when implementing the private class. Both
  UnitCategoryPrivate::convert and UnitCategory::convert essentially
  the same thing. However, all sub categories modify the
  UnitCategoryPrivate virtual method, so we should be calling that one.


  This was caught while trying to debug the currency converter. It has its
  own custom convert function.


Diffs
-

  src/unitcategory.cpp c34217e 

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


Testing
---

Fixes a test in another patch.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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

(Updated Jan. 23, 2015, 2:17 p.m.)


Review request for KDE Frameworks.


Changes
---

This patch actually makes the test fail without changing the default values 
(another patch on reviewboard)


Bugs: 340819
https://bugs.kde.org/show_bug.cgi?id=340819


Repository: kunitconversion


Description
---

Currency: Fetch the currency file properly

Properly run an event loop and wait for the file to be fetched.

Also add a test to make sure currency conversion is working.

This patch also contains -
* https://git.reviewboard.kde.org/r/122182/
* https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/

This is because reviewboard refuses to upload only a part of the diff. Please 
only look at currency.cpp w.r.t the EventLoop.


Diffs (updated)
-

  autotests/convertertest.h 8129a48 
  autotests/convertertest.cpp ae4298e 
  src/currency.cpp 715233c 

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


Testing
---

Test now passes.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122182: [KUnitConversion] Currency: Give default values for various currencies

[Vishesh Handa]

 On Jan. 22, 2015, 12:22 p.m., Martin Gräßlin wrote:
  would it be possible to return an error if there is no useable value?
  
  We can update now, but in a month or so the values could be completely 
  different again (c.f. EUR - CHF from two weeks ago and today).

Given the way KUnitConversion is structured it would not be a trivial change.

Please note that this case will only ever occur if you use currency conversion 
the first time and there is no network access. The moment we have network 
access a more accurate currency file will be downloaded.


- Vishesh


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


On Jan. 21, 2015, 2:47 p.m., Vishesh Handa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122182/
 ---
 
 (Updated Jan. 21, 2015, 2:47 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kunitconversion
 
 
 Description
 ---
 
 The currency converter works by fetching a file from the network every
 day and using those conversion values. If network is not available it
 will try and use the previous version of that file. If no file 
 then it will use the default values.
 
 The default values was 1e+99 in all cases. This patch updates it to the
 current values as reflected in the file.
 
 
 Diffs
 -
 
   src/currency.cpp 715233c 
 
 Diff: https://git.reviewboard.kde.org/r/122182/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Vishesh Handa
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

 On Jan. 22, 2015, 12:45 p.m., David Faure wrote:
  src/currency.cpp, line 672
  https://git.reviewboard.kde.org/r/122183/diff/1/?file=343932#file343932line672
 
  Ouch.
  
  Hello unexpected reentrancy.
  
  If this code is async, it should provide a Job API instead of 
  masquerading under a sync API with a nasty nested event loop.
  
  Or is this running in a separate thread?

It isn't running in a separate thread.

Hmm. This is strange. The previous code is as follows -
QNetworkReply *reply = manager.get(QNetworkRequest(QUrl(URL)));
QFile cacheFile(m_cache);
cacheFile.open(QFile::WriteOnly);
while (!reply-error()  !reply-atEnd()) {
if (reply-bytesAvailable()0 || reply-waitForReadyRead(500)) {
cacheFile.write(reply-readAll());
}
}

QNetworkReply::waitForReadyRead is not implemented, so it uses 
QIODeivce::waitForReadyRead which just returns false. So this code basicallly 
keeps looping until we get a reply.

Possible ways to fix this -
1. Event Loop but only process some events
2. A loop as before
3. Async code where we fetch the currency rates in the background and for this 
result we convert it with the previous rates.

I think (3) would be the best option. Opinions?


- Vishesh


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


On Jan. 21, 2015, 2:47 p.m., Vishesh Handa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122183/
 ---
 
 (Updated Jan. 21, 2015, 2:47 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 340819
 https://bugs.kde.org/show_bug.cgi?id=340819
 
 
 Repository: kunitconversion
 
 
 Description
 ---
 
 Currency: Fetch the currency file properly
 
 Properly run an event loop and wait for the file to be fetched.
 
 Also add a test to make sure currency conversion is working.
 
 This patch also contains -
 * https://git.reviewboard.kde.org/r/122182/
 * https://git.reviewboard.kde.org/r/122181/
 * https://git.reviewboard.kde.org/r/122180/
 
 This is because reviewboard refuses to upload only a part of the diff. Please 
 only look at currency.cpp w.r.t the EventLoop.
 
 
 Diffs
 -
 
   autotests/convertertest.h 8129a48 
   autotests/convertertest.cpp ae4298e 
   src/currency.cpp 715233c 
   src/unit.cpp 013b5d7 
   src/unitcategory.cpp c34217e 
 
 Diff: https://git.reviewboard.kde.org/r/122183/diff/
 
 
 Testing
 ---
 
 Test now passes.
 
 
 Thanks,
 
 Vishesh Handa
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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

Review request for KDE Frameworks.


Bugs: 340819
https://bugs.kde.org/show_bug.cgi?id=340819


Repository: kunitconversion


Description
---

Currency: Fetch the currency file properly

Properly run an event loop and wait for the file to be fetched.

Also add a test to make sure currency conversion is working.

This patch also contains -
* https://git.reviewboard.kde.org/r/122182/
* https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/

This is because reviewboard refuses to upload only a part of the diff. Please 
only look at currency.cpp w.r.t the EventLoop.


Diffs
-

  autotests/convertertest.h 8129a48 
  autotests/convertertest.cpp ae4298e 
  src/currency.cpp 715233c 
  src/unit.cpp 013b5d7 
  src/unitcategory.cpp c34217e 

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


Testing
---

Test now passes.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 122180: [KUnitConversion] Unit::setUnitMultiplier: Do not call oneself

[Vishesh Handa]

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

Review request for KDE Frameworks.


Repository: kunitconversion


Description
---

KUnitConversion: Unit::setUnitMultiplier: Do not call oneself


Diffs
-

  src/unit.cpp 013b5d7 

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


Testing
---

Created a test case in another patch. We no longer crash.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 122181: [KUnitConversion] UnitCategory convert: Call UnitCategoryPrivate instead of duplicating it

[Vishesh Handa]

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

Review request for KDE Frameworks.


Repository: kunitconversion


Description
---


  This is probably a mistake when implementing the private class. Both
  UnitCategoryPrivate::convert and UnitCategory::convert essentially
  the same thing. However, all sub categories modify the
  UnitCategoryPrivate virtual method, so we should be calling that one.


  This was caught while trying to debug the currency converter. It has its
  own custom convert function.


Diffs
-

  src/unitcategory.cpp c34217e 

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


Testing
---

Fixes a test in another patch.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 122182: [KUnitConversion] Currency: Give default values for various currencies

[Vishesh Handa]

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

Review request for KDE Frameworks.


Repository: kunitconversion


Description
---

The currency converter works by fetching a file from the network every
day and using those conversion values. If network is not available it
will try and use the previous version of that file. If no file 
then it will use the default values.

The default values was 1e+99 in all cases. This patch updates it to the
current values as reflected in the file.


Diffs
-

  src/currency.cpp 715233c 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Baloo Framework - License Exception

[Vishesh Handa]

Hello people

It seems that some people really do not want Baloo to become a framework as
long as it is effectively GPL. We might want to think about not restricting
ourselves in this way. If we didn't have plans to move away from Xapian it
would effectively become a core library that a lot of applications use that
has its own independent versioning and release schedule.

For now, Baloo will continue to be released part of Plasma. However, we
will be spoofing our version number to 5.6. So, everyone is will continue
using it as if it were a framework (the finder, library names, etc), but it
will not be released with frameworks.

-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Baloo Framework - License Exception

[Vishesh Handa]

On Mon, Jan 5, 2015 at 1:58 PM, Sebastian Kügler se...@kde.org wrote:

  Are we going to shoot ourselves in the foot (= making our lives more
  complicated, by having a dependency that works differently from the other
  dependencies and has to be released separately) just so that we can say
 all
  frameworks are LGPL ?

 I wouldn't call it shooting ourselves in the foot (since we know well about
 its consequences), but yes, I think it's worth it.


Great. We have somewhat of a consensus, and I would have to do a lot of
work to not make it a framework. I'll request the move and everything that
is required. I'll also add a disclaimer that Baloo is effectively GPL.

I also started hacking on the Xapian replacement, and I have a student
experimenting with Lucene, so Baloo will eventually become LGPL.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Baloo Framework - License Exception

[Vishesh Handa]

On Wed, Dec 17, 2014 at 2:38 PM, Milian Wolff m...@milianw.de wrote:

 May I ask the question why Baloo should become a framework in the first
 place?
 On Windows or Mac, people won't use it anyways, no? Or is it required there
 for e.g. searches in KMail?


* Windows and Mac people should be using the search indexes already present
there.
* Baloo is used as a library by - Plasma, KRunner, Dolphin and GwenView. It
would be good to give it API + ABI guarantees. While I agree it doesn't
need to be a framework for this, nothing really has to be a framework. We
all can just release software independently.
* I'm hoping that other desktop environments might want to think about use
it. We don't have many file searching solutions in the linux world. Again,
it doesn't need to be a framework to do this. However, releasing it as part
of Plasma does make it appear to be more tightly bound to Plasma.
* I like the 1 month cycles that Frameworks follows.
* It isn't required for KMail any more. It's purely for file searching +
indexing.

Anyway, I guess I could possibly just move Baloo to extragear and start
doing 1 month releases on my own. But it would be taxing.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Re: Baloo Framework - License Exception

[Vishesh Handa]

On Mon, Dec 15, 2014 at 9:27 AM, Martin Gräßlin mgraess...@kde.org wrote:

 I think Jonathan should respond to it. Your argumentation makes sense to
 me,
 but the question is whether Baloo is currently derived work of Xapian or
 not.
 If there is baloo internal an abstraction allowing to easily swap out
 Xapian
 by something different I would say it's not derived work. But if Xapian is
 deeply wired into Baloo I would say it's derived work.


It's not deeply wired but it is a very important component. We can easily
replace Xapian if we want to / find something nicer.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Re: Baloo Framework - License Exception

[Vishesh Handa]

On Mon, Dec 15, 2014 at 9:27 AM, Martin Gräßlin mgraess...@kde.org wrote:

 I think Jonathan should respond to it. Your argumentation makes sense to
 me,
 but the question is whether Baloo is currently derived work of Xapian or
 not.
 If there is baloo internal an abstraction allowing to easily swap out
 Xapian
 by something different I would say it's not derived work. But if Xapian is
 deeply wired into Baloo I would say it's derived work.


I've thought about this some more. Here is the current situation -

* We have the baloo_file executable which has a lot of Xapian code, which
is not trivial to replace.
* We have the Baloo library, which does the searching through Xapian. This
part is very very well abstracted out. In fact it started out allowing
other search providers.

However, Johnathon tells me that just the fact that the Baloo library is
linking against Xapian is enough to make it GPL.


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Baloo Framework - License Exception

[Vishesh Handa]

Hey guys

I would like to promote Baloo to be a framework for 5.6.

All of Baloo's code is LGPL, however, we internally use Xapian as a full
text index (GPL). This would make Baloo GPL. Could we have an exception for
now?

We have been looking into alternatives since we have a rather large share
of problems with Xapian [1]. But this will take time, I'm not too keen on
switching to something on a whim without proper testing. Plus, it would be
a substantially large change.

-- 
Vishesh Handa

[1] https://community.kde.org/Baloo/XapianProblems
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

[Vishesh Handa]

 On Oct. 13, 2014, 1:35 p.m., Marco Martin wrote:
  src/quickaddons/managedtexturenode.h, line 52
  https://git.reviewboard.kde.org/r/120550/diff/2/?file=318205#file318205line52
 
  even if will always remain just this member, just to me sure, it should 
  be in a dpointer
 
 Aleix Pol Gonzalez wrote:
 I don't think it's a good idea to add a d-pointer there. It's a class to 
 encapsulate the object, I don't see why we should store more things in there.
 
 If needs changed, then I'd suggest to create another class.
 
 Marco Martin wrote:
 if it's exported, i prefer a dpointer anyways
 
 Aleix Pol Gonzalez wrote:
 Can we please discuss this? I really don't think we want to be so cheap 
 when it comes to memory usage. This means that each node in the scene will 
 take a payload for no much reason.

It's not only about memory usage. The memory gets defragmented as well.

Also, maybe considering this class is so small, you may want to inline the 
function definitions.


- Vishesh


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


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120550: Use the same strategy as FrameSvgItem and IconItem in QIconItem

[Vishesh Handa]

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



src/quickaddons/imagetexturescache.h
https://git.reviewboard.kde.org/r/120550/#comment47668

Strange indentation.


- Vishesh Handa


On Oct. 13, 2014, 5:54 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120550/
 ---
 
 (Updated Oct. 13, 2014, 5:54 p.m.)
 
 
 Review request for KDE Frameworks, Plasma, David Edmundson, and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Moves the caching types used in Plasma Workspace into a QuickAddons submodule.
 
 Adopts them in QIconItem by moving it from a QPainterItem to a QQuickPainter 
 item. Uses the same strategies used in Plasma Framework for caching the 
 textures.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 2977812 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.h 839a33f 
   src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 0eb46b1 
   src/quickaddons/CMakeLists.txt PRE-CREATION 
   src/quickaddons/imagetexturescache.h PRE-CREATION 
   src/quickaddons/imagetexturescache.cpp PRE-CREATION 
   src/quickaddons/managedtexturenode.h PRE-CREATION 
   src/quickaddons/managedtexturenode.cpp PRE-CREATION 
   tests/qiconitem_test.qml PRE-CREATION 
   src/CMakeLists.txt eb0dfd3 
 
 Diff: https://git.reviewboard.kde.org/r/120550/diff/
 
 
 Testing
 ---
 
 I added some manual test (that was impossible to run before the patch). Also 
 tested it in KRunner and Muon Discover, which use the component. Everything 
 still works.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120049: KDirNotify: Use QUrl::toStringList

[Vishesh Handa]

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

(Updated Oct. 10, 2014, 10:32 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

Instead of a custom function. QUrl::toStringList reserves the size of the new 
list before hand.


Diffs
-

  src/core/kdirnotify.cpp 14f4c9c 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119796: Add a --run-env option

[Vishesh Handa]

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

(Updated Oct. 10, 2014, 11:03 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, Àlex Fiestas, Aleix Pol Gonzalez, and 
Michael Pyne.


Repository: kdesrc-build


Description
---

This adds a '--run-env' option which spawns a shell with the correct
environment so that the user can run the installed programs. This patch
does not work as I could not figure out how to get the kdedir and qtdir
configuration values from Module.pm. Could someone help?


Diffs
-

  modules/ksb/Application.pm 0324002 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Initial standalone Plasma::Package repository

[Vishesh Handa]

On Tue, Sep 16, 2014 at 2:19 PM, Marco Martin notm...@gmail.com wrote:

 right now all the classes are still under the Plasma namespace, and should
 probably be renamed and cmakes to be cleaned up (especially because it
 would
 be used by plasma too to the two identically named classes, new and
 deprecated
 would clash)


Just to confirm - With this, the package class in plasma-framework will now
be deprecated?




-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 120049: KDirNotify: Use QUrl::toStringList

[Vishesh Handa]

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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

Instead of a custom function. QUrl::toStringList reserves the size of the new 
list before hand.


Diffs
-

  src/core/kdirnotify.cpp 14f4c9c 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: split kdepimlibs

[Vishesh Handa]

On Tuesday, August 26, 2014 11:50:50 AM Kevin Ottens wrote:
  gpgme++
  kabc
  kalarmcal
  kblog
  kcalcore
  kcalutils
 
 This one looks like a dumping ground of random things. Maybe some of it
 should  move in other frameworks?

KAbc definitely doesn't seem like a dumping ground. It's a valuable library 
that is also used by kpeople. It seems like a good candidate for an individual 
framework.

-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 119796: Add a --run-env option

[Vishesh Handa]

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

Review request for KDE Frameworks, Àlex Fiestas, Aleix Pol Gonzalez, and 
Michael Pyne.


Repository: kdesrc-build


Description
---

This adds a '--run-env' option which spawns a shell with the correct
environment so that the user can run the installed programs. This patch
does not work as I could not figure out how to get the kdedir and qtdir
configuration values from Module.pm. Could someone help?


Diffs
-

  modules/ksb/Application.pm 0324002 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Making KFileMetaData a framework

[Vishesh Handa]

On Tuesday, August 05, 2014 07:58:03 PM David Edmundson wrote:
 In general that's some of the tidied code I've seen in a long time.

Thanks!

 
 Comments below. One major, most not.
 
 
 TypeInfo/PropertyInfo/SimpleExtractionResult need a working operator=
 otherwise we shallow copy d.
 
 I can cause a crash in dump.cpp

Fixed

 
 
 Extracting info from a file can take a long time, right now you spawn
 a separate execuatble for Baloo; but for other uses (i.e dolphin
 getting info on a non-indexed file) you might want to add a helper
 API; either a new thread or a service like baloo-file-extractor and a
 wrapper round calling it.
 
 
 
 ExtractorPluginManager::Private::allExtractors
 what's going on with the variable plugins? You're always checking if
 an empty list contains things. Looks leftover from something.
 

Fixed

 
 
 In properties.h I would space out the enum so that in the future when
 you insert extra ones you can put it alongside the relevant category
 without breaking API
 
 i.e
 //Audio
 BitRate = 100,
 Channels,
 Duration,
 
 // Documents
 Author =200,
 Title,
 ...
 
 Otherwise it'll be an ungrouped mess in future releases if you ever
 have to add another audio property.
 

Might make sense. Currently adding gaps in the properties would break my 
tests, but it might be a good idea. I'll look more into it.

 -
 ExtractionPluginManager possibly shouldn't be a QObject?

Fixed

 
 It /might/ be a good idea to make ExtractionPluginManager static so
 you don't load the plugins every time (which can be a bit expensive)
 

Hmm, Possibly.

 
 
 
 dump.cpp should check there's at least one arg.
 (I know it's a test, but you explicitly check for =2 but not 1)
 

Fixed 

Thanks for reviewing the code.

-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Making KFileMetaData a framework

[Vishesh Handa]

On Tuesday, August 05, 2014 11:19:16 PM Kevin Ottens wrote:
 Hello,
 
 On Tuesday 05 August 2014 18:36:24 Vishesh Handa wrote:
  I would appreciate it if everyone could review the code once before it
  gets
  into frameworks.
 
 metainfo.yaml should have release: false until it's part of a KF release.
 Also it should be integration and not functional (relies on plugins).

Fixed

 
 The test folder should be named tests (see directory structure policy).


Fixed
 
 Public headers should use  style include. Also it seems you're not
 following the k convention but the namespace convention for your classes,
 then the includes in public headers should be namespaced as well (e.g.
 kfilemetadata/foo.h).
 
 I'm not sure why ExtractorPluginManager is exported. A function in a
 namespace would be enough, there's no point in instantiating a manager by
 hand from the client code perspective, at best looks like leaking an
 implementation detail.

We could just expose a function which loads all the Extractors, but then the 
client side would need to iterate over all of them and figure out which ones 
match the mimetype they are targeting. I was hoping to avoid that.

Currently, ExtractorPluginManager just provides a fetchExtractors(mimetype) 
function. This cannot just be made a function as one doesn't want to load all 
the plugins each time. Hmm, or maybe we just save all the plugins in a static 
variable.

 
 Similarly the ExtractorPlugin naming is odd in the public API as it states
 it is necessarily a plugin (implementation detail). I'd rename it
 ExtractorInterface, I'd drop the suffix altogether or I'd keep
 ExtractorPlugin for plugin implementors while they'd be wrapped in
 Extractor instances on the client code side (I think that's actually my
 favorite solution).

Just to clarify -
* ExtractorPlugin - All plugins would derive from this. We could also call it 
ExtractorInterface (I do like this name better).
* We have an Extractor class which is just a wrapper on top of the 
ExtractorInterface which does ... ?

 
 AFAICT there's no reason for ExtractorPlugin to inherit from QObject at that
 point. Same thing for ExtractorPluginManager.

Fixed ExtractorPluginManager

ExtractorPlugin - This now requires every plugin to derive from both 
ExtractorPlugin and QObject. This might be less friendly.

 
 Creating by hand the ExtractionResult, then passing it by pointer to the
 extract method looks odd. I'd expect calling extract() with a bunch of
 parameters and getting a result back (another reason for wrapping plugins on
 the client side).

The idea was that every client should implement their own ExtractionResult. By 
default it is a pure virtual class. Cause you really don't want to be storing 
all the text in memory.

 
 The whole API is synchronous which we probably don't want. It'd be better to
 have an async API (much better to build up sync on top of async than the
 contrary).

Hmm, how would we do async? I thought people could just run it in another 
thread / process if they want. The only thing I can think of changing is that 
the plugins return the result later via a signal instead of doing all the work 
in one function.

 
 What about the thread safety? At a glance I would say ExtractorPluginManager
 is not
 
 That's about it after a quick glance while tired. Keep us posted for a
 second round.

Thanks for the review!

 
 Regards.

-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Making KFileMetaData a framework

[Vishesh Handa]

Hello people

I would like to make KFileMetaData a framework. It's a simple library for 
extracting file metadata and text. It depends on the i18n and KArchive (can be 
made optional), along with a number of external dependencies which are 
optional.

The code is of decent quality, and we have a certain number of unit tests. 
It's currently only used by Baloo.

I would appreciate it if everyone could review the code once before it gets 
into frameworks.
 
-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119155: Make the desktop containment respect minimum sizes

[Vishesh Handa]

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



containments/desktop/package/contents/ui/AppletAppearance.qml
https://git.reviewboard.kde.org/r/119155/#comment43027

The curly brackets aren't really required. Are we supposed to always have 
them even in QML code?

I know it's a requirement in C++ code.



containments/desktop/package/contents/ui/AppletAppearance.qml
https://git.reviewboard.kde.org/r/119155/#comment43024

This function would be called twice.

Maybe -

property Object minimumSize = computeMinimumSize();
property int minimumWidth = minimumSize.width;
property int minimumHeight = minimumSize.height;



containments/desktop/package/contents/ui/AppletAppearance.qml
https://git.reviewboard.kde.org/r/119155/#comment43023





containments/desktop/package/contents/ui/AppletAppearance.qml
https://git.reviewboard.kde.org/r/119155/#comment43025

You're returning 0 over here. This will result in a warning in line 264 and 
265.



containments/desktop/package/contents/ui/main.qml
https://git.reviewboard.kde.org/r/119155/#comment43026

?


- Vishesh Handa


On July 7, 2014, 12:51 p.m., Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119155/
 ---
 
 (Updated July 7, 2014, 12:51 p.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: plasma-desktop
 
 
 Description
 ---
 
 make the applets respect the minimum size, specified by applets, with  the 
 following logic:
 if there is a preferred representation, enforce its minimum size,
 if there is a compact representation, enforce its minimum size.
 
 note: the minimum size of the full representation is not enforced if it's not 
 the preferred one, because it must still be possible to switch to the compact 
 one.
 
 this would fix bugs such as
 https://bugs.kde.org/show_bug.cgi?id=337069
 
 
 Diffs
 -
 
   containments/desktop/package/contents/ui/AppletAppearance.qml 9149f48 
   containments/desktop/package/contents/ui/main.qml a9cf53e 
 
 Diff: https://git.reviewboard.kde.org/r/119155/diff/
 
 
 Testing
 ---
 
 * even if the property reading is buried down in a function, property binding 
 still works correctly.
 
 * if the minimum size grows, it may happen the case of overlapping applets, 
 thing that was avoideed with care by the new layout engine. This may be a 
 blocker in shipping it for 5.0
 
 
 Thanks,
 
 Marco Martin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 119110: Release Blocker - KProtocolManager: Fix double mutex locking on a non recursive mutex

[Vishesh Handa]

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

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

KProtocolManager: Fix double mutex locking on a non recursive mutex

Currently called reparseConfiguration results in an infinite block
because the code tries to lock the mutex twice even though it is not a
recursive mutex.

Stack trace -
 KProtocolManager::entryMap - Tries to lock it
 KIO::SlaveConfigPrivate::readGlobalConfig
 KIO::SlaveConfig::reset
 KProtocolManager::reparseConfiguration - Holds the lock

This can easily be solved by making the mutex recursive, but that breaks
the asserts as they are using QMutex::tryLock to check if the mutex is
locked. With the mutex being recursive tryLock just results in them
getting locked again.

I'm not sure if this is the correct way of fixing it

Here is the full backtrace if anyone is interested -

#0  0x7328a359 in syscall () from /usr/lib/libc.so.6
#1  0x73e21830 in _q_futex (addr=0x731a4b00 (anonymous 
namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder, op=0, 
val=3, timeout=0x0)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:154
#2  0x73e21a04 in lockInternal_helperfalse (d_ptr=..., timeout=-1, 
elapsedTimer=0x0) at 
/home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:195
#3  0x73e21891 in QBasicMutex::lockInternal (this=0x731a4b00 
(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:211
#4  0x73e2164c in QMutex::lock (this=0x731a4b00 (anonymous 
namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex.cpp:225
#5  0x72eed989 in QMutexLocker::QMutexLocker (this=0x7fffc6f0, 
m=0x731a4b00 (anonymous 
namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
at /opt/qt5/include/QtCore/qmutex.h:136
#6  0x72ee7f27 in KProtocolManager::entryMap (group=...) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:294
#7  0x72ee5748 in KIO::SlaveConfigPrivate::readGlobalConfig 
(this=0x7b5d10) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:73
#8  0x72ee5fa6 in KIO::SlaveConfig::reset (this=0x7b6b20) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:227
#9  0x72ee7dfc in KProtocolManager::reparseConfiguration () at 
/home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:278
#10 0x72ea37ee in KIO::SessionData::reset (this=0x7b52f0) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:136
#11 0x72ea3289 in KIO::SessionData::configDataFor (this=0x7b52f0, 
configData=..., proto=...) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:102
#12 0x72edb7f9 in KIO::SchedulerPrivate::metaDataFor (this=0x7b52d0, 
protocol=..., proxyList=..., url=...)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1049
#13 0x72edc18b in KIO::SchedulerPrivate::setupSlave (this=0x7b52d0, 
slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, config=0x0)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1094
#14 0x72edb737 in setupSlave (slave=0x82d3d0, url=..., protocol=..., 
proxyList=..., newSlave=true, config=0x0)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1042
#15 0x72eda759 in KIO::ProtoQueue::startAJob (this=0x7c11b0) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:621
#16 0x72edd795 in KIO::ProtoQueue::qt_static_metacall (_o=0x7c11b0, 
_c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffcfa0)
at /home/vishesh/kde5/build/frameworks/kio/src/core/moc_scheduler_p.cpp:244
#17 0x740879bf in QMetaObject::activate (sender=0x7c1208, 
signalOffset=3, local_signal_index=0, argv=0x0)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/kernel/qobject.cpp:3680


Diffs
-

  src/core/kprotocolmanager.cpp 63baa6d 

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


Testing
---

This can be tested by running `khotnewstuff plasmoids.knsrc` in 
knewstuff/tests/. Without this patch it just blocks.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119110: Release Blocker - KProtocolManager: Fix double mutex locking on a non recursive mutex

[Vishesh Handa]

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

(Updated July 4, 2014, 9:58 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

KProtocolManager: Fix double mutex locking on a non recursive mutex

Currently called reparseConfiguration results in an infinite block
because the code tries to lock the mutex twice even though it is not a
recursive mutex.

Stack trace -
 KProtocolManager::entryMap - Tries to lock it
 KIO::SlaveConfigPrivate::readGlobalConfig
 KIO::SlaveConfig::reset
 KProtocolManager::reparseConfiguration - Holds the lock

This can easily be solved by making the mutex recursive, but that breaks
the asserts as they are using QMutex::tryLock to check if the mutex is
locked. With the mutex being recursive tryLock just results in them
getting locked again.

I'm not sure if this is the correct way of fixing it

Here is the full backtrace if anyone is interested -

#0  0x7328a359 in syscall () from /usr/lib/libc.so.6
#1  0x73e21830 in _q_futex (addr=0x731a4b00 (anonymous 
namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder, op=0, 
val=3, timeout=0x0)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:154
#2  0x73e21a04 in lockInternal_helperfalse (d_ptr=..., timeout=-1, 
elapsedTimer=0x0) at 
/home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:195
#3  0x73e21891 in QBasicMutex::lockInternal (this=0x731a4b00 
(anonymous namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:211
#4  0x73e2164c in QMutex::lock (this=0x731a4b00 (anonymous 
namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex.cpp:225
#5  0x72eed989 in QMutexLocker::QMutexLocker (this=0x7fffc6f0, 
m=0x731a4b00 (anonymous 
namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
at /opt/qt5/include/QtCore/qmutex.h:136
#6  0x72ee7f27 in KProtocolManager::entryMap (group=...) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:294
#7  0x72ee5748 in KIO::SlaveConfigPrivate::readGlobalConfig 
(this=0x7b5d10) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:73
#8  0x72ee5fa6 in KIO::SlaveConfig::reset (this=0x7b6b20) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:227
#9  0x72ee7dfc in KProtocolManager::reparseConfiguration () at 
/home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:278
#10 0x72ea37ee in KIO::SessionData::reset (this=0x7b52f0) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:136
#11 0x72ea3289 in KIO::SessionData::configDataFor (this=0x7b52f0, 
configData=..., proto=...) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:102
#12 0x72edb7f9 in KIO::SchedulerPrivate::metaDataFor (this=0x7b52d0, 
protocol=..., proxyList=..., url=...)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1049
#13 0x72edc18b in KIO::SchedulerPrivate::setupSlave (this=0x7b52d0, 
slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, config=0x0)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1094
#14 0x72edb737 in setupSlave (slave=0x82d3d0, url=..., protocol=..., 
proxyList=..., newSlave=true, config=0x0)
at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1042
#15 0x72eda759 in KIO::ProtoQueue::startAJob (this=0x7c11b0) at 
/home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:621
#16 0x72edd795 in KIO::ProtoQueue::qt_static_metacall (_o=0x7c11b0, 
_c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffcfa0)
at /home/vishesh/kde5/build/frameworks/kio/src/core/moc_scheduler_p.cpp:244
#17 0x740879bf in QMetaObject::activate (sender=0x7c1208, 
signalOffset=3, local_signal_index=0, argv=0x0)
at /home/vishesh/kde5/qt5/qtbase/src/corelib/kernel/qobject.cpp:3680


Diffs
-

  src/core/kprotocolmanager.cpp 63baa6d 

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


Testing
---

This can be tested by running `khotnewstuff plasmoids.knsrc` in 
knewstuff/tests/. Without this patch it just blocks.


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119110: Release Blocker - KProtocolManager: Fix double mutex locking on a non recursive mutex

[Vishesh Handa]

 On July 4, 2014, 7:22 p.m., David Faure wrote:
  recursive mutexes are more expensive. Can you try this patch instead?
  http://www.davidfaure.fr/2014/kprotocolmanager.cpp.diff
 
 David Faure wrote:
 Actually, nothing beats a unittest :) Added, fix pushed.
 
 http://commits.kde.org/kio/842bc8008f5eed03698e8ec67351e791b65ad2b1
 
 I'll repack kio after dinner.
 
 David Faure wrote:
 Done (v4.100.0-rc2). You can discard this RR.

Just out of curiosity, what's so expensive about Recursive mutexes?


- Vishesh


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


On July 4, 2014, 9:58 p.m., Vishesh Handa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119110/
 ---
 
 (Updated July 4, 2014, 9:58 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Repository: kio
 
 
 Description
 ---
 
 KProtocolManager: Fix double mutex locking on a non recursive mutex
 
 Currently called reparseConfiguration results in an infinite block
 because the code tries to lock the mutex twice even though it is not a
 recursive mutex.
 
 Stack trace -
  KProtocolManager::entryMap - Tries to lock it
  KIO::SlaveConfigPrivate::readGlobalConfig
  KIO::SlaveConfig::reset
  KProtocolManager::reparseConfiguration - Holds the lock
 
 This can easily be solved by making the mutex recursive, but that breaks
 the asserts as they are using QMutex::tryLock to check if the mutex is
 locked. With the mutex being recursive tryLock just results in them
 getting locked again.
 
 I'm not sure if this is the correct way of fixing it
 
 Here is the full backtrace if anyone is interested -
 
 #0  0x7328a359 in syscall () from /usr/lib/libc.so.6
 #1  0x73e21830 in _q_futex (addr=0x731a4b00 (anonymous 
 namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder, op=0, 
 val=3, timeout=0x0)
 at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:154
 #2  0x73e21a04 in lockInternal_helperfalse (d_ptr=..., timeout=-1, 
 elapsedTimer=0x0) at 
 /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:195
 #3  0x73e21891 in QBasicMutex::lockInternal (this=0x731a4b00 
 (anonymous 
 namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
 at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp:211
 #4  0x73e2164c in QMutex::lock (this=0x731a4b00 (anonymous 
 namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
 at /home/vishesh/kde5/qt5/qtbase/src/corelib/thread/qmutex.cpp:225
 #5  0x72eed989 in QMutexLocker::QMutexLocker (this=0x7fffc6f0, 
 m=0x731a4b00 (anonymous 
 namespace)::Q_QGS_kProtocolManagerPrivate::innerFunction()::holder)
 at /opt/qt5/include/QtCore/qmutex.h:136
 #6  0x72ee7f27 in KProtocolManager::entryMap (group=...) at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:294
 #7  0x72ee5748 in KIO::SlaveConfigPrivate::readGlobalConfig 
 (this=0x7b5d10) at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:73
 #8  0x72ee5fa6 in KIO::SlaveConfig::reset (this=0x7b6b20) at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/slaveconfig.cpp:227
 #9  0x72ee7dfc in KProtocolManager::reparseConfiguration () at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/kprotocolmanager.cpp:278
 #10 0x72ea37ee in KIO::SessionData::reset (this=0x7b52f0) at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:136
 #11 0x72ea3289 in KIO::SessionData::configDataFor (this=0x7b52f0, 
 configData=..., proto=...) at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/sessiondata.cpp:102
 #12 0x72edb7f9 in KIO::SchedulerPrivate::metaDataFor (this=0x7b52d0, 
 protocol=..., proxyList=..., url=...)
 at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1049
 #13 0x72edc18b in KIO::SchedulerPrivate::setupSlave (this=0x7b52d0, 
 slave=0x82d3d0, url=..., protocol=..., proxyList=..., newSlave=true, 
 config=0x0)
 at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1094
 #14 0x72edb737 in setupSlave (slave=0x82d3d0, url=..., protocol=..., 
 proxyList=..., newSlave=true, config=0x0)
 at /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:1042
 #15 0x72eda759 in KIO::ProtoQueue::startAJob (this=0x7c11b0) at 
 /home/vishesh/kde5/src/frameworks/kio/src/core/scheduler.cpp:621
 #16 0x72edd795 in KIO::ProtoQueue::qt_static_metacall (_o=0x7c11b0, 
 _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffcfa0

Re: Review Request 118960: Add a test to print out KLauncher's autostart files

[Vishesh Handa]

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


ping?

It's just a test.

- Vishesh Handa


On June 26, 2014, 2:48 p.m., Vishesh Handa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118960/
 ---
 
 (Updated June 26, 2014, 2:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kinit
 
 
 Description
 ---
 
 Added it in order to debug something. Seemed like a useful things have.
 
 
 Diffs
 -
 
   CMakeLists.txt 631b9d0 
   tests/CMakeLists.txt PRE-CREATION 
   tests/autostarttest.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/118960/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Vishesh Handa
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: qt5 polkit-qt-1 and kdesrc-build

[Vishesh Handa]

On Sat, Jun 28, 2014 at 5:09 PM, Sune Vuorela nos...@vuorela.dk wrote:

 On 2014-06-27, David Faure fa...@kde.org wrote:
 
  This question is still open. We're releasing kauth as part of KF5 but
  polkit-qt-1 isn't getting released.
 
  Is there any maintainer for polkit-qt-1?
 
  For that matter, who maintains KAuth, which optionally depends on
 polkit-qt-1?

 Isn't KAuth fully useless on linux without polkit-qt ?


Yes.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118960: Add a test to print out KLauncher's autostart files

[Vishesh Handa]

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

(Updated June 30, 2014, 12:45 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kinit


Description
---

Added it in order to debug something. Seemed like a useful things have.


Diffs
-

  CMakeLists.txt 631b9d0 
  tests/CMakeLists.txt PRE-CREATION 
  tests/autostarttest.cpp PRE-CREATION 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118970: libkeduvocdocument: Remove KDELibs4Support dependency.

[Vishesh Handa]

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


The code formatting is quite off. You probably want to run everything under 
astyle at some point.


keduvocdocument/keduvoccsvreader.cpp
https://git.reviewboard.kde.org/r/118970/#comment42529

No fancy headers? :(



keduvocdocument/keduvocdocument.cpp
https://git.reviewboard.kde.org/r/118970/#comment42530

I cannot say about KFilterDev, but a TemporaryFile doesn't need to be 
explicitly closed before destruction.



keduvocdocument/sharedkvtmlfiles.cpp
https://git.reviewboard.kde.org/r/118970/#comment42531

Maybe you QDebug instead of QtDebug? The are  internally the same thing, 
but the former looks way more consistent.



keduvocdocument/tests/converter.cpp
https://git.reviewboard.kde.org/r/118970/#comment42532

I'm slightly confused. You're using i18n everywhere else, but here you use 
translate?


- Vishesh Handa


On June 27, 2014, 4:55 a.m., Jeremy Whiting wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118970/
 ---
 
 (Updated June 27, 2014, 4:55 a.m.)
 
 
 Review request for KDE Edu, KDE Frameworks, Aleix Pol Gonzalez, and Inge 
 Wallin.
 
 
 Repository: libkdeedu
 
 
 Description
 ---
 
 Port tests away from KCmdLineArgs to QCommandLineParser.
 Port from KDE_DEPRECATED to KEDUVOCDOCUMENT_DEPRECATED (Maybe we should 
 remove these?)
 Port from KLocale to klocalizedstring.h
 
 
 Diffs
 -
 
   CMakeLists.txt 4feb10b67f78530170aaae216b9347cf94c0e51a 
   keduvocdocument/CMakeLists.txt c242c46fa79f128d63ccdeb5c4f611ad7294c874 
   keduvocdocument/keduvocarticle.h b0b69c06d3d91ecc1680ecb18c9dd074beed1cb1 
   keduvocdocument/keduvoccsvreader.cpp 
 e3d471c4ecbb1236bae73714f27e357a1681ec70 
   keduvocdocument/keduvoccsvwriter.cpp 
 32172e47d3617c082378969735dbd23d3178f06e 
   keduvocdocument/keduvocdocument.h 9da14c62c84a07ffc53ef46f4c48e35824210d2c 
   keduvocdocument/keduvocdocument.cpp 
 7026ccb9046968821b11ce6e07276d81e58e41c6 
   keduvocdocument/keduvockvtml2reader.cpp 
 3884724422c318e34a5b8efc5e05481e27f7c6bc 
   keduvocdocument/keduvockvtmlcompability.cpp 
 e486729054a9e69fd52c4e4f1031a0bdbb3433cf 
   keduvocdocument/keduvocpaukerreader.cpp 
 4ded3e480222ea9401b8cbfddad2b99ba82036fb 
   keduvocdocument/keduvoctranslation.h 
 c41d293755f0df7f87a41300569b093d020beb06 
   keduvocdocument/keduvocvokabelnreader.cpp 
 ffbd64d3cd50e519af319d6d52a1491a7ed1588a 
   keduvocdocument/keduvocwqlreader.cpp 
 0841b1054ec41b93f4fffb1165c2c0d51806aeae 
   keduvocdocument/keduvocxdxfreader.cpp 
 4317f6ff1fbc8a4a19c2bbcc72af2ace295743b6 
   keduvocdocument/sharedkvtmlfiles.cpp 
 9f74aa2af2f69f25fca8efa1585b8c8ea1127cd9 
   keduvocdocument/tests/CMakeLists.txt 
 ac01a2b4a3e3b925517b2f004feb93bf365344a7 
   keduvocdocument/tests/converter.cpp 
 1916b2269a88e932beb46f165e6cebebcefecb3c 
   keduvocdocument/tests/sharedkvtmlfilestest.cpp 
 489dad4fcafa0a97fe4e93905206fb0fc53e0cf5 
 
 Diff: https://git.reviewboard.kde.org/r/118970/diff/
 
 
 Testing
 ---
 
 It still builds and the tests run.
 
 Unrelated the sharedkvtmlfilestest exposes a bug in KEduVocDocument::open 
 where the KFilterDev wont open, (bug from previous commit) not sure what to 
 do here to fix it though.
 
 
 Thanks,
 
 Jeremy Whiting
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118960: Add a test to print out KLauncher's autostart files

[Vishesh Handa]

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

Review request for KDE Frameworks.


Repository: kinit


Description
---

Added it in order to debug something. Seemed like a useful things have.


Diffs
-

  CMakeLists.txt 631b9d0 
  tests/CMakeLists.txt PRE-CREATION 
  tests/autostarttest.cpp PRE-CREATION 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118783: Set the normal shortcut when setting the default shortcut as well

[Vishesh Handa]

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

(Updated June 20, 2014, 11:18 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and Martin Gräßlin.


Repository: kglobalaccel


Description
---

When a client calls setDefaultShortcut, only the default shortcut is
set. This makes sense, however kglobalaccel doesn't actually do anything
with the global shortcut, it just writes it to the configuration and
reads it back.

All the actual logic is implemented behind active or normal shortcuts.
In kdelibs4, most applications would call KAction::setGlobalShorcut which
had a default of setting BOTH the active and the default shortcut. Again,
I'm not sure what they point of this was if the default shortcut does not
actually do anything.

This fixes bugs such as the brightness key not working because
Powerdevil only sets the default shortcut.


Diffs
-

  src/kglobalaccel.cpp 54d18ec 

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


Testing
---


Thanks,

Vishesh Handa

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118844: Introduce convenient methods to set active and default shortcut

[Vishesh Handa]

 On June 20, 2014, 11:17 a.m., Vishesh Handa wrote:
  I approve.
  
  The only problem I have is with the documentation, but we can improve that 
  in another patch.
 
 Martin Gräßlin wrote:
 well let's improve directly. What do you think is missing with these new 
 methods or is that more a general comment?

Stuff that should be mentioned - 

* What a default shortcut is - It is simply something written in a 
configuration file. This is something the user can later use to reset the 
shortcut. It cannot be overwritten by a user. Also, maybe how you can change 
the default shortcut from an application point of view. Maybe even a big 
disclaimer that this will NOT actually set the shortcut.

* What an active shortcut is - It is the shortcut currently in use. This can be 
changed by the user.

I'm not sure if this should be mentioned in this method's documentation or 
somewhere else.
  


- Vishesh


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


On June 20, 2014, 10:01 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118844/
 ---
 
 (Updated June 20, 2014, 10:01 a.m.)
 
 
 Review request for KDE Frameworks and Vishesh Handa.
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 Introduce convenient methods to set active and default shortcut
 
 Simplifies the setting of shorctus when default and active should be
 the same or if only one shortcut is needed.
 
 
 Diffs
 -
 
   src/kglobalaccel.h d11881c89e889a77f47c1ba5db42db6ebef665b1 
   src/kglobalaccel.cpp 54d18ecf918d4d0890c0a37aec10270119edd103 
 
 Diff: https://git.reviewboard.kde.org/r/118844/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

KGlobalAccel Problems

[Vishesh Handa]

Hey guys

I've been looking into kglobalaccel in order to fix some problems that I
was having. I seems to have opened a can of worms.

Background Information: KGlobalAccel has 2 kinds of shortcuts - Active
Shortcuts and Default Shortcuts. Active Shortcuts are the ones currently in
use, whereas the default ones are just the ones which an application
recommends that should be the default. Setting this default shortcut just
results in it being written to a configuration file.

In the KDE4 days you would set a global shortcut via
KShortcut::setGlobalShortcut(KShortcut, flags). The flags over here were by
(Default | Active), so most of the applications would just set it as both
the default and the active one. Everything was nice and simple.

Now, stuff that has changed -
1. The API has an explicit setDefaultShortcut and setShortcut command. This
is bad because setting the deafult shortcut does not set the active one and
vice versa. This means applications need to call both. No application
currently does this.

2. We allow multiple global shortcuts per application - This is awesome,
cause it is super useful, but it also results in -
* The KCM only showing one shortcut. And when you change that shortcut,
then all the others are cleared.
*  Having different lists of default and normal shortcuts results in the
KCM flaking out and not showing the active shortcut properly. I've been
trying to diagnose this, but haven't been successful. I'll try again
tomorrow.

How do we go about fixing this? Options -

a.) Set the active shortcut whenever you set the default shortcut. This is
a good compromise, but it also means that any successive call to
setShortcut(..) will not do anything cause the shortcut has already been
set.

Certain application such as KRunner which set one default shortcut, and
multiple active shortcuts would break, and need to be fixed. (Which is
fine, it's just what happens).

We should probably also add some documentation regarding this.

b.) Let stuff remain the way it is and add extra documentation. This would
mean we would need to update every app using KGlobalAccel and make them set
both the default and the active shortcut.

c.) Introduce the concept of flags in setShortcut where the default is
active + default. This would make it similar to how it was in kdelibs4. The
whole active + default thing is still confusing though and we would need
better documentation.

---

Fixing (2) - I like having multiple default shortcuts available, so I
suppose we need to update the KCM? This means fixing up KShortcutEditor
from kxmlgui. The code there is a little ambiguous, so I'm not looking
forward to that. Also, not sure how it would look visually.

Other minor Issues -

KXmlGui - KShortcutEditor seems to have some fixmes for KDE5. Do we want to
do fix those? Perhaps someone should go through all the frameworks and make
these changes or remove the comments?


-- 
Vishesh Handa
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel