Re: Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-12 Thread Aleix Pol Gonzalez 

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



autotests/kextracolumnsproxymodeltest.cpp (line 1)


It should have a licence.


I think this is really cool. :)

+1

- Aleix Pol Gonzalez


On July 12, 2015, 1:13 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 12, 2015, 1:13 p.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> New proxy: KExtraColumnsProxyModel, allows to add columns to an existing 
> model.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-13 Thread David Faure 

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

(Updated July 13, 2015, 8:31 a.m.)


Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.


Changes
---

add missing license header to autotest


Repository: kitemmodels


Description (updated)
---

REVIEW: 124331


Diffs (updated)
-

  autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
  autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
  autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
  src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
  src/kextracolumnsproxymodel.h PRE-CREATION 
  src/kextracolumnsproxymodel.cpp PRE-CREATION 

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


Testing
---

Wrote autotests (as extensive as possible); used this in one real project, 
AFAIK Jesper (blackie) did as well.


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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-13 Thread Sune Vuorela 

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


I like the concept, and have a whole bunch of nitpickery and a couple of more 
important bits.


src/kextracolumnsproxymodel.h (line 92)


where is the implementation of this? (and the unit tests ?  :)



src/kextracolumnsproxymodel.cpp (line 43)


Are this one of the cases where mmutz and mwolff won't hunt you down for 
using QList ?



src/kextracolumnsproxymodel.cpp (line 77)


Is this assert actually needed? isn't it kind of valid to pass in a nullptr 
?



src/kextracolumnsproxymodel.cpp (line 78)


shouldn't the old source model's about to be changed signals be 
disconnected here, or are QAPM taking care of that ?



src/kextracolumnsproxymodel.cpp (line 99)


isn't this kind of a normal state. is noisy output warranted here?



src/kextracolumnsproxymodel.cpp (line 115)


I know it is widely used outside Qt but .. Q_D is actually not public Qt 
api. Similar for Q_Q.



src/kextracolumnsproxymodel.cpp (line 170)


this is surprising me a bit. I'd expect that I by default would have 
emitted dataChanged inside my implementation of setExtraColumnData, especially 
if setting a piece of data influences multiple roles.



src/kextracolumnsproxymodel.cpp (line 194)


Wouldn't it be better to redelegate this to a set of separate functions, 
just like the actual data so that other roles are actually supported? (In my 
similar, but not as thorough implementations of this, I've often needed other 
roles like color and/or decoration)


- Sune Vuorela


On July 13, 2015, 8:31 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 13, 2015, 8:31 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-14 Thread David Faure 


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.h, line 92
> > 
> >
> > where is the implementation of this? (and the unit tests ?  :)

Ouch, good catch, I forgot to finish this. Done now.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 43
> > 
> >
> > Are this one of the cases where mmutz and mwolff won't hunt you down 
> > for using QList ?

No, sizeof(QPersistentModelIndex) = one pointer, so it's fine. (and I got this 
from QIdentityProxyModel, which mmutz would have fixed by now if it was wrong 
;) ).


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 78
> > 
> >
> > shouldn't the old source model's about to be changed signals be 
> > disconnected here, or are QAPM taking care of that ?

Good point, fixed.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 99
> > 
> >
> > isn't this kind of a normal state. is noisy output warranted here?

No, this should never ever happen. Every time it does, it means something will 
go terribly wrong very soon, i.e. it's a bug in the proxy which doesn't 
reimplement some method that ends up going into that code path. Putting a 
breakpoint on this line has been very useful to fix all the bugs :-)


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 115
> > 
> >
> > I know it is widely used outside Qt but .. Q_D is actually not public 
> > Qt api. Similar for Q_Q.

At this point, it's so widely used in KDE and other libs, that we would all 
fight for some compatibility, if this was ever to change in Qt.
I claim this is fine, based on the amount of prior art (332 instances of Q_Q 
and 1739 instances of Q_D in all the frameworks).

Anyhow, they're both one-liners, we can easily replicate them locally if this 
is a problem one day.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 170
> > 
> >
> > this is surprising me a bit. I'd expect that I by default would have 
> > emitted dataChanged inside my implementation of setExtraColumnData, 
> > especially if setting a piece of data influences multiple roles.

I thought this would make things easier, especially since setExtraColumnData 
doesn't get a QModelIndex, so you wouldn't be able to emit dataChanged. The 
whole idea being that the proxy reimplementation doesn't need to know/care 
about how many the source model has.
But yeah, extraColumnDataChanged actually allows doing this just fine, I'll 
change it.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 194
> > 
> >
> > Wouldn't it be better to redelegate this to a set of separate 
> > functions, just like the actual data so that other roles are actually 
> > supported? (In my similar, but not as thorough implementations of this, 
> > I've often needed other roles like color and/or decoration)

In the headers?

I see two solutions: 1) a virtual headerDataForExtraColumn, 2) letting you 
reimplement headerData and using extraColumnForProxyColumn() from there (I'll 
make it protected in any case, for anything the proxy doesn't provide out of 
the box, like DnD support...).

