Moving Plasma Media Center to extragear
david at davidedmundson.co.uk
Mon Apr 22 12:35:17 BST 2013
Few minor comments. Nothing particularly exciting.
All in all I was fairly impressed with the code, it's neat and tidy
and you've had 2 releases which is the right approach to getting in
Assuming you've addressed Albert's comments, +1 from me on this.
don't call beginResetModel() instead call invalidateFilter()
avoid creating a new KStandardDirs instance. Use KGlobal::dirs()
you're storing a QModelIndex for the thumb being fetched. Always store
QPersistantModelIndexes in case the model changes
BackendsModel and PlaylistsModel should be using a d-pointer, as they
are in the library and the headers are installed. (Assuming this is
meant to be a public API. The fact that you've already called the
release 1.0, to me implies it should be, people assume a lot from a
version number). Obviously fixing this will break the ABI... so I'm
not sure what you want to do.
Related, make sure you are setting your .so version number correctly.
It's on 0.9.. which could mean you've been ABI stable since 0.9 (which
is great) or it could mean you set it the first release and forgot
about it. I've not checked the git log to confirm either way.
whitespace is all over the place
Not enough comments, this applies to almost every software project
ever made, so this isn't a blocker - but as the maintainer I suggest
you keep an eye out for it, and start telling people to add comments
in review when they write something non-trivial.
I'm not sure the last release should have gone in "stable" on
download.kde.org given as it wasn't in extragear yet. It's too late to
change now, but I wanted to draw attention on the list to it. I think
we maybe need to define that better.
More information about the kde-core-devel