GNOME Bugzilla – Bug 345345
PrintOperation::paginate is not emitted for class handler
Last modified: 2015-08-18 13:17:45 UTC
PrintOperation::paginate is emitted if a callback is attached, but it doesn't check the class method. @@ -2042,8 +2042,9 @@ print_pages_idle (gpointer user_data) goto out; } - - if (g_signal_has_handler_pending (data->op, signals[PAGINATE], 0, FALSE)) + + if (GTK_PRINT_OPERATION_GET_CLASS (data->op)->paginate || + g_signal_has_handler_pending (data->op, signals[PAGINATE], 0, FALSE)) { gboolean paginated = FALSE;
Actually, why is the check needed? Just initialize paginated with TRUE, and emit signal. If there is a handler, it will set the return value; if no handler, the return value is unchaged and everything is fine.
that might work; I believe the has_handler_pending check is a leftover from when I thought there would be a default handler.
Is it legal to subclass GtkPrintOperation instead of creating GtkPrintOperation instance and connect to its signals? If yes, can this be fixed?
(In reply to comment #1) > Actually, why is the check needed? Just initialize paginated with TRUE, and > emit signal. If there is a handler, it will set the return value; if no > handler, the return value is unchaged and everything is fine. > Check is needed, boolean return value is initialized to FALSE by g_signal_emit.
Created attachment 85945 [details] [review] patch Ping?
Created attachment 85946 [details] [review] patch Or this, whatever is nicer.
2007-04-29 Matthias Clasen <mclasen@redhat.com> * gtk/gtkprintoperation.c (print_pages_idle): Also check the default handler when deciding whether to emit the paginate signal. (#345345, Yevgen Muntyan)
This change broke printing in gtkmm and has been reverted (see bug #482089). I think we need a test case for this problem to fix it properly.
By the way, to check that gtkmm printing was not broken again, just try the gtkmm/book/printing/simple example. With this change, printing or print preview did nothing.
gtkmm is a test case by itself, as you could see. The callback must return TRUE when it's done paginating. If it returns FALSE (which gtkmm's default handler does, if I understand it right), then you get paginating which never ends.
If the default should be TRUE, then I suspect that the correct fix (without requiring gtkmm to know what is a good value for a particular default signal handler) is just to implement a default signal handler in GTK+ which returns TRUE. gtkmm will just call it to get the return value. That keeps the default value in one place, without language bindings having to know it and duplicate it. But without a test case I couldn't know whether this solves your problem.
gtk code is very very weird: there should be no default and no emission at all. Anyway, if gtk implements a default handler which returns TRUE, then applications which connect callbacks will break. Then, gtkmm overwrites the gtk handler anyway, so it will return FALSE, so you get same thing again. This whole thing is pretty weird, absolutely.
> Anyway, if gtk implements a default handler which returns TRUE, then > applications which connect callbacks will break. Is that because TRUE means "don't call any other signal handlers" in this case? > then, gtkmm overwrites the gtkhandler anyway, so it will return FALSE, so you > get same thing again. gtkmm doesn't override the gtk handler completely - it calls the GTK+ default handler, so there will be no change of behaviour. It exists only so that it can be overriden by derived C++ classes. Anyway, I am still completely unable to discuss any of these changes without knowing what problem they would be meant to fix. Is it, for instance, that something is not working when you implement the paginate default signal handler in a derived GtkPrintOperation instead of connecting a callback?
(In reply to comment #13) > > Anyway, if gtk implements a default handler which returns TRUE, then > > applications which connect callbacks will break. > > Is that because TRUE means "don't call any other signal handlers" in this case? Yes. But, the accumulator could probably be removed with no harm to applications. One could make up weird code which uses the current behaviour, in fact one could connect do-nothing callback to be able to use the virtual method (I have had this idea, to use such a workaround for this bug, since I hoped the bug would be fixed, but thanks god I am simply not using paginate signal at all instead). So harm is possible, but it's unlikely. > > then, gtkmm overwrites the gtkhandler anyway, so it will return FALSE, so you > get same thing again. > > gtkmm doesn't override the gtk handler completely - it calls the GTK+ default > handler, so there will be no change of behaviour. It exists only so that it can > be overriden by derived C++ classes. That's good. (Now that's real help!) > Anyway, I am still completely unable to discuss any of these changes without > knowing what problem they would be meant to fix. Is it, for instance, that > something is not working when you implement the paginate default signal handler > in a derived GtkPrintOperation instead of connecting a callback? Exactly. The "something not working" means paginate signal is not emitted (and naturally the virtual method isn't called) unless you connect a callback (exactly this weird way: if you do connect a callback, then the class handler will be called, as a part of usual signal emission). I believe you'll see this bug if you try to use paginate signal in your example gtkmm code (simply make it print something and return TRUE, no work is required). So, removing the accumulator and implement the default handler sounds good.
> > Anyway, if gtk implements a default handler which returns TRUE, then > > applications which connect callbacks will break. > > Is that because TRUE means "don't call any other signal handlers" in this case? > > Yes. But I believe that signals are connected as "before the default signal handler" normally by GTK+ applications. So the default signal handler will then never run, so it can't have an effect. It would require that gtkmm signals be explicitly connected "before" - they connect "after" in gtkmm by default. But we are quite used to having to do that sometimes with these gboolean-returning signals - it's one reason why class derivation is more obvious.
All the signal handlers are executed, no matter who's connected when, that's the problem here. The paginate handler is supposed to return FALSE to indicate that the signal should be emitted again, i.e. that there is more work to do. So if there is an application callback and the default handler, then both will be called (since paginate will need to be emitted more than once). So the fix (which wouldn't break anything and would enable the virtual method at the same time) would be: implement default handler which returns TRUE; make the signal RUN_FIRST; remove the accumulator. This way, if someone implements virtual method, then the method will be used; if someone connects a callback, then the callback will run after the default handler, so the signal return value will be correct (the default handler return value will be ignored). If someone does both (or connects two callbacks), then its his own problems. Another fix would be to remove the virtual method completely, which would suck. How does it sound?
Here is an alternative patch, that uses g_signal_emitv(). Does that work for you ?
Created attachment 97306 [details] [review] new patch
Created attachment 97313 [details] C test case This patch fixes the issue, as the original one, since it does the same thing as the original one. And it breaks gtkmm too.
Created attachment 97314 [details] gtkmm test case Use #define USE_PAGINATE to see how the virtual method works, and #undef it to see how it breaks with the default gtkmm's virtual method which returns false. Thanks a lot to Murray for the test case ;)
Created attachment 97566 [details] [review] patch
Thanks. I can confirm that the gtkmm test case from #20 does not print (or print preview), presumably because its on_paginate() (the default signal handler) is not being called. I am sure that a C test case is possible and would have been clearer. I have tested the patch from #21 and it fixes this test case. In fact, on_paginate seems to be called twice. I don't know if that's normal. Do you expect this patch to introduce any other problems? I am concerned about this change, which seems to be a change of API behaviour: * Emitted after the #GtkPrintOperation::begin-print signal, but before * the actual rendering starts. It keeps getting emitted until it - * returns %FALSE. + * returns %TRUE. Also, the GTK+ patch really needs a ChangeLog entry explaining what it does and why
(In reply to comment #22) > I am sure that a C test case is possible and > would have been clearer. Comment #19. > I have tested the patch from #21 and it fixes this test case. In fact, > on_paginate seems to be called twice. I don't know if that's normal. Yes, that's right, the test case simulates real printing where paginating is supposed to be done in chunks. Two passes in this case. > Do you expect this patch to introduce any other problems? I am concerned about > this change, which seems to be a change of API behaviour: > * Emitted after the #GtkPrintOperation::begin-print signal, but before > * the actual rendering starts. It keeps getting emitted until it > - * returns %FALSE. > + * returns %TRUE. No, it's docs correction. The behavior does not change.
Thanks. So, this seems quite simple and fixes the problem for gtkmm. I hope that this can be approved by the GTK+ maintainers so it can go in a 2.12.x bug-fix release.
Could someone please review this patch? This is affecting applications.
Review of attachment 97566 [details] [review]: ::: gtk/gtkprintoperation.c @@ +2102,3 @@ + { + gboolean paginated = FALSE; These whitespace changes probably shouldn't be here.
Can we push this, without the whitespace changes?
Created attachment 309399 [details] [review] patch I have re-run into this problem today. Muntyan's patch still largely applies and fixes the issue. I tested both with a GtkPrintOperation subclass and with Gedit (which uses GtkPrintOperation as is) and it seems to work as expected. However I have not tested gtkmm since at the moment did not build for me. Attached is a version of the patch rebased to current master and with a changelog and explanation.
(In reply to Paolo Borelli from comment #29) > However I have > not tested gtkmm since at the moment did not build for me. The patch was fine with gtkmm last time I tried, in comment #24, so I wouldn't worry about that. If there's a real problem with the gtkmm build, please let me know, via email if you like.
(In reply to Murray Cumming from comment #30) > The patch was fine with gtkmm last time I tried, in comment #24, so I > wouldn't worry about that. > Ok, so I went ahead and pushed it > If there's a real problem with the gtkmm build, please let me know, via > email if you like. It is more of a problem with jhbuild... it decides that cairomm should not be rebuilt since it is from a tarball, but then pangomm tells me cairomm is too old. It could also be that I need to update jhbuild.