What do you think?

I'm not sure we should provide a new set of virtual methods methods for 
everything (this reminds me of Q3ScrollView::viewportMouseMoveEvent and friends 
:-). Data mapping is the hard part, header data isn't.


- David


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


On July 13, 2015, 8:31 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 13, 2015, 8:31 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44

Re: Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-14 Thread David Faure 

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

(Updated July 14, 2015, 10:42 a.m.)


Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.


Changes
---

implemented and unittested extraColumnDataChanged; other changes as suggested 
by Sune.


Repository: kitemmodels


Description
---

REVIEW: 124331


Diffs (updated)
-

  autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
  autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
  autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
  src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
  src/kextracolumnsproxymodel.h PRE-CREATION 
  src/kextracolumnsproxymodel.cpp PRE-CREATION 

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


Testing
---

Wrote autotests (as extensive as possible); used this in one real project, 
AFAIK Jesper (blackie) did as well.


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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-14 Thread Milian Wolff 

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

Ship it!



src/kextracolumnsproxymodel.cpp (line 36)


the slots here is not required, no? only in the public part? if it's 
required, then you also need a 'public:' section below for the data members



src/kextracolumnsproxymodel.cpp (line 44)


these two really should be QVector. As far as I can see, you do not 
interact with the Qt API and thus need QList for them, or am I missing 
something? QModelIndex is too large, and thus you'll allocate every item in the 
list on the heap. int is, on 64bit, too small and thus you waste space (factor 
of 2).



src/kextracolumnsproxymodel.cpp (line 85)


here and elsewhere: why not use the newstyle connect syntax?


- Milian Wolff


On July 14, 2015, 10:42 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 14, 2015, 10:42 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-14 Thread Sune Vuorela 

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



src/kextracolumnsproxymodel.h (line 39)


Are there any of these "not supported" things we might want to support in 
the future? Can we implement e.g. adding/removing columns at runtime and still 
keep the abi/api ?
I guess for removing columns, one could just add a QSFPM on top and filter 
out the column.


- Sune Vuorela


On July 14, 2015, 10:42 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 14, 2015, 10:42 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-15 Thread David Faure 


> On July 14, 2015, 4:58 p.m., Milian Wolff wrote:
> > src/kextracolumnsproxymodel.cpp, line 44
> > 
> >
> > these two really should be QVector. As far as I can see, you do not 
> > interact with the Qt API and thus need QList for them, or am I missing 
> > something? QModelIndex is too large, and thus you'll allocate every item in 
> > the list on the heap. int is, on 64bit, too small and thus you waste space 
> > (factor of 2).

This isn't QModelIndex, but QPersistentModelIndex, which has a d pointer, so 
its size is that of a pointer, no problem there.
You're right about int though. QIdentityProxyModel has the same problem then.
You're right about this being internal.

I'll change it to QVector for both, although I maintain that the QPMI one was 
fine ;)


> On July 14, 2015, 4:58 p.m., Milian Wolff wrote:
> > src/kextracolumnsproxymodel.cpp, line 85
> > 
> >
> > here and elsewhere: why not use the newstyle connect syntax?

I tried, doesn't work with private slots. &MyQObject::method requires 'method' 
to be in MYQObject, but with Q_PRIVATE_SLOT the method is in the private class, 
while the QObject is the public class.


- David


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


On July 14, 2015, 10:42 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 14, 2015, 10:42 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-15 Thread David Faure 


> On July 15, 2015, 5:54 a.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.h, line 39
> > 
> >
> > Are there any of these "not supported" things we might want to support 
> > in the future? Can we implement e.g. adding/removing columns at runtime and 
> > still keep the abi/api ?
> > I guess for removing columns, one could just add a QSFPM on top and 
> > filter out the column.

I don't know if any of this will be needed in the future, but the API will 
allow for it:
1) making appendColumn emit columnsInserted, if it should work with a live proxy
2) adding a removeExtraColumn method

"having a different number of columns in subtrees" isn't really useful with the 
available views from Qt anyway

dnd and move support is just more virtuals to reimplement. Ah, we won't be able 
to delegate to new virtuals though. So it'll have to be "reimplement the 
existing virtuals, e.g. dropMimeData, and check the column number", i.e. 
documenting how the app should do it.


- David


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


On July 14, 2015, 10:42 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 14, 2015, 10:42 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-15 Thread David Faure 

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

(Updated July 15, 2015, 8:59 a.m.)


Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.


Changes
---

