On Thursday 02 June 2005 09:39, Fred Schaettgen wrote:
> > hm.. looking at the kmenu patch i really wonder if the DCOP signal
> > shouldn't be attached directly to the RecentlyLaunchedApps (RLA) class.
>
> ...
>
> I guess it's fine if the signal is attached to the kmenu and RLA stays a
> rather generic statistics class. We might want to ignore some launch
> signals at a later time (don't know what.. maybe if we also signal recent
> document urls or something), and the decision to ignore the signal should
> not be done by the RLA class then.

sure. and this is accomplished by simply not emitting the signal in the first 
place =)

> > kmenu (which is a PanelServiceMenu (PSM) subclass) item selected ->
> > PSM::exec emits the dcop signal -> kmenu gets the signal calls ->
> > PSM::updateRecentMenuItems -> calls a bunch of RLA methods
> >
> > in fact, it seems that PSM::updateRecentMenuItems(service) gets called
> > twice, once by KMenu and once by PSM itself.
>
> You're right.. now I remember what the sender argument in the signal was
> good for ;) In the new patch it ignores messages sent by itself. We could
> also use just the DCOP signal, but somehow I don't like the idea of an
> application relying on dcop to communicate with its own modules.

if this was all moved into the RLA class, it wouldn't be an issue =) in fact, 
if we move RLA into libkicker, it becomes rather trivial to access it and we 
can use regular signals throughout. RLA should become a singleton class 
obviating the need for a static member in PSM and making it simple to connect 
to from applets and what not. thoughts?

> > next time kmenu is opened -> KMenu::initialize() is called -> calls
> > PSM::updateRecent() -> PSM messes around with the entries in the KMenu.
> > there is no reason for this to be in PSM!
>
> Yeah, I wondered myself why the RLA-related code was put in the base class
> (PSM) and not in the kmenu class. Maybe the author wanted to introduce
> recently used lists not just in the toplevel kmenu?

perhaps, but i don't think that makes much sense. nor does it reflect the 
current reality. =)

> > so it seems we have some very snaky code here. this problem obviously
> > predates your patches. instead, i'd like to see this happen:
> >
> > dcop signal is emitted (from minicli or PSM) ->
> > RLA::applicationLaunched() called -> if it results in a change, emit
> > RLA::recentAppsChanged() -> KMenu::updateRecentApplications() (moved from
> > PSM) is called
> >
> > much nicer, no? what do you think?
>
> Certainly, if the RLA feature isn't supposed to be used in other PSM-based
> classes.

even we keep updateRecentApplications in PSM, the rest shoudl be moved into 
RLA so any class can get at it easily. 
PanelServiceMenu::updateRecentMenuItems(KService::Ptr & service) really 
belongs in RLA, IMHO, with RLA emitting signals to trigger updates. the dcop 
signal likewise belongs in RLA. this will allow us to use RLA wherever we 
wish, and not just in the service menus.

and seeing as only the K menu uses updateRecentApplications, i would like to 
see it moved into k_mnu.cpp. i find that methods in base classes which are 
only used by one specific subclass to be more confusing than necessary. when 
coding, it means you have to have one more window open to see the code paths 
and it's not immediately clear that there are no other classes that use that 
code.

and now my usual annoying style/formatting sniggles:

 - don't use NULL, use 0
 - try and keep to the kicker style guide in new code, e.g. use braces even if 
the if() block has only one statement. i know that's not how the rest of this 
applet is formatted, but if we correct as we go it slowly falls into place
 - i find code like this:
	QString url = *iter++;
   unecessarily ambiguous. it's not any faster than:
	QString url = *iter;
	++iter;
  and the latter is far more obvious since the reader doesn't have to consider 
the order of operations due to operator precedence.

-- 
Aaron J. Seigo
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20050611/8fc91b2c/attachment.pgp