leinir marked 3 inline comments as done. leinir added inline comments. INLINE COMMENTS
> broulik wrote in author.h:111 > `ProfilePage`? In principle yes, except we can't change homepage to homePage, until KF6... and then we'd end up with mixed spelling styles, and that just makes me sad ;) > broulik wrote in commentsmodel.cpp:184 > Any particular reason not do just return a null `QVariant()`? Hmm... This actually is supposed to be "Unknown model role"... It's kind of corner-casey, but i've noticed in the past that something other than just an invalid QVariant is occasionally useful for those times where you've, say, missed the h in "depth" ;) Usually this wouldn't be hit, of course, so it's not actually expensive in any real way. But yes, compared to this, QVariant() would be the better option. > broulik wrote in commentsmodel.h:70 > Why is this an explicit `Engine` pointer rather than generic `QObject`? Entirely to make it awkward for people to use it in a counter-intended fashion :) > broulik wrote in commentsmodel.h:75 > You might want to set that to `Qt::DisplayRole` Hmm... i guess, except that it's not reeeally the obvious choice (because i don't really see one of those)... but yeah, having one is probably not terrible anyway. > broulik wrote in commentsmodel.h:88 > `QModelIndex` is usable from QML these days, this overload isn't neccessary, > you can do from QML: > > model.data(model.index(row, column), role); Aah! i did not know this, very handy indeed. Couple of places where that'll make life simpler :) > broulik wrote in engine.cpp:803 > ah, so this is why it's not `const` Indeed :) (and yes, that cache still needs to be made...) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D21721 To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
