Classic KMenu Extension

[Sebastian Sauer]

Cyberbeat wrote:
> Hi,

Hi too :)
 
> I made modifications to the classic-kmenu-applet:
> 
> I send a svn-diff for an overview of the changes to existing files. And an
> archive with all modified/new files. I will be happy if I get
> feedback/suggestions. This is my first qt/kde-programming-experience :)

I would say, that it looks pretty good and even more great if it's your first 
qt/kde-programming-experience! Thanks for that. So, some comments;

+    QLabel *showRecentLabel = new QLabel(i18n("Recent applications:"), p);
+    l->addWidget(showRecentLabel, 2, 0);
+    d->showRecentCheckBox = new QCheckBox(i18n("Show on top of standard 
menu"),p);
+    d->showRecentCheckBox->setChecked(d->showRecentApplications);
+    showRecentLabel->setBuddy(d->showRecentCheckBox);
+    l->addWidget(d->showRecentCheckBox, 2, 1);
+
+    QLabel *recentLabel = new QLabel(i18n("Recent applications:"), p);
+    l->addWidget(recentLabel, 3, 0);
+    d->recentApplicationsSpinBox = new QSpinBox(p);
+    d->recentApplicationsSpinBox->setMaximum(10);
+    d->recentApplicationsSpinBox->setMinimum(0);
+    
d->recentApplicationsSpinBox->setValue(Kickoff::RecentApplications::self()->maximum());
+    recentLabel->setBuddy(d->recentApplicationsSpinBox);
+    l->addWidget(d->recentApplicationsSpinBox, 3, 1);

Two times a "Recent applications:" label? I would suggest to remove the first 
label and change the text of the checkbox to something like "Show recent 
applications" or so. Maybe the second label should then say "Number of recent 
application items" (my english sucks but the point here is, to indicate that 
the spin changes the number of items what is useful to know if e.g. a 
screenreader does read that label).

Do you think it would be a good idea to introduce logic to disable or even 
hide those both options if some other view like e.g. "logout" was choosen 
where those options don't make sense?

-    <signal name="cleanRecentDocumentsAndDocuments"/>
+    <signal name="clearRecentDocumentsAndApplications"/>

That's a bit of a problem cause it changes the dbus-API. That would mean, that 
e.g. scripts or applets written against <4.3 connecting with that signal 
won't work any longer with 4.3 what is evil (specially since it's a 
runtime-error probably hard to discover). But then I guess it's safe to 
assume that noone outside of kickoff uses that signal yet. Anyway, I would 
suggest to backport that fix to the yet unreleased 4.2 branch before 4.2 is 
out and others may start to use that signal.

-    Q_ASSERT(maximum > 0);
+    Q_ASSERT(maximum >= 0);

uh? Does it really make sense to allow to set 0 recent items? I would suggest 
to use d->recentApplicationsSpinBox->setMinimum(1);

+       while (privateSelf->serviceQueue.count() > privateSelf->maxServices) {
+               QString removeId = privateSelf->serviceQueue.takeFirst();
+               kDebug() << "More than max services added.  Removing" << 
removeId << "from queue.";
+               privateSelf->serviceInfo.remove(removeId);
+               emit 
applicationRemoved(KService::serviceByStorageId(removeId));
+       }

Sorry, probably a stupid question, but does that also affect the recentmodel 
used in kickoff (so, not the classic menu)? If yes, then that's a problem 
cause that option can only be changed in the classic menu but is used at both 
of them. If not, then forget this comment :)