Review Request 109611: Add option to show "Recently Installed" apps in kickoff plasmoid
Wolfgang Bauer
wbauer at tmo.at
Sun May 19 22:40:39 BST 2013
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 640
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line640>
> >
> > Can it really be null? Did you see this elsewhere, or did you add it "just in case"?
> > It seems unnecessary to me.
This was there "just in case".
On second thought, e could only be null when the list is empty, I think. But then this line wouldn't be reached anyway...
So I have removed this check.
I also removed the check for list.isEmpty() a few lines before that, because if the list is empty list.begin() equals list.end() and the for loop won't be entered. So the check is unnecessary.
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 649
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line649>
> >
> > 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.
.desktop is removed because it is redundant anyway.
This shortens the strings to compare (so it's a little bit faster) and saves space in the config file.
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 610
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line610>
> >
> > What is "-" for?
"-" means "don't show this".
It's set for entries that are older than 3 days (and for all entries on first run), so they won't get shown anymore.
This is an optimization, for older entries (which are most of the entries) only 1 char has to be saved and the date doesn't have to be compared due to this.
Since seenPrograms is a QHash<QString, QDate> now, I am using an empty QDate() for this instead.
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 644
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line644>
> >
> > |= 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?)
Well, its meaning is the variable seenPrograms has changed, so it's called "seenProgramsChanged"
I could change that of course if you think this would be better.
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 663
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line663>
> >
> > 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.
The member variable was a QString, so it couldn't just be replaced with QDate::currentDate().
But I have removed d->currentDate, since the QDate is stored now in seenPrograms instead of a QString.
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 654
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line654>
> >
> > This whole for() loop could be replaced with something like
> > index = d->seenPrograms.indexOf(shortStorageId);
> > no?
Because seenPrograms is a QHash<QString, QDate> now, the for loop has been replaced by "it_find = d->seenPrograms.find(shortStorageId)"
> On April 16, 2013, 4:45 p.m., David Faure wrote:
> > plasma/desktop/applets/kickoff/core/applicationmodel.cpp, line 669
> > <http://git.reviewboard.kde.org/r/109611/diff/1/?file=120582#file120582line669>
> >
> > 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.
> >
Thanks for the suggestion!
I have changed this, seenPrograms is a QHash<QString, QDate> now.
Regarding the config file:
The list is not stored in plasmarc, but in kickoffrc.
Is it huge? Well, it depends how you define "huge"... the size of kickoffrc is 7.3 KiB on my system. This is much much smaller than the StringLists stored by the akonadi_pop3_resource, f.e. (2 lists, seenUidList and seenUidTimeList, each in one very long line resulting in 712 KiB on my system)
But I have changed this now to write desktopId=date entries as you suggested. They are stored in the group "[Seen Applications]".
- Wolfgang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109611/#review31163
-----------------------------------------------------------
On May 19, 2013, 11:39 p.m., Wolfgang Bauer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109611/
> -----------------------------------------------------------
>
> (Updated May 19, 2013, 11:39 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).
>
> The seen entries are saved as "id=date" pairs in the "Seen Applications" group.
> Entries older than 3 days get their date set to empty, which means "don't show this entry anymore".
> Also if there have no entries been saved yet, the date of all found entries is set to empty as well. This prevents that all menu entries are shown in the "Recently Installed" submenu on first start.
>
> The "Recently Installed" submenu 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
> example kickoffrc with this patch
> http://git.reviewboard.kde.org/media/uploaded/files/2013/04/18/kickoffrc
> sample kickoffrc written by this patch
> http://git.reviewboard.kde.org/media/uploaded/files/2013/05/19/kickoffrc
>
>
> Thanks,
>
> Wolfgang Bauer
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130519/9f400018/attachment.htm>
More information about the kde-core-devel
mailing list