Qt 4.6 behaves differently when "overloading" signals

[Dario Freddi]

On Sunday 08 November 2009 13:50:54 Olivier Goffart wrote:
> Le Saturday 07 November 2009, Thiago Macieira a écrit :
> > Em Sábado 7. Novembro 2009, às 19.35.44, Dario Freddi escreveu:
> > > On Saturday 07 November 2009 19:19:22 Thiago Macieira wrote:
> > > > Em Sábado 7. Novembro 2009, às 18.26.48, Dario Freddi escreveu:
> > > > > On Saturday 07 November 2009 18:19:44 you wrote:
> > > > > > That warning and behaviour is about 5 or six years old. I asked
> > > > > > thiago about it some weeks before. You only see it now because it
> > > > > > only appears in qt developer builds which we only started to use
> > > > > > when switching to the qt git repo.
> > > > > >
> > > > > > So i think the ... it worked with qt <4.6 may be unlikely. Are
> > > > > > you really sure?
> > > > >
> > > > > Sure, otherwise for example the k3bsetup and some other polkit-qt
> > > > >  applications wouldn't work as expected with qt 4.5.
> > > >
> > > > The warning is correct.
> > > >
> > > > Overriding signals is an extremely bad idea. Since signals aren't
> > > > virtual, the code in the base class that emits the signal will emit
> > > > the lower-ID signal. However, the connect() call searches from the
> > > > most derived class towards QObject -- that means you connect to the
> > > > most-derived class's signal.
> > > >
> > > > In other words, the signal is emitted, but it's never caught.
> > > >
> > > > DO NOT override signals.
> > >
> > > Could be good to have this information somewhere.
> >
> > That's what the warning is there for.
> >
> > > But being good or bad,
> > >  the problem is that some applications working with Qt 4.5 won't work
> > > with Qt 4.6 (even if I still would like to test using a non developer
> > > build, if somebody could point me in the right direction).
> >
> > If it's a behaviour regression, please report it. A testcase would do
> >  wonders to speed up its fixing.
> >
> > If it's really a behaviour regression in dealing with signal-slot
> >  connections, this needs to be fixed *now* before 4.6.0-RC1.
> >
> > > I can come up with a patch for partitionmanager and that wouldn't hurt
> > >  since it's an application, whereas polkit-qt is a library and quite a
> > > more complex matter, since I would have to add another (different)
> > > signal or work around it somehow, and to avoid having people rewrite
> > > parts of their application I'd surely favor for the second, which would
> > > anyway bring up problems.
> > >
> > > > Also, Michael is right: that warning is extremely old. It was added
> > > > for Qt 4.0.
> > >
> > > The problem is that is showing (at least for me) only using 4.6, but
> > > could be because of developer builds
> >
> > Could be too that the test that the warning depended on didn't work
> > before. Olivier optimised the area around the warning and maybe corrected
> > the check.
> >
> > He maybe introduced the regression too.
> >
> > Like I said, we need a testcase.

After the last pull on kde-qt my testcase works, so it was probably an old 
build's fault

> 
> The thing is that the warning was shown in Qt 4.5 if the signal conflict
>  with one signal in the class dirrectly under the hierarchy.
> 
> (It has been fixed in 919b7230.  It is a bit difficult to read but one can
>  see that in QMetaObject::indexOfSignal, before the patch, it will do
> m = m->d.superdata;  before exiting the while loop, and then again
> m->d.superdata->indexOfMethod(signal);
> But now the while loop is breaked before.)
> 
> So the warning may appears more often.
> 
> But the signal was already overriding the QAction::activated meaning that
> QAction::activated cannot be emitted anymore. which is quite bad.
> 

-- 
-------------------

Dario Freddi
KDE Developer
GPG Key Signature: 511A9A3B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20091108/a5ed70de/attachment.sig>