Review Request: kickoff: save recent applications list on every change to it

[Andriy Gapon]

> On May 31, 2012, 10:09 a.m., Aaron J. Seigo wrote:
> > plasma/desktop/applets/kickoff/core/recentapplications.cpp, line 42
> > <http://git.reviewboard.kde.org/r/105112/diff/1/?file=65943#file65943line42>
> >
> >     holding on to groups like this is a bit dangerous; you need to be able to know that the kconfig object behind it is always valid at all times. better to just create the group when needed.
> >     
> >     a method that returns such a group on demand lets the code be shared, avoiding duplication of magic strings. e.g.:
> >     
> >     KConfigGroup config()
> >     {
> >         return componentData().config()->group("RecentlyUsed");
> >     }

I've been using the original version of the patch for several weeks and haven't run into any problems.  In theory, either holding a group should be safe or not, there can't be any middle ground. If it is unsafe, then a group may become invalid even between group("xxx") is called and its return result is "immediately" used. The _probability_ of the problem could be reduced, but correctness won't be increased nevertheless.
In any case, it was trivial to comply with this request, so I just did that.  I didn't create a separate config() method as was suggested, because the group is obtained in only a single place, save() method.


> On May 31, 2012, 10:09 a.m., Aaron J. Seigo wrote:
> > plasma/desktop/applets/kickoff/core/recentapplications.cpp, line 77
> > <http://git.reviewboard.kde.org/r/105112/diff/1/?file=65943#file65943line77>
> >
> >     it pains me to see such calls in paths which could be called quite often. yes, in theory should only happen when apps launch, but there is no guarantee that this will remain that way or that such events will not happen in rapid bursts.

If you have better suggestions I am all ears :-)
Again, I haven't run into any issues with this patch myself...
Theoretically speaking, the bursts, if any, would be limited by human being capabilities anyway. And if the kickoff menu to be interacting with more than just the kickoff menu, then, I hope, this logic would get the major overhaul being discussed.  So this code (and the patch) would become irrelevant and the stuff would be done via zeitgeist (plus nepomuk, etc).


- Andriy


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105112/#review14293
-----------------------------------------------------------


On July 4, 2012, 7:57 a.m., Andriy Gapon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105112/
> -----------------------------------------------------------
> 
> (Updated July 4, 2012, 7:57 a.m.)
> 
> 
> Review request for Plasma and Trever Fischer.
> 
> 
> Description
> -------
> 
> Currently recent applications list in kickoff is saved only when kickoff gracefully exits.  This could be a minor annoyance when X/KDE/plasma crashes.  I think that saving the list on every update to it should be a good idea.  It should be a low overhead too, because the list changes only when a user launches an application via KDE.
> 
> 
> This addresses bug 206511.
>     http://bugs.kde.org/show_bug.cgi?id=206511
> 
> 
> Diffs
> -----
> 
>   plasma/desktop/applets/kickoff/core/recentapplications.cpp 3e05389 
> 
> Diff: http://git.reviewboard.kde.org/r/105112/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120704/2c87dfd0/attachment-0001.html>