[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: PATCH: Re: kdelibs/kdeui
From:       Ellis Whitehead <kde () ellisw ! net>
Date:       2002-01-30 16:53:55
[Download RAW message or body]

Simon,

If after you read this, you still feel like having unplugAll() in ~KAction() 
might be risky, I'll go ahead and revert the commit.

> On Wednesday 30 January 2002 03:48, Simon Hausmann wrote:
> > On Tue, Jan 29, 2002 at 09:38:27PM -0500, Ellis Whitehead wrote:
> > > This patch has KAction::~KAction() call unplugAll() to remove itself
> > > from any menu or toolbar it's in.
> >
> > I remember we had trouble with that, and Waldo also remembered that
> > we had this in some 2.x version and had a lot of bug reports about
> > this one. 

According to the records on kde-core-devel and bugs.kde.org, the problem was 
in krootwm.cc and not in KAction::~KAction() or KAction::unplugAll().  The 
bookmark list was getting deleted twice, and unplugAll() is merely where the 
error showed up.

> > In addtion it doesn't work quite the way one would expect
> > as calling that virtual method will only do half of the job by
> > calling only KAction's own unplugAll(). 

Good that you pointed out the issue with calling virtual unplug() from the 
KAction destructor.  However, I don't think that it's a "real" problem.  
Judging by a quick look at all the virtual unplug() methods in kaction.cpp, 
they are basically just cut-and-paste without:
      if ( m_parentCollection )
        m_parentCollection->disconnectHighlight( {bar,menu}, this );
Seems like they can be removed anyhow.  I'll have to look at it more closely, 
but if we add a check for QMenuBar in KAction::unplug(), then maybe all of 
them could be removed.  If there are cases where that's not true, then 
unplugAll() will need to be called from the destructor of that class.

> > (see previous threads on
> > kde-core-devel about this problem)

There was a discussion in early November (while I was at the ALS conference 
and not keeping up on the list), but in the end the only reason stated 
against calling unplugAll() from ~KAction() was about not needing to make 
those calls during shut-down. 
http://lists.kde.org/?l=kde-core-devel&m=95846925315076&w=2

The safety and convenience is worth it, though, so that actions from a dynamic 
widget can be used by a different persistent widget.  Without the unplugAll() 
call in ~KAction(), attempts to handle this are very hacky.

Regards,
Ellis

[prev in list] [next in list] [prev in thread] [next in thread]