Re: Alternative to QDateTime::isDateOnly ?
On Wed, Apr 29, 2015, at 12:33 AM, Aleix Pol wrote:
> On Tue, Apr 28, 2015 at 10:11 PM, Christian Mollekopf
> wrote:
> >
> > I may be a bit extreme that way, but QDateTime::isValid() would be a
> > blocker
> > for the isDateOnly() functionality IMO.
> >
> >> I would most certainly not go into template stuff (i.e. 3, 4 and 5) 2
> >> looks ok but if we get to add the API in Qt, we'll get to port things
> >> much more easily.
> >
> > I agree that the Qt solution would be the easiest, but why would you not
> > use the template solutions?
> > They actually seem to be the cleanest to me.
>
> - value
> Using templates when you know the 2 types it will have is not better
> than just making both methods.
>
It avoids encoding the type into the name, but yes, it's essentially the
same.
> - variant/QPair<>
> It's hard to read the code afterwards.
>
I don't find a "variant" hard to read.
> What about having a new class (In KCoreAddons? KCalCore?) to replace
> KDateTime in PIM?
>
> Something like:
> class Occasion
> {
> QDateTime dateTime() const;
> QDate date() const;
> bool isAllDay() const;
> }
Definitely a good suggestion in line with John's Duration.
What I like about the "discriminated union" [0] approach is that it
solves pretty much the same problem,
using a more generic solution.
The only good argument I'd see for an Occasion class would be if it
offered additional helpers that are useful
to both date-only and date-time occasions. I.e. a date() accessor, that
returns the date of either date-time or date.
Sooo I'm still not entirely sure, but something similar like
boost::variant (a "discriminated union"), doesn't seem like the worst
idea, but something like Occasion is not that far off either. I'll sleep
over it again ;-)
Cheers,
Christian
[0] http://www.drdobbs.com/cpp/discriminated-unions-i/184403821?pgno=2
Re: Alternative to QDateTime::isDateOnly ?
On Tue, Apr 28, 2015, at 08:47 PM, John Layt wrote: > On 27 April 2015 at 21:17, Christian Mollekopf > wrote: > Hey John, > > 1. add isDateOnly functionality to QDateTime > ... > > Opinions following: > > 1. I'm not sure whether it semantically makes sense to have a QDateTime > > without a time. > > Adding it to QDateTime was not an option Thiago was happy with so it > didn't make it, It is something only used inside PIM so there's no > wide use-case for it. I do think I could add it fairly easily as a new > internal attribute flag, but it could complicate the code a lot and > I'm also not sure it's possible to do with a backwards compatible > behaviour either. > Ok. > Using a QDate in the api is probably not an option for PIM though as > it doesn't have a QTimeZone attached which you will certainly always > need (and yes, that is a reason for doing it in QDateTime). > We don't need a timezone for date-only (AFAIR), but we do need date-time sometimes. > One option is the invalid QTime that Aleix mentions. I did have that > in the back of my mind while re-writing QDateTime internals and so > whatever QDate, Qtime and QTimeZone you set should persist in spite of > the QDateTIme overall being invalid. However it's not really a > solution as how would you know when it was really invalid or when it > was Date Only? > That's seems like too much of a hack for me. > Personally, I suspect relying on the QDateTime to tell you it is Date > Only is perhaps the wrong approach? Perhaps it's actually an attribute > of the Event that it is Date Only and not an attribute of the > QDateTime? Rather than checking the QDateTime you've received from a > call to start() to see if it is Date Only, you should be checking if > the Event itself is Date Only? Your setter could have an enum for > choosing, or separate setters for QDate and QDateTime, and then have a > getter for QDateTIme and another for the enum. While this may seem > like a little more work for PIM, it's not used in many places so I > think this may be a better option, and easier to achieve too in the > required timeframe as it's all in your own control. > Not a bad idea at all... The end/due date must follow the start date by definition after all. On the other hand, the problem of the variable return type is not solved by that, overloads and separate getters always seemed like a workaround to me, so it seems like Qt would benefit from something like boost::variant (aka. a discriminated union) > > It would make sense to have a PointInTime with higher or > > lower accuracy though. > > I had pondered for a while having that, but with QDateTime internals > now entirely held as milliseconds relative to the Unix epoch and > translated into the QDate and QTime relative to the QTimeZone on the > fly, that's effectively what QDateTime has become. Now, a QDuration > class, or duration mode for QDateTime, could be useful though... > True, a separate Duration class that can either point to a day (date-only), or a second (date-time) could be a solution as well. Thanks for your ideas, Christian
Re: Alternative to QDateTime::isDateOnly ?
Hey Aleix, > > What about considering the port to be like: > QDateTime().time().isNull()? > > Even QDateTime::isValid documentation mentions that the date and the > time need to be valid, therefore the time can be invalid. > > With that assumption, I'd say we could even implement > QDateTime::isDateOnly() or similar. > I appreciate the pragmatism of that approach, but I just consider an interface that returns an invalid QDateTime fundamentally broken (tm). I mean, that would be like the first thing I'd check in a unittest, and the behaviour would IMO be completely unexpected. I may be a bit extreme that way, but QDateTime::isValid() would be a blocker for the isDateOnly() functionality IMO. > I would most certainly not go into template stuff (i.e. 3, 4 and 5) 2 > looks ok but if we get to add the API in Qt, we'll get to port things > much more easily. I agree that the Qt solution would be the easiest, but why would you not use the template solutions? They actually seem to be the cleanest to me. Thanks for your input! Christian
Alternative to QDateTime::isDateOnly ?
Hey, KDateTime used to have a date-only functionality, that QDateTime is lacking. The problem with that is that we need to find a new solution for interfaces that allow to set/get either QDate or QDateTime. One such interface is KCalCore::Event::start(). For the sake of discussion getters are more interesting, because a simple overload is not possible. I see the following possible solutions: 1. add isDateOnly functionality to QDateTime 2. Overloads with different names: QDate startDate(), QDateTime startDateTime() 3. Overloads using templates: T start() 4. QVariant that can contain either QDateTime or QDate: QVariant start() 5. boost::variant that can contain either QDateTime or QDate: boost::variant start() Given that this should be a fairly common porting problem (at least in the PIM realm), it would be nice to have a standard solution for it. Opinions following: 1. I'm not sure whether it semantically makes sense to have a QDateTime without a time. It would make sense to have a PointInTime with higher or lower accuracy though. 2. Not a solution, but a workaround. 3. Better than 2., but not by much. 4. Would be pretty good IMO, but unfortunately leads to an unexpressive interface (because QVariant can't be parametrized with valid values). 5. boost::variant solves the problem of 4., and is header-only, but I'm not sure to what extent boost is accepted in interfaces? I think the variant solutions are actually not that bad, semantically, but QVariant seems a bit useless for a case like this. Any ideas or opinions? Cheers, Christian
Re: Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals
> On Jan. 26, 2015, 9:41 a.m., Christian Mollekopf wrote: > > Looks reasonable to me. I'll apply the patch locally and test it for a > > while. > > Christian Mollekopf wrote: > This patch brings the original problem back, that shared folders do not > appear until something causes a dataChanged signal (usually a sync). Since > the model now seems to be behaving correctly, I assume the kmail model stack > is buggy in yet another place (would have been to trivial otherwise wouldn't > it?), and the superfluous dataChanged signal just happened to hide that > problem. I tracked the problem down to still be in FolderTreeWidgetProxyModel (which is a KRecursiveFilterProxyModel). I know the relevant index makes it through the sourcemodel because of the debug output added in FolderTreeWidgetProxyModel::acceptRow (which also returns the correct values). I'm fairly sure that the model is the cause for the folder not showing up because I set the foldertreewidgetproxymodel directly as source of the folderTreeView (a QTreeView). Given that the filtering seems to work correctly, and assuming QTreeView isn't buggy, the reason for the folder not showing up has to be in KRecursiveFilterProxyModel (right?). The problem is most likely timing/order dependant because I cannot reproduce the problem with another user but the same kmail/kdelibs version + the same account. The problematic folder tree looks as follows: ``` * Shared Folders (no mimetype) ** shared (no mimetype) *** lists (no mimetype) kde.org (no mimetype) * pim (mail mimetype) * ... ... *** ... ``` The problem is that the complete folder hierarchy, including "Shared Folders" doesn't make it into the visible tree. The top 4 folders of the hierarch would of course only be pulled in by the actual mail folder (pim). So far I couldn't find the actual problem to write another testcase, but I have to assume that KRecursiveFilterProxyModel ist still buggy, and the additional dataChanged signal just happen to rectify the problem. - Christian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122252/#review74753 --- On Jan. 25, 2015, 6:51 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122252/ > --- > > (Updated Jan. 25, 2015, 6:51 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > --- > > by using an internal cache of the filtering state. > > The tricky part is that filterAcceptsRow() must not use the cache > (too bad, it would be faster), because of the setFilterFixedString() > feature which can change the filtering status of indexes without > KRFPM even being informed (QSFPM has no virtual method, no event...) > So it only ever writes to the cache, but when dataChanged() > or row insertion/removal comes in, we can look into the cache > to find the old state and compare. > > > Diffs > - > > kdeui/itemviews/krecursivefilterproxymodel.h > c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4 > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/122252/diff/ > > > Testing > --- > > Unit tests. > > Using kmail (and especially testing the substring filtering in the "move > dialog", which an earlier version of this patch broke). > Looking at the number of calls to > Akonadi::FavoriteCollectionsModel::Private::reload() for a single > dataChanged() signal from the ETM, which went from 10 to 4 (still too many, > but the remaining problem is elsewhere). > > > Thanks, > > David Faure > >
Re: Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals
> On Jan. 26, 2015, 9:41 a.m., Christian Mollekopf wrote: > > Looks reasonable to me. I'll apply the patch locally and test it for a > > while. This patch brings the original problem back, that shared folders do not appear until something causes a dataChanged signal (usually a sync). Since the model now seems to be behaving correctly, I assume the kmail model stack is buggy in yet another place (would have been to trivial otherwise wouldn't it?), and the superfluous dataChanged signal just happened to hide that problem. - Christian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122252/#review74753 --- On Jan. 25, 2015, 6:51 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122252/ > --- > > (Updated Jan. 25, 2015, 6:51 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > --- > > by using an internal cache of the filtering state. > > The tricky part is that filterAcceptsRow() must not use the cache > (too bad, it would be faster), because of the setFilterFixedString() > feature which can change the filtering status of indexes without > KRFPM even being informed (QSFPM has no virtual method, no event...) > So it only ever writes to the cache, but when dataChanged() > or row insertion/removal comes in, we can look into the cache > to find the old state and compare. > > > Diffs > - > > kdeui/itemviews/krecursivefilterproxymodel.h > c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4 > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/122252/diff/ > > > Testing > --- > > Unit tests. > > Using kmail (and especially testing the substring filtering in the "move > dialog", which an earlier version of this patch broke). > Looking at the number of calls to > Akonadi::FavoriteCollectionsModel::Private::reload() for a single > dataChanged() signal from the ETM, which went from 10 to 4 (still too many, > but the remaining problem is elsewhere). > > > Thanks, > > David Faure > >
Re: Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122252/#review74753 --- Looks reasonable to me. I'll apply the patch locally and test it for a while. - Christian Mollekopf On Jan. 25, 2015, 6:51 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122252/ > --- > > (Updated Jan. 25, 2015, 6:51 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > --- > > by using an internal cache of the filtering state. > > The tricky part is that filterAcceptsRow() must not use the cache > (too bad, it would be faster), because of the setFilterFixedString() > feature which can change the filtering status of indexes without > KRFPM even being informed (QSFPM has no virtual method, no event...) > So it only ever writes to the cache, but when dataChanged() > or row insertion/removal comes in, we can look into the cache > to find the old state and compare. > > > Diffs > - > > kdeui/itemviews/krecursivefilterproxymodel.h > c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4 > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/122252/diff/ > > > Testing > --- > > Unit tests. > > Using kmail (and especially testing the substring filtering in the "move > dialog", which an earlier version of this patch broke). > Looking at the number of calls to > Akonadi::FavoriteCollectionsModel::Private::reload() for a single > dataChanged() signal from the ETM, which went from 10 to 4 (still too many, > but the remaining problem is elsewhere). > > > Thanks, > > David Faure > >
Re: Review Request 122227: KRecursiveFilterProxyModel: many many more unittests, and fixing what they found.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/17/#review74744 --- Ship it! I tested this over the weekend. It fixes the problem originally addressed and seems to fix the crash I was having. - Christian Mollekopf On Jan. 23, 2015, 6:11 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/17/ > --- > > (Updated Jan. 23, 2015, 6:11 p.m.) > > > Review request for kdelibs and Christian Mollekopf. > > > Repository: kdelibs > > > Description > --- > > 1) setData(false), i.e. a dataChanged that removes and item from the filter, > didn't actually lead to removal. The code was only looking at changing to > get in, not changing to get out. > > 2) On insertion, we can avoid emitting dataChanged up the chain, by > finding out before the insertion which exact ancestor will be changed > (lastHiddenAscendantForInsert). > > 3) On removal, well simplify the code (completeRemove was always true, unless > ignoreRemove was set, so we only need to keep ignoreRemove), and avoid > emitting dataChanged up the chain, by finding out which the last parent > before one that should still be visible, and hide just that one. > > 4) While at it, an obvious optimization that could have been done > since day one: filterAcceptsRow can return true as soon as a child wants > to be shown. > > > Diffs > - > > kdeui/itemviews/krecursivefilterproxymodel.cpp > efa286ad87ded962b20c8a581b659d1b154ebf3a > kdeui/tests/krecursivefilterproxymodeltest.cpp > 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 > > Diff: https://git.reviewboard.kde.org/r/17/diff/ > > > Testing > --- > > Unittest, obviously. > + KMail smoke testing. > > > Thanks, > > David Faure > >
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- (Updated Jan. 22, 2015, 5:35 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and Stephen Kelly. Bugs: 338950 http://bugs.kde.org/show_bug.cgi?id=338950 Repository: kdelibs Description --- (This problem probably applies to KF5 as well, and we'll need to forward port this patch.) KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- (Updated Jan. 22, 2015, 4:06 p.m.) Review request for kdelibs and Stephen Kelly. Bugs: 338950 http://bugs.kde.org/show_bug.cgi?id=338950 Repository: kdelibs Description --- (This problem probably applies to KF5 as well, and we'll need to forward port this patch.) KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs (updated) - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- (Updated Jan. 22, 2015, 3:11 p.m.) Review request for kdelibs and Stephen Kelly. Bugs: 338950 http://bugs.kde.org/show_bug.cgi?id=338950 Repository: kdelibs Description --- (This problem probably applies to KF5 as well, and we'll need to forward port this patch.) KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs (updated) - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
> On Jan. 22, 2015, 7:47 a.m., David Faure wrote: > > kdeui/itemviews/krecursivefilterproxymodel.cpp, line 149 > > <https://git.reviewboard.kde.org/r/120119/diff/1/?file=310592#file310592line149> > > > > refreshAll isn't used anymore, the new code always "refreshes all > > ascendants" The default is false which is used mostly, except on line 257 - Christian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/#review74519 ------- On Jan. 22, 2015, 3:11 p.m., Christian Mollekopf wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120119/ > --- > > (Updated Jan. 22, 2015, 3:11 p.m.) > > > Review request for kdelibs and Stephen Kelly. > > > Bugs: 338950 > http://bugs.kde.org/show_bug.cgi?id=338950 > > > Repository: kdelibs > > > Description > --- > > (This problem probably applies to KF5 as well, and we'll need to forward port > this patch.) > > > KRecursiveFilterProxyModel: Fixed the model > > The model was not working properly and didn't include all items under > some circumstances. > This patch fixes the following scenarios in particular: > > * The change in sourceDataChanged is required to fix the shortcut condition. > The idea is that if the parent is already part of the model (it must be if > acceptRow returns true), > we can directly invoke dataChanged on the parent, resulting in the changed > index > getting reevaluated. However, because the recursive filterAcceptsRow version > was used > the shortcut was also used when only the current index matches the filter and > the parent index is in fact not yet in the model. In this case we failed to > call > dataChanged on the right index and thus the complete branch was never added > to the model. > > * The change in refreshAscendantMapping is required to include indexes that > were > included by descendants. The intended way how this was supposed to work is > that we > traverse the tree upwards and find the last index that is not yet part of the > model. > We would then call dataChanged on that index causing it and its descendants > to get reevaluated. > However, acceptRow does not reflect wether an index is already in the model > or not. > Consider the following model: > > - A > - B > - C > - D > > > If C is include in the model by default but D not and A & B only gets > included due to C, we have the following model: > - A > - B > - C > - D > > If we then call refreshAscendantsMapping on D it will not consider B as > already being part of the model. > This results in the toplevel index A being considered lastAscendant, and a > call to dataChanged on A results in > a reevaluation of A only, which is already in the model. Thus D never gets > added to the model. > > Unfortunately there is no way to probe QSortFilterProxyModel for indexes that > are > already part of the model. Even the const mapFromSource internally creates a > mapping when called, > and thus instead of revealing indexes that are not yet part of the model, it > silently > creates a mapping (without issuing the relevant signals!). > > As the only possible workaround we have to issues dataChanged for all > ancestors > which is ignored for indexes that are not yet mapped, and results in a > rowsInserted > signal for the correct indexes. It also results in superfluous dataChanged > signals, > since we don't know when to stop, but at least we have a properly behaving > model > this way. > > > Diffs > - > > kdeui/itemviews/krecursivefilterproxymodel.cpp > 6d6563166bcc9637d826f577925c47d5ecbef2cd > kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 > kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/120119/diff/ > > > Testing > --- > > > Thanks, > > Christian Mollekopf > >
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
> On Jan. 22, 2015, 7:47 a.m., David Faure wrote: > > The commit log has the same tree twice. Was "D" supposed to be omitted from > > the second tree? > > > > Overall I'm extremely surprised that this class didn't have a unittest yet. > > I recently wrote many proxymodels for a customer, with full test coverage, > > so this hurts my eyes. I will expand this unittest once it's in. > > > > If I understand correctly, the bug in refreshAscendantMapping is that > > basically the code was hoping to test the "old" state of the tree, while in > > fact acceptRow returns the new state already (so in this case, acceptRow > > returns true for every ascendant, so we don't know where to stop). A cache > > (remembering what we have inserted into the tree) would solve this, but > > managing the cache would be so much more work. > > A more clever solution could be to store the state of things (in fact just > > "lastAscendant") in a slot connected to rowsAboutToBeInserted, and use that > > later in the slot connected to rowsInserted. What do you think of this > > approach? > > > > (Personally I think we need more unittests before making any kind of change > > :-) "D" indeed was supposed to be omitted from the second tree. More tests for that model are clearly not a bad idea ;-) If we could test the old state that could indeed solve the problem, not sure what the original intentations fo the code where. In general we simply need a way to ask the model wether it already has a mapping for a certain index, but I couldn't find anything providing this information. So IMO it's either QSortFilterProxyModel's fault that it doesn't provide that information and we have to workaround that, or we shouldn't be using QSortFilterProxyModel. The caching of the information does sound like yet another thing that can break in this already fragile construct, so I'd rather avoid it. Besides performance superfluous dataChanged signals shouldn't hurt, so I'd suggest to stick to that. In the long run we should figure out wether we can adjust QSortFilterProxyModel to facilitate our usecase, or move away from a QSortFilterProxyModel based implementation. > On Jan. 22, 2015, 7:47 a.m., David Faure wrote: > > kdeui/tests/krecursivefilterproxymodeltest.cpp, line 126 > > <https://git.reviewboard.kde.org/r/120119/diff/1/?file=310594#file310594line126> > > > > Shouldn't rowsInserted even be emitted 3 times? Once for row1, once for > > child, once for subchild? AFAIK our models typically query for children, so toplevel is enough if children are immediately available. > On Jan. 22, 2015, 7:47 a.m., David Faure wrote: > > kdeui/tests/krecursivefilterproxymodeltest.cpp, line 159 > > <https://git.reviewboard.kde.org/r/120119/diff/1/?file=310594#file310594line159> > > > > I would also expect 3 signal emissions here same reason as above - Christian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/#review74519 --- On Sept. 10, 2014, 8:15 a.m., Christian Mollekopf wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120119/ > --- > > (Updated Sept. 10, 2014, 8:15 a.m.) > > > Review request for kdelibs and Stephen Kelly. > > > Bugs: 338950 > http://bugs.kde.org/show_bug.cgi?id=338950 > > > Repository: kdelibs > > > Description > --- > > (This problem probably applies to KF5 as well, and we'll need to forward port > this patch.) > > > KRecursiveFilterProxyModel: Fixed the model > > The model was not working properly and didn't include all items under > some circumstances. > This patch fixes the following scenarios in particular: > > * The change in sourceDataChanged is required to fix the shortcut condition. > The idea is that if the parent is already part of the model (it must be if > acceptRow returns true), > we can directly invoke dataChanged on the parent, resulting in the changed > index > getting reevaluated. However, because the recursive filterAcceptsRow version > was used > the shortcut was also used when only the current index matches the filter and > the parent index is in fact not yet in the model. In this case we failed to > call > dataChanged on the right index and thus the complete branch was never
Re: Changes to our Git infrastructure
On Tuesday 23 December 2014 13.21:37 Milian Wolff wrote: > On Wednesday 24 December 2014 00:20:18 Ben Cooksley wrote: > > Hi all, > > > > As has been made evident in the prior thread there are quite a few > > interesting ideas floating around about what our Git infrastructure > > should be capable of. > > > > Our current one was constructed when KDE first seriously migrated to > > Git following the Gitorious experiments, and it shows. (As a sysadmin > > I can attest that parts of it are held together by digital glue and > > tape). > > > > Before we go ahead and jump to a new platform though - we need to know > > what we want. > > Can everyone please suggest what they think are the things they'd like > > to see feature wise? > > > > In doing so, please refrain from mentioning any existing solutions - > > all we want to do at this point is construct a wishlist of what people > > would like to see the system be capable of. > > > > Items that we (sysadmin) would like to see community comment on > > include the code review system, clone and scratch repositories and how > > they function, and whether people would be bothered by repository > > locations changing as they move around. Also useful would be comment > > on how anongit and other systems that rely on the Git infastructure > > work. > > Here's a list of things that I'd like to have for my workflow, which is > basically two folded: On one hand, I actively develop new software and new > features. On the other hand, I spent a considerable amount of KDE time > reviewing other peoples work. > > ## The Developer > > For productivity, I'd love to have the ability to push changes directly from > the command line to somewhere others can then review the code. This should > support both, individual commits as well as feature branches. I.e. when > pushing a merge request for review, I still want others to see individual > commits. Updating such reviews should be trivial and keeps the review > history intact. I can just continue hacking and push new changes as they > come without influencing the previous patches sent for review. > > Optimally, I'm never forced to go to a website to manipulate the review. > I.e. to add reviewers, or select branches or other metadata. Nor do I want > to be forced to manually "publish" a review, I just pushed it for review > after all. > > ## The Reviewer > > Here, my biggest wish reflects that of the developer: I want to be able to > see the developers intent, i.e. individual commits that make up one bigger > mergeable hunk. I want to comment directly on lines of code in the patch. > The patch should be displayed as easily readable as possible, with syntax > highlighting and side-by-side diff view. As much as possible should be > shown on a single page, I do not want to be forced to navigate between > different files or the like, I need to see the big picture. > > Optimally, I could apply hotfixes, such as whitespace changes, directly when > doing the review. Finally, once the review is good to go, I want to click a > button (or better: hotkey) to merge the patch into the upstream branch. > > Furthermore, I'd like to use the same review mechanism for post-review. When > a patch is triggering problems, I'd like to start a discussion in the > context of the commit that was merged. Again, I want to annotate source > lines. So rather than sending mails to kde-commits which are then lost, I > want to have that tracked on a website for others to see. > Excellent summary, I fully agree. Cheers, Christian
Re: Changes to our Git infrastructure
On Monday 29 December 2014 11.23:19 Ben Cooksley wrote: > Hi all, > > Based on the current feedback: > > 1) It seems people see no use in clone repositories. > 2) Little commentary has been made on the merits of scratch > repositories, with some dismissing them as pointless. > > Therefore sysadmin proposes that both clone and scratch repositories > be eliminated as a concept with the next iteration of our Git > infrastructure. Having some mechanism where a developer can just create his own repositories that may or may not be shared with others is IMO essential. With the current infrastructure scratch repositories provide this mechanism and if not replaced with something similar I don't think it would be a good idea to remove this functionality. I find clones useful to publish my "private" branches somewhere, either as backup, or to share with someone. I also use force-pushes sometimes on those to i.e. cleanup a patch queue, and to avoid proliferation of branches by using $branchv1, $branchv2, ... If we established that everyone pushes his branches into a prefix (cmollekopf/lksjflsjdf), and we can force-push and delete branches, then clones become IMO less important. Cheers, Christian
Re: Changes to branch management
On Wednesday 24 December 2014 00.04:22 Ben Cooksley wrote: > Hi all, > > As the other thread has gotten a bit congested with various threads, I > thought I would split up the topics to make things a bit easier to > manage. > > The first seems the least contentious: allowing all developers to > delete branches on our mainline repositories, except for certain > protected branches (like "master" and "KDE/*" for instance). > > Any suggestions or variations on this? > This would be a great improvement indeed. I assume this would also work on scratch repositories? Since one person has to "own" a scratch repository, I think other people should still be able to delete branches in that repository. Cheers, Christian
Re: [Kde-pim] Problems with infrastructure
On Wednesday 10 December 2014 16.53:09 Jan Kundrát wrote: > On Wednesday, 10 December 2014 10:28:59 CEST, Christian Mollekopf wrote: > > * pull requests/the webinterface: reviewboard is awesome for single > > patches > > every now and then, it's rather useless when you work with > > branches IMO. With github we have a nice webinterface to review > > branches while keeping it simple. > > Gerrit may be what I'm looking for though (never used it, > > looking forward to see how the testing goes). > > That depends on what you're looking for. Gerrit puts emphasis on review of > individual patches. It will present related patches "together", but you > won't be able to review them as a single entity -- the goal of Gerrit is to > ensure that the history is clean, and it is operating under an assumption > that each commit matters. Unless I'm mistaken, a GitHub pull request shows > you a diff which represents the starting point and the final point of that > branch as a single unit, with an option to go into individual commits. > Gerrit is different -- not worse, different :). > I do like being able to view patches individually, so that's fine by me. I just want to avoid sending and reviewing individual patches when they belong together. The current workflow I'm using for reviewing is that I review feature branches developed by others, and if I'm happy with them I merge them and delete them from upstream. What I lack with that is of course a tool to communicate about defects in the provided patches. Reviewboard can be used for that, but creating a reviewboard entry for every commit is IMO too much work, and having all commits merged isn't all that useful as it get's messy. So if gerrit allows to treat feature branches as a whole while not merging the commits it may just be what I need. > Regarding the "testing of Gerrit", I haven't received much feedback yet. > Three repositories are using it so far (kio, plasma-framework, trojita), > and if a repo owner/maintainer of some other project is willing to take > part in this testing, I won't see a problem with it. > Thanks for the offer. Right now I lack the time for this experiment, but I'll get back to you when I see an opportunity. Cheers, Christian
Re: [Kde-pim] Problems with infrastructure
On Wednesday 10 December 2014 15.27:31 Ben Cooksley wrote: > Hi all, > > It has come to my attention that some developers have "issues" with > KDE infrastructure in certain areas. This is the first time i've heard > of these "problems" and to my knowledge nobody has ever spoken to > sysadmin regarding them. > > If people have an issue, can they please actually raise it so we can > discuss the matter and reach an agreement on something which would > sort out your problem - and might actually help others too? > That might have been me, since I recently used github for some repos/branches that could have ended up on kde infrastructure. I did that mostly because I thought it didn't matter, because I wanted to try something else, and because I was too lazy to write this mail. So, since I used github a bit now, let me share my whishlist with you =) * deleting branches: This is the only major gripe I have with the kde infrastructure. I think everyone should be able to delete branches (except some blacklisted ones). If I cannot delete my branches when I no longer need them I try to avoid pushing them, which doesn't help. Personal clones are not a solution IMO because you have to manage additional remotes. IMO the benefits outweigh the danger of someone accidentally deleting someone else's branch. Perhaps a naming scheme could be established for such branches, such as: dev/$foo or feature/$foo * pull requests/the webinterface: reviewboard is awesome for single patches every now and then, it's rather useless when you work with branches IMO. With github we have a nice webinterface to review branches while keeping it simple. Gerrit may be what I'm looking for though (never used it, looking forward to see how the testing goes). * The rest I suppose is mostly psychological: ** On github I can click my way to a new repo, on kde I have to lookup a command (being able to do it from the commandline in the first place is a benefit though) ** I find githubs webinterface prettier and more useful. I actually use it sometimes, and I never use projects.kde.org or quickgit.kde.org. If you're interested I could try to figure out what it actually is that I like more, because I can't really tell right now. I know the last few are a bit silly but it's what could make the decision, unless I make the conscious decision that I want it on kde infrastructure because it's kde infrastructure (i.e. for community cohesion). This is quite an abstract thought, that although we all should be able to make it, without it it's a free market where sexyness may win ;-) Anyways, thanks for doing a great job and caring, I'll try to be a bit more helpful in the future as well. Cheers, Christian
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- (Updated Sept. 10, 2014, 8:15 a.m.) Review request for kdelibs and Stephen Kelly. Changes --- added not that this applies to KF5 as well. Bugs: 338950 http://bugs.kde.org/show_bug.cgi?id=338950 Repository: kdelibs Description (updated) --- (This problem probably applies to KF5 as well, and we'll need to forward port this patch.) KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- (Updated Sept. 9, 2014, 4:26 p.m.) Review request for kdelibs and Stephen Kelly. Bugs: 338950 http://bugs.kde.org/show_bug.cgi?id=338950 Repository: kdelibs Description --- KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
Re: Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- (Updated Sept. 9, 2014, 4:18 p.m.) Review request for kdelibs. Bugs: 338950 http://bugs.kde.org/show_bug.cgi?id=338950 Repository: kdelibs Description --- KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
Review Request 120119: KRecursiveFilterProxyModel: Fixed the model
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120119/ --- Review request for kdelibs. Repository: kdelibs Description --- KRecursiveFilterProxyModel: Fixed the model The model was not working properly and didn't include all items under some circumstances. This patch fixes the following scenarios in particular: * The change in sourceDataChanged is required to fix the shortcut condition. The idea is that if the parent is already part of the model (it must be if acceptRow returns true), we can directly invoke dataChanged on the parent, resulting in the changed index getting reevaluated. However, because the recursive filterAcceptsRow version was used the shortcut was also used when only the current index matches the filter and the parent index is in fact not yet in the model. In this case we failed to call dataChanged on the right index and thus the complete branch was never added to the model. * The change in refreshAscendantMapping is required to include indexes that were included by descendants. The intended way how this was supposed to work is that we traverse the tree upwards and find the last index that is not yet part of the model. We would then call dataChanged on that index causing it and its descendants to get reevaluated. However, acceptRow does not reflect wether an index is already in the model or not. Consider the following model: - A - B - C - D If C is include in the model by default but D not and A & B only gets included due to C, we have the following model: - A - B - C - D If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in a reevaluation of A only, which is already in the model. Thus D never gets added to the model. Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are already part of the model. Even the const mapFromSource internally creates a mapping when called, and thus instead of revealing indexes that are not yet part of the model, it silently creates a mapping (without issuing the relevant signals!). As the only possible workaround we have to issues dataChanged for all ancestors which is ignored for indexes that are not yet mapped, and results in a rowsInserted signal for the correct indexes. It also results in superfluous dataChanged signals, since we don't know when to stop, but at least we have a properly behaving model this way. Diffs - kdeui/itemviews/krecursivefilterproxymodel.cpp 6d6563166bcc9637d826f577925c47d5ecbef2cd kdeui/tests/CMakeLists.txt f661b9177a6e0e1de7f49bc3cb9fbb5e04f427c1 kdeui/tests/krecursivefilterproxymodeltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120119/diff/ Testing --- Thanks, Christian Mollekopf
