Skip Menu |

This queue is for tickets about the Tkx CPAN distribution.

Report information
The Basics
Id: 42454
Status: resolved
Priority: 0/
Queue: Tkx

People
Owner: Nobody in particular
Requestors: MJCARMAN [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 1.04
Fixed in: (no value)



Subject: Megawidget method delegation ignores overrides
Tkx has some problems when delegating method calls from one megawidget to another megawidget. I'm trying to create a Tkx equivalent for Perl/Tk's Scrolled megawidget. The basic design is a frame-based megawidget that embeds a widget of a user-defined type. I want to delegate all method calls to that subwidget so I override the _mpath() method to do so. For a built-in text widget everything appears to work properly. e.g. I can call the insert() method on my megawidget and the text is added to the subwidget. When I embed a Tkx::ROText widget the text is *not* added. It appears that Tkx is calling the Tcl insert method (which has been stubbed to do nothing) instead of the Perl method. The key bit of code appears to be line 165 of Tkx.pm: return scalar(Tkx::i::call($self->_mpath($i[0]), @i, @_)); This is as close as I get to my insert method. I never make it to the overridden definition in Tkx::ROText. I need to call the method in $i[0] on the Perl widget named $self->_mpath but there's no reference, just a name. Is there any way to get the object reference from the name? I've noticed that Tkx has no equivilent of Perl/Tk's Advertise() and Subwidget() methods. In my particular case the problem is partially due to the tricks I play to make the widget readonly (renaming the Tcl widget and stubbing the insert and delete methods for the original name) but the core problem remains. For example, if I add a "blurb" method to Tkx::ROText I can't call it via Scrolled: bad option "blurb": must be bbox, cget, compare... Here's a fairly small example that demonstrates the problem; hopefully RT doesn't mangle it too badly... use strict; use warnings; package Tkx::Scrolled; use Tkx; use base qw(Tkx::widget Tkx::MegaConfig); __PACKAGE__->_Mega('tkx_Scrolled'); sub _Populate { my $class = shift; my $widget = shift; my $path = shift; my $type = shift; my %opt = @_; my $self = $class->new($path)->_parent->new_frame(-name => $path); $self->_class($class); my $new_thing = "new_$type"; my $w = $self->$new_thing(-name => 'scrolled', %opt); $w->g_pack(); return $self; } sub _mpath { $_[0] . '.scrolled' } package main; use Tkx; use Tkx::ROText; my $mw = Tkx::widget->new('.'); my $text = $mw->new_tkx_Scrolled('tkx_ROText'); # 'text' works fine $text->g_pack(); $text->insert('end', 'Fee Fie Fo Fum'); Tkx::MainLoop();
I've now applied http://github.com/gisle/tkx/commit/b0a3f1bdcc9b51de8bd7608386e56cfd21f40962 as fix for this. The crux of the patch is this: --- a/Tkx.pm +++ b/Tkx.pm @@ -162,7 +162,9 @@ sub AUTOLOAD { if ($prefix eq "m_") { my @i = Tkx::i::expand_name(substr($method, 2)); - return scalar(Tkx::i::call($self->_mpath($i[0]), @i, @_)); + my $p = $self->_mpath($i[0]); + return scalar(Tkx::i::call($p, @i, @_)) if $p eq $$self || !$class{$p}; + return (bless \$p, $class{$p})->$method(@_); } elsif (index($prefix, "_") != -1) { require Carp; On problem here is that if the sub-mega-widget implement an 'insert' method we will not find it because the $w->insert() call is first rewritten into $w->m_insert() before it is forwarded. We could just declare that mega widgets should use the 'm_' prefix on all its methods. Does that feel right? Alternative is to add this enhancement to the patch: --- a/Tkx.pm +++ b/Tkx.pm @@ -171,6 +172,9 @@ sub AUTOLOAD { Carp::croak("method '$method' reserved by Tkx"); } + my $p = $self->_mpath($method); + return (bless \$p, $class{$p})->$method(@_) if $p ne $self && $class{$p}; + $method = "m_$method"; return $self->$method(@_); } which make sure to forward method to perl megawidgets unchanged. I'm not totally happy with this because now we end up calling _mpath() twice for most $w->foo calls that don't use the m_ prefix.
Show quoted text
> We could just declare that mega widgets should use the > 'm_' prefix on all its methods. Does that feel right?
It wasn't clear to me whether you meant requiring module authors to name their methods 'm_foo' or requiring users to call them that way. After trying it both ways it appears that you meant the former. (That's good -- I would have strongly protested the latter.) While it doesn't quite feel right to me it's palatable. The extra effort is trivial. My concern would be ensuring that developers know that they should do it. I think it's really a question of what you want the API for building megawidgets to be. As far as I know I'm the only person who has developed any (at least for CPAN) so changing it now isn't a big deal. Whatever you choose, you'll be stuck with it eventually, so choose carefully. :D From an API perspective I prefer the additional change but I can understand your desire to keep AUTOLOAD as lightweight as possible. Would it be feasible to use AUTOLOAD to install a handler instead? That way all the name and path parsing would only happen the first time you called a method.
On Fri Jan 16 10:26:26 2009, MJCARMAN wrote: Show quoted text
> > We could just declare that mega widgets should use the > > 'm_' prefix on all its methods. Does that feel right?
> > It wasn't clear to me whether you meant requiring module authors to name > their methods 'm_foo' or requiring users to call them that way. After > trying it both ways it appears that you meant the former. (That's good
That's right. Show quoted text
> -- I would have strongly protested the latter.) While it doesn't quite > feel right to me it's palatable. The extra effort is trivial. My concern > would be ensuring that developers know that they should do it.
That's what documentation is for :) Show quoted text
> I think it's really a question of what you want the API for building > megawidgets to be. As far as I know I'm the only person who has > developed any (at least for CPAN) so changing it now isn't a big deal.
That's probably right. For ActiveState's applications we just ended up doing our mega widgets in Tcl, mostly because we "out-sourced" their development to our Tcl guy. Show quoted text
> Whatever you choose, you'll be stuck with it eventually, so choose > carefully. :D
If I apply this extra patch we'll be stuck with it. If I don't it should not be a problem to apply it later as this is really just an extension to the API. Show quoted text
> From an API perspective I prefer the additional change but I can > understand your desire to keep AUTOLOAD as lightweight as possible.
There is actually another reason now that I think about it. Forcing the widget implementors to use the m_ prefix make sure that widgets behave the same whether they are implemented in Tcl or Perl. Otherwise only Tcl widgets could be called both with the m_ prefix and without, while Perl widgets would always have to be called without. Not that this is a big deal. Show quoted text
> Would it be feasible to use AUTOLOAD to install a handler instead? That > way all the name and path parsing would only happen the first time you > called a method.
That's possibly a good idea regardless. I think I should try to spend some time in Devel::NYTProf first to figure out if the time spent in the AUTOLOAD function is significant and only try this if it is. BTW, as you probably noticed Tkx is now maintained in git with github as the central repository. If you are interested in commit access to the repo I'm happy to enable it for you. You would need a github account first.
Show quoted text
>> My concern would be ensuring that developers know that they >> should do it.
> > That's what documentation is for :)
Well, yeah, if they actually read it. :P I'm visualizing a scenario where if the author doesn't give their method names an 'm_' prefix the problem wouldn't be detected until someone tried to embed it and delegate to it. That's when I'd start getting reports about how Tkx::Scrolled breaks Tkx::Whatever. I could be making a mountain out of a molehill. I don't know how common it will be to nest megawidgets. It might be more than one would expect since there's no way to subclass the built-in widgets other than using the megawidget API. e.g. Tkx::ROText should really be a subclass of the text widget, not a megawidget. Show quoted text
> Forcing the widget implementors to use the m_ prefix make sure > that widgets behave the same whether they are implemented in > Tcl or Perl.
That's a good point. I find it rather compelling, but I'm curious -- is it possible to change the implementation between Perl and Tcl without affecting the way it's used? Wouldn't the user at least have to change "use Tkx::Foo" to "Tkx::package_require('widget::foo')"? Show quoted text
> I think I should try to spend some time in Devel::NYTProf first > to figure out if the time spent in the AUTOLOAD function is > significant and only try this if it is.
It probably is. Tkx calls AUTOLOAD for every method call and calls it twice when the 'm_' prefix isn't included (although that could be avoided fairly easily). That could add up quickly for things like $text->insert() although I'm not sure that the display can keep up anyway... <tangent>I wish I could get Devel::NYTProf to build on Windows</tangent> Show quoted text
> If you are interested in commit access to the repo I'm > happy to enable it for you.
I'm not sure I know enough to be trusted with a commit bit. :P I did join the tcltk mailing list. Is there a roadmap for Tkx? My work so far has mostly been trying to plug holes for those transitioning from Perl/Tk. I'd be happy to make that part of the core Tkx distribution instead of an smattering of separate pieces, but I have the impression that Tkx is intended to be minimalist.
On Fri Jan 16 13:39:20 2009, MJCARMAN wrote: Show quoted text
> >> My concern would be ensuring that developers know that they > >> should do it.
> > > > That's what documentation is for :)
> > Well, yeah, if they actually read it. :P > > I'm visualizing a scenario where if the author doesn't give their method > names an 'm_' prefix the problem wouldn't be detected until someone > tried to embed it and delegate to it. That's when I'd start getting > reports about how Tkx::Scrolled breaks Tkx::Whatever.
Seems like something that could happen. Don't know if we can do much about it besides documenting the issue. Having something like Tkx::Scrolled out there make it more likely that this is discovered. Show quoted text
> I could be making a mountain out of a molehill. I don't know how common > it will be to nest megawidgets. It might be more than one would expect > since there's no way to subclass the built-in widgets other than using > the megawidget API. e.g. Tkx::ROText should really be a subclass of the > text widget, not a megawidget.
Right, but some compromises must be made in a cross-language library like Tkx. Having to make it a megawidget just to tweak the behaviour of a Tcl widget seems acceptable to me. Show quoted text
> > Forcing the widget implementors to use the m_ prefix make sure > > that widgets behave the same whether they are implemented in > > Tcl or Perl.
> > That's a good point. I find it rather compelling, but I'm curious -- is > it possible to change the implementation between Perl and Tcl without > affecting the way it's used? Wouldn't the user at least have to change > "use Tkx::Foo" to "Tkx::package_require('widget::foo')"?
Sure, but that's a small change compared to walking though all the places you might call methods on this widget. It's not that likely that widget implementors would change the language, but having a single documented mapping for widget users seems valuable. Show quoted text
> > I think I should try to spend some time in Devel::NYTProf first > > to figure out if the time spent in the AUTOLOAD function is > > significant and only try this if it is.
> > It probably is. Tkx calls AUTOLOAD for every method call and calls it > twice when the 'm_' prefix isn't included (although that could be > avoided fairly easily). That could add up quickly for things like > $text->insert() although I'm not sure that the display can keep up anyway...
I tried today. The AUTOLOAD function wasn't alarmingly expensive in the test programs I had, but I still found that it would be a good idea to swap the order of dealing with "m_" and "g_" methods since the "m_" methods were many times more frequent. Show quoted text
> <tangent>I wish I could get Devel::NYTProf to build on Windows</tangent>
It's supposed to build fine on Windows since version 2.07. If it doesn't it might be a good idea to report it. Show quoted text
> > If you are interested in commit access to the repo I'm > > happy to enable it for you.
> > I'm not sure I know enough to be trusted with a commit bit. :P I did > join the tcltk mailing list. Is there a roadmap for Tkx? My work so far > has mostly been trying to plug holes for those transitioning from > Perl/Tk. I'd be happy to make that part of the core Tkx distribution > instead of an smattering of separate pieces, but I have the impression > that Tkx is intended to be minimalist.
There is no roadmap. I don't really have a need for extending Tkx much as it works fine for what what we do as it is and it already adopts automatically to Tcl/Tk as it improves over time. Having Tkx be small is a good thing, so I think it is better to keep most megawidgets outside, but if anything that support the construction of these should probably be part of the core. Having the commit access might make it easier for you to fix things in that area as you encounter them and for instance minor documentation nits you might find. Just sending me patches is fine as well. BTW, I uploaded Tkx-1.06 to CPAN today. It contains the fix for forwarding to megawidgets in perl.