Review Request 109611: Add option to show "Recently Installed" apps in kickoff plasmoid

David Faure faure at kde.org
Tue Apr 16 15:45:02 BST 2013 

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130416/7e9d0ee8/attachment.htm>

More information about the kde-core-devel mailing list