----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106272/#review18278 -----------------------------------------------------------
plasma/declarativeimports/qtextracomponents/fullmodelaccess.h <http://git.reviewboard.kde.org/r/106272/#comment14453> include should be changed to #include <QAbstractListModel> plasma/declarativeimports/qtextracomponents/fullmodelaccess.h <http://git.reviewboard.kde.org/r/106272/#comment14448> 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() plasma/declarativeimports/qtextracomponents/fullmodelaccess.h <http://git.reviewboard.kde.org/r/106272/#comment14449> Properties should have a NOTIFY signal. This makes it possible to bind other properties to them. plasma/declarativeimports/qtextracomponents/fullmodelaccess.h <http://git.reviewboard.kde.org/r/106272/#comment14457> I assume you need this static method to easily init rootIndex. I think it should be renamed to something a bit more explicit, like indexForModelItem() plasma/declarativeimports/qtextracomponents/fullmodelaccess.h <http://git.reviewboard.kde.org/r/106272/#comment14450> This function should be const. plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp <http://git.reviewboard.kde.org/r/106272/#comment14452> Coding style: kdelibs coding style says brackets are mandatory for one-line if (there are other occurences of this further down). plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp <http://git.reviewboard.kde.org/r/106272/#comment14451> Maybe the content of this if() can be replaced with: disconnect(m_sourceModel, 0, this, 0); Given the number of connections, it would avoid future problems if one adds a new signal but forgets to add a disconnect() call there. plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp <http://git.reviewboard.kde.org/r/106272/#comment14454> Message should be more explicit, and should use kWarning() so that it includes class and method name. If using kWarning() is not an option, use qWarning() << __FUNCTION__ << ... plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.h <http://git.reviewboard.kde.org/r/106272/#comment14456> I see you're copy'n'pasting my precious unittest code :) plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.cpp <http://git.reviewboard.kde.org/r/106272/#comment14455> I think it would be interesting to also run ModelTest *after* a root index and source model has been set. - Aurélien Gâteau On Aug. 30, 2012, 10:43 a.m., Aleix Pol Gonzalez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106272/ > ----------------------------------------------------------- > > (Updated Aug. 30, 2012, 10:43 a.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 [email protected] https://mail.kde.org/mailman/listinfo/plasma-devel
