----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109611/#review31163 -----------------------------------------------------------
plasma/desktop/applets/kickoff/core/applicationmodel.h <http://git.reviewboard.kde.org/r/109611/#comment23185> Why is this a slot? It doesn't seem to be used as one? -> make it private. plasma/desktop/applets/kickoff/core/applicationmodel.h <http://git.reviewboard.kde.org/r/109611/#comment23167> const QString & Also, don't name this the same as the slot without argument, it's really confusing. See below for a naming suggestion. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23168> begin -> constBegin, end -> constEnd, to avoid detaching. Or use Q_FOREACH, more readable. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23170> foo.count() -> !foo.isEmpty() plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23169> This line serves no purpose (I presume the string is already empty, in the AppNode ctor). Same for other similar lines. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23171> remove trailing space plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23172> d->seenPrograms.isEmpty() plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23173> QString() for a null string. QString::null is a Qt3 leftover. OK, so clearly this is a helper for the slot, separated so it can be recursive. So call this createNewProgramListForPath(), or createNewProgramListHelper()? plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23174> What is "-" for? plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23183> Can it really be null? Did you see this elsewhere, or did you add it "just in case"? It seems unnecessary to me. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23177> |= is a bitwise operator, for booleans you have to write a = foo || a (in this order to avoid shortcut-evaluation). In this particular case it would be more readable to write if (createNewProgramList(...)) seenProgramsChanged = true; (shouldn't this variable be named seenNewPrograms, rather?) plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23176> Use QString::remove instead. But is there any purpose in removing the .desktop, actually? It's part of the storageId, which is the best key to use, I see no reason to do extra string manipulation just to shorten it. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23178> This whole for() loop could be replaced with something like index = d->seenPrograms.indexOf(shortStorageId); no? plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23175> Why not just use QDate::currentDate here, and remove the member variable? If this was due to performance, a local variable is enough, IMHO, rather than polluting the class with an extra member var. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23179> If you don't use the resulting index, then contains() is faster than indexOf(). plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23180> This is really an unexpected data structure. It took me some time to figure out that it's a list where odd and even items don't have the same meaning: (storageId1, date1, storageId2, date2, ...) This requires some reverse-engineering skills to understand. I would have suggested the obvious QHash<QString, QDate>, but I see why you did this: in order to save the list into KConfig. Well, I would have done the conversion from/to QStringList at the time of the readEntry/writeEntry, but I would have used the QHash for everything else, it makes algorithms much easier to write. It makes things a bit faster too, since this code (create...) is going to be called every time a desktop file is added/modified, while the KConfig code is only called once on startup and once every time a new desktop file is installed. (but not when desktop files are updated, e.g. make install with new translations). The indexOf() that I suggested (linear search in the large list of all desktop files) would become a QHash lookup. Major performance improvement. Maybe the storage to KConfig should be a separate group in fact, rather than one humongous value. It must be huge, no? Two strings per desktop file, all concatenated into one big string... I think I would use a group, and just write entries into it like desktopId=date. By iterating over the QHash, obviously. And to avoid bloating up plasmarc, I would even move this to a separate config file. If parsing the file takes too much time, we could even replace it with a binary cache one day. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23181> see above. plasma/desktop/applets/kickoff/core/applicationmodel.cpp <http://git.reviewboard.kde.org/r/109611/#comment23184> Space before and after operators (here and on many other lines). plasma/desktop/applets/kickoff/simpleapplet/simpleapplet.cpp <http://git.reviewboard.kde.org/r/109611/#comment23182> Please open your own patch in reviewboard to see trailing slashes, or configure your editor to remove them (on lines that you actually add/modify). At least QtCreator can do this, maybe kdevelop too. - David Faure On April 2, 2013, 6:07 p.m., Wolfgang Bauer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109611/ > ----------------------------------------------------------- > > (Updated April 2, 2013, 6:07 p.m.) > > > Review request for kde-workspace. > > > Description > ------- > > (This comes from a patch included in openSUSE) > > This patch makes kickoff remember all the .desktop files it sees (in > kickoffrc). > New ones are additionally shown in a seperate submenu named "Recently > Installed" (for 3 days). > > This can be toggled on and off in the plasmoid's settings. > > > This addresses bug 316916. > http://bugs.kde.org/show_bug.cgi?id=316916 > > > Diffs > ----- > > plasma/desktop/applets/kickoff/applet/applet.cpp a6f7379 > plasma/desktop/applets/kickoff/applet/kickoffConfig.ui 8664ac8 > plasma/desktop/applets/kickoff/core/applicationmodel.h f0f8872 > plasma/desktop/applets/kickoff/core/applicationmodel.cpp 57b6ba5 > plasma/desktop/applets/kickoff/simpleapplet/simpleapplet.cpp 28fba18 > plasma/desktop/applets/kickoff/ui/launcher.h 2a234c3 > plasma/desktop/applets/kickoff/ui/launcher.cpp 4425bcc > > Diff: http://git.reviewboard.kde.org/r/109611/diff/ > > > Testing > ------- > > Created a new .desktop file in ~/.local/share/applications, ran kbuildsycoca4 > and the menu "Recently Installed" appeared with this entry. > After logout/login this is still present. > Deleted the .desktop file again, ran kbuildsycoca4 and the menu "Recently > Installed" disappeared again. > > I have been using the (vanilla KDE) kickoff applet with this patch for about > a week now. > > Also this patch is already part of openSUSE for several years... > > > File Attachments > ---------------- > > Settings dialog (kickoff style) with patch > > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/02/kickoff-settings.png > Plasmoid (kickoff style) showing the "Recently Installed" submenu > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/02/kickoff.png > Settings dialog (classic style) with patch > > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/02/classic-settings.png > Plasmoid (classic style) showing the "Recently Installed" submenu > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/02/classic.png > > > Thanks, > > Wolfgang Bauer > >
