canvas.clear() with color argument causes a crash

[eort]

Hi,

While I was looking for examples how to raise warnings properly, I realized that the canvas.clear() function has a color argument, which, if used, causes a crash that is not caught in an exception. First, the variable cfg is not defined. Second, if it was and indeed contained u'color', warnings.warn causes a crash, because this module hasn't been imported yet at that point. I'm not sure, what exactly would be the desired behaviour. Once you tell me, I could try to also fix that.

    def clear(self, color=None):
        if color is not None:
            if u'color' in cfg:
                warnings.warn(u'color is a deprecated style argument for '
                    'canvas.clear(). Use background_color instead.',
                    DeprecationWarning)
            self.background_color = color
        self.stim_list = []
        self.prepare()

Best,

Eduard

[smathot]

Thanks for spotting this. It's a tricky one, because it requires understanding how decorators work in general, and what the @configurable decorator does in this case:

• The @configurable decorator takes all keyword arguments that are specified as configurables in the constructor, in this case openexp.canvas._canvas.pyscho.__init__(). So those are things like penwidth and background_color.
• Those style keywords are then used to change the style of the canvas. So, for example, the background_color might be set to 'blue'. However, the orignal style is remembered.
• Then the decorator calls the function that it decorates (clear() in this case), and passes all arguments and keywords (minus the style keywords that it has processed) to the function.
• When the function is finished, the decorator restores the canvas style to the way it was before.

Does that make sense? It's not super difficult, but it requires a certain familiarity with decorator logic.

You could say that, because of @configurable, this:

c.fixdot(color='red')

Is a shortcut for:

old_color = c.color
c.color = 'red'
c.fixdot()
c.color= old_color

Get it?

You say that:

Second, if it was and indeed contained u'color', warnings.warn causes a crash, because this module hasn't been imported yet at that point.

But it actually doesn't, does it? The reason is that the color keyword will be processed by @configurable and stripped off before clear() is called; so, color is always None for clear().

I'm not sure what the best way to handle this is. For now, this check (if color is not None) and the color keyword should be removed from clear() in all canvas backends. Even if it doesn't crash, it's nonsensical and confusing. So please go ahead and do that.

Then we're faced with the following question:

• Are we going to accept backwards incompatibility, in the sense that clear(color='red') now no longer changes the background color, and you have to use clear(background_color='red') instead) Or;
• Is this important enough to think of some hack to preserve backwards compatibility?

What do you think?

[eort]

Ah, great. Thanks to you and that essay, I finally get what decorators are about. Very helpful.

But it actually doesn't, does it? The reason is that the color keyword will be processed by @configurable and stripped off before clear() is called; so, color is always None for clear().

Alright, this makes perfect sense. However, what would the warning add, if it's never executed anyway?

What do you think?

I would accept backwards incompatibility. The only reason why I ever used the clear() function, is to wipe respective canvas and bring it back to the default state. In my opinion, wiping it and changing some parameters of it in one go, is not what this function is about (even though it's quite a neat feature). Besides, I believe that the error message that would accompany such a crash can easily made informative enough so that the average user shouldn't have any problems fixing the code instantly. So basically, as I don't expect many users to be affected by the change, and the issue being easily fixable, I'd opt for accepting backwards incompatibility.

Even if it doesn't crash, it's nonsensical and confusing. So please go ahead and do that.

Will do!

Eduard

[smathot]

However, what would the warning add, if it's never executed anyway?

Absolutely nothing. It's dummy code, which is why needs to go!

Besides, I believe that the error message that would accompany such a crash can easily made informative enough so that the average user shouldn't have any problems fixing the code instantly.

So passing a color keyword to clear() actually won't give an error message, because it is a valid keyword, and it will be processed by configurable (I removed the @ because there is actually a GitHub user named configurable that we keep sending mentions to! :wink: ). It just doesn't do anything, because it specifies the foreground rather than the background color.

Will do!

:ok: