> On Aug. 30, 2012, 12:17 p.m., Aurélien Gâteau wrote: > > plasma/declarativeimports/qtextracomponents/fullmodelaccess.h, line 25 > > <http://git.reviewboard.kde.org/r/106272/diff/2/?file=82325#file82325line25> > > > > I don't like the "FullModelAccess" name. It is a model, so to avoid > > confusion its name should end with "Model". It works like a proxy model, so > > I suggest renaming it to ColumnProxyModel. > > > > I am also wondering whether the code wouldn't be simpler if the class > > was inheriting from QAbstractProxyModel rather than QAbstractListModel. I > > think it would make it possible to remove: > > - data() > > - rowCount() > > - headerData() > > - sourceDestroyed() > > - sourceModel() > > Marco Martin wrote: > +1 for ColumnProxyModel. > good also to put more emphasys on the columns rather than the possibility > to dive in the tree nodes, that is here only because visualdatamodel can't be > used in conjunction with other proxies, since doesn't inherit from qaim > > Aleix Pol Gonzalez wrote: > I already tried that, but I didn't like it because it provides an API > that suggests that mapFrom/ToSource will work. > > Also reducing those won't help much. The big part of the code is the > forwarding of the signals and proxy models doesn't do that. I tried with > QIdentityProxyModel, but then it tries to forward all signals and we only > need some of them. > > Regarding the name, I like the addition of proxy, although I think it's > misleading if it doesn't inherit QAbstractProxyModel. I don't really like to > have "Column" there. > > Aurélien Gâteau wrote: > > it provides an API that suggests that mapFrom/ToSource will work > > Isn't it the case? I would indeed expect mapFrom/ToSource to work. If > they don't work then I agree the class should not inherit from > QAbstractProxyModel, and should have "Proxy" in its name. > > > Regarding the name, I like the addition of proxy, although I think it's > misleading if it doesn't inherit QAbstractProxyModel. I don't really like to > have "Column" there. > > This model gives you access to one column of a multi-column model, which > can be a tree model or a list model, that is why I think "Column" should be > in the name, but maybe it will be more often used to access the first column > of a subtree within a tree model. If it cannot inherit from > QAbstractProxyModel I suggest ListSubModel or SubListModel.
Regarding Proxies: well, in the proxy you have less items than in the source. That is all the other columns and tree branches don't have a representation so you keep returning invalid QModelIndex'es for those. Regarding the name: I think I'll change to ColumnProxyModel, I'm lacking any inspiration whatsoever in this regard and I don't want to stop this patch because of this :). - Aleix ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106272/#review18278 ----------------------------------------------------------- On Aug. 31, 2012, 3:45 p.m., Aleix Pol Gonzalez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106272/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2012, 3:45 p.m.) > > > Review request for Plasma, Aurélien Gâteau and Marco Martin. > > > Description > ------- > > This patch adds a component called ListifyModel (yeah, I hate the name too). > The idea behind is to expose as a QAbstractListModel any part of a > QAbstractItemModel. > > This solves the problem we have in QML given the limitation that ListView > only displays the first column of the root items. Here we can specify what > column we want and what root index we want to have. > > > Diffs > ----- > > plasma/declarativeimports/qtextracomponents/CMakeLists.txt 05a1195 > plasma/declarativeimports/qtextracomponents/fullmodelaccess.h PRE-CREATION > plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp > PRE-CREATION > plasma/declarativeimports/qtextracomponents/qtextracomponentsplugin.cpp > 429282e > plasma/declarativeimports/qtextracomponents/tests/CMakeLists.txt > PRE-CREATION > plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.h > PRE-CREATION > plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.cpp > PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/106272/diff/ > > > Testing > ------- > > There's a passing unit test, albeit limited. > I also tested it with a QML example I had with KPeople. If anybody is > interested I can provide it too. > > > Thanks, > > Aleix Pol Gonzalez > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
