search

eserte / perl-tk

the perl module Tk
https://metacpan.org/release/Tk
Other
44 stars 31 forks source link

Prevent eventGenerate with event.xany.window=None [test+fix] #1

Closed mcast closed 9 years ago

mcast commented 11 years ago

Hi Slaven,

While writing a test for some code built on pTk, I discovered that eventGenerate will attempt to operate with window=None, if called before Tk_MakeWindowExist.

The test shows this happening. The fix turns that into an error.

Thanks for your work + hth,

eserte commented 11 years ago

Thanks. Just one small plea: can you rewrite the test to not use Try::Tiny (it's not part of standard perl).

Regards, Slaven

mcast commented 11 years ago

On Thu, Apr 25, 2013 at 12:46:54PM -0700, Slaven Rezić wrote:

Thanks. Just one small plea: can you rewrite the test to not use Try::Tiny (it's not part of standard perl).

Done that, sorry for the inconvenience.

(It was habit. For our code we chose to replace all block evals with Try::Tiny)

Matthew

The Wellcome Trust Sanger Institute is operated by Genome Research Limited, a charity registered in England with number 1021457 and a company registered in England with number 2742969, whose registered office is 215 Euston Road, London, NW1 2BE.

eserte commented 11 years ago

Your fix is merged (with some changes).

Thanks, Slaven

eserte commented 10 years ago

Unfortunately I have to revert the change, because it breaks the optionmenu and menubutton handling on Windows (nothing happens if clicking on an optionmenu button, no menu appears, but the new error appears on the console). It seems that under Windows the virtual <> event does not set the "window" field.

mcast commented 10 years ago

On Fri, Nov 15, 2013 at 03:07:50AM -0800, Slaven Rezić wrote:

Unfortunately I have to revert the change, because it breaks the optionmenu and menubutton handling on Windows

Sorry about that.

Also I notice that my pull request acquired bcca4ce 0a86e2a after you first closed it. This is I think due to my misuse of master in a pull request.

I have recreated branch event-generate-no-xid for this, to match what you merged. I cannot [see how to] update the pull request to refer to the other branch.

(nothing happens if clicking on an optionmenu button, no menu appears, but the new error appears on the console). It seems that under Windows the virtual <> event does not set the "window" field.

Apart from the code being better (less surprising for people not expecting events to disappear), I have no great need for this to remain merged.

Would it be OK if the error was demoted to a warning? It's enough to avoid surprises. Maybe skip the warning if event is <> and $^O suggests Windows?

Or, is it safe for HandleEventGenerate to ensure the xid exists before proceeding?

Matthew

The Wellcome Trust Sanger Institute is operated by Genome Research Limited, a charity registered in England with number 1021457 and a company registered in England with number 2742969, whose registered office is 215 Euston Road, London, NW1 2BE.