use QVector


Repository: kitemmodels


Description
---

REVIEW: 124331


Diffs (updated)
-

  autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
  autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
  autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
  src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
  src/kextracolumnsproxymodel.h PRE-CREATION 
  src/kextracolumnsproxymodel.cpp PRE-CREATION 

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


Testing
---

Wrote autotests (as extensive as possible); used this in one real project, 
AFAIK Jesper (blackie) did as well.


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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-15 Thread Milian Wolff 

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

Ship it!


Yu are right about QPMI, but it just shows again how much simpler the world 
would be if we'd simply use QVector everywhere. Ah, I'll continue to dream :)

Thanks for fixing it though!


src/kextracolumnsproxymodel.cpp (line 85)


ah, a pity. Btw, in KDevelop where I can use lambdas (can you here?), I 
stopped using private signals and instead do something like

connect(foo, &Foo::bar,
this, [this] (...) { d->onBar(...); });

I quite like that, as it does not leak anything to the public header at all.


- Milian Wolff


On July 15, 2015, 8:59 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 15, 2015, 8:59 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-15 Thread Mark Gaiser 


> On jul 15, 2015, 9:36 a.m., Milian Wolff wrote:
> > src/kextracolumnsproxymodel.cpp, line 85
> > 
> >
> > ah, a pity. Btw, in KDevelop where I can use lambdas (can you here?), I 
> > stopped using private signals and instead do something like
> > 
> > connect(foo, &Foo::bar,
> > this, [this] (...) { d->onBar(...); });
> > 
> > I quite like that, as it does not leak anything to the public header at 
> > all.

Yes you can use them unconditionally in frameworks. 
https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11


- Mark


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


On jul 15, 2015, 8:59 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated jul 15, 2015, 8:59 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-17 Thread David Faure 


> On July 15, 2015, 9:36 a.m., Milian Wolff wrote:
> > src/kextracolumnsproxymodel.cpp, line 85
> > 
> >
> > ah, a pity. Btw, in KDevelop where I can use lambdas (can you here?), I 
> > stopped using private signals and instead do something like
> > 
> > connect(foo, &Foo::bar,
> > this, [this] (...) { d->onBar(...); });
> > 
> > I quite like that, as it does not leak anything to the public header at 
> > all.
> 
> Mark Gaiser wrote:
> Yes you can use them unconditionally in frameworks. 
> https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11

Good idea. One downside though: you can't disconnect that specific connection 
(e.g. at the beginning of setSourceModel).
The only thing I can think of is disconnect(foo, &Foo:bar, this, 0), which 
disconnects other slots too, if any.


- David


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


On July 15, 2015, 8:59 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated July 15, 2015, 8:59 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-17 Thread Nicolás Alvarez 


> On Jul. 15, 2015, 6:36 a.m., Milian Wolff wrote:
> > src/kextracolumnsproxymodel.cpp, line 85
> > 
> >
> > ah, a pity. Btw, in KDevelop where I can use lambdas (can you here?), I 
> > stopped using private signals and instead do something like
> > 
> > connect(foo, &Foo::bar,
> > this, [this] (...) { d->onBar(...); });
> > 
> > I quite like that, as it does not leak anything to the public header at 
> > all.
> 
> Mark Gaiser wrote:
> Yes you can use them unconditionally in frameworks. 
> https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11
> 
> David Faure wrote:
> Good idea. One downside though: you can't disconnect that specific 
> connection (e.g. at the beginning of setSourceModel).
> The only thing I can think of is disconnect(foo, &Foo:bar, this, 0), 
> which disconnects other slots too, if any.

You can disconnect lambdas if you save the return value of connect(). But imo 
that starts getting unnecessarily complicated, if the current code with a 
private slot already works...


- Nicolás


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


On Jul. 15, 2015, 5:59 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> ---
> 
> (Updated Jul. 15, 2015, 5:59 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> ---
> 
> REVIEW: 124331
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> ---
> 
> Wrote autotests (as extensive as possible); used this in one real project, 
> AFAIK Jesper (blackie) did as well.
> 
> 
> 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 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

2015-07-17 Thread David Faure 

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

(Updated July 17, 2015, 10:45 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.


Changes
---

Submitted with commit 8837e8d2d423c34c3921fd9f8b8c0e4f825c45f5 by David Faure 
to branch master.


Repository: kitemmodels


Description
---

REVIEW: 124331


Diffs
-

  autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
  autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
  autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
  src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
  src/kextracolumnsproxymodel.h PRE-CREATION 
  src/kextracolumnsproxymodel.cpp PRE-CREATION 

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


Testing
---

Wrote autotests (as extensive as possible); used this in one real project, 
AFAIK Jesper (blackie) did as well.


Thanks,

David Faure

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