From: SourceForge.net <no...@so...> - 2012-08-31 14:34:23
|
Bugs item #2443069, was opened at 2008-12-17 15:21 Message generated for change (Comment added) made by andygoth You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=112997&aid=2443069&group_id=12997 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: 53. [selection] Group: development: 8.6b2 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Andy Goth (andygoth) Assigned to: Donal K. Fellows (dkf) Summary: Cannot replace [selection handle] command Initial Comment: % package require Tk % proc report {string args} {return $string} % selection handle . {report 1} % selection own . % selection get 1 % selection get -type UTF8_STRING 1 % selection handle . {report 2} % selection get 2 % selection get -type UTF8_STRING 1 When replacing the [selection handle] command, the automatically generated UTF8_STRING handler is *not* updated, only the STRING handler. The attached patch fixes Tk_CreateSelHandler() to update both. It also updates the Tk_CreateSelHandler() documentation to describe the automatic UTF8_STRING handler creation, updating, and removal. I tested the patch successfully on X11. Workaround 1: Use [selection handle -type UTF8_STRING] to explicitly set the UTF8_STRING handler in addition to the STRING handler. Workaround 2: Use [selection handle $win ""] to clear the handler before changing it. ---------------------------------------------------------------------- >Comment By: Andy Goth (andygoth) Date: 2012-08-31 07:34 Message: I got the core.tcl.tk account, but beyond that I've had no time to work on it. I apologize. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-31 05:35 Message: Any progress on this? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-07-13 05:11 Message: (let me know via email; this doesn't belong in this bug report…) ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-07-13 05:10 Message: I can set up the account easily. Andy, let me know what username you want and an email address and I'll get it sorted. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-07-13 00:26 Message: >ask him or a volunteer to test patches I write. Andy, My suggestion would be to get commit access on core.tcl.tk (Donal, can you give that to Andy?) and work on the bug-2443069 branch. Then anyone can test/review your changes from there. I can help you with merging it to trunk, after everything is OK. A test-case which reproduces the problem discovered by Don (which crashes when multiple handers have the same clientData), would be the perfect way to prove correctness of your upcoming fixes. ---------------------------------------------------------------------- Comment By: Andy Goth (andygoth) Date: 2012-07-12 12:47 Message: I'll have a more detailed look tonight. I currently lack access to a good development environment, so it may be a while before I can test any fixes. dgp put a lot of good information in 3543050, so I might be able to go forward from there and ask him or a volunteer to test patches I write. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-07-12 11:46 Message: Bug 3543050 shows that the current patch is not ready to be released yet. I don't want the 8.6b3 release to be blocked because of that. Andy, please have a look at this. I now reverted the change from trunk, but it still is in the bug-2443069 branch. (Donal, can you add andygoth to the tktoolkit project, and assign this issue to him? That seems most logical) ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-07-11 12:54 Message: Merging to trunk done now. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-07-10 05:11 Message: Whoops; meant to mention before that merging is the task that's left. :-) ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-07-05 03:36 Message: Ping! With everything sorted out, what's left to be done, apart from merging this to trunk? ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-07-01 04:04 Message: >And the minimal fix is to update the expected output to the real output; I don't agree. It appears that switching the "selection handle" command to the new synax (see bug #2441988) fixes the bug as well, with the output as expected. updated bug-2443069 to trunk now, which makes all select/clipboard-related test pass. To me it's OK to be merged to trunk, I'm convinced. Donal? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-06-30 16:03 Message: And the minimal fix is to update the expected output to the real output; it's correct. We're just no longer throwing that information away completely and leaving bugs to fester. OSX/Aqua has a *lot* more failing tests. Don't know the significance yet. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-06-30 15:56 Message: Digging with 'fossil annotate' indicates that the provenance of that weird [selection] syntax is "the mists of time"; it predates Tk's switch to CVS. In fact, it probably predates Tk 4.0, as I don't ever remember seeing that syntax documented *anywhere* in all the years I've used it. I also don't *think* it is mentioned in the 1st ed Ousterhout book; it's **OLD**. I guess that: selection handle .f1 ERROR errHandler is equivalent to: selection handle .f1 -type ERROR errHandler (That is, it's the PRIMARY selection's ERROR target fetched in STRING wire format.) ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-06-30 15:34 Message: Tk doesn't get tested nearly as often as Tcl does, because it's entirely practical to do a run of Tcl's test suite in parallel with something else (only async.test is intrusive at all on this system). Tk's test suite is far less of a good neighbor (for good and obvious reasons). It's also too dependent on the fonts installed and the way the window manager behaves. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-06-26 14:14 Message: It turns out that test select-5.15 is already failing for ages (why did no-one notice that before....). This test was written to test for bug 2441988, so I re-opened that one. Conclusion: the failure of select-5.15 has nothing to do with Andy's [selection handle] changes. See: <http://core.tcl.tk/tk/info/bd1165a06d17b3df426b97cda3e7df675656a81e> Donal, it appears that you know a lot more about selection handles than I do. If you are OK with merging this with trunk, I am as well. It's too small to let it go though the TIP process. ---------------------------------------------------------------------- Comment By: Andy Goth (andygoth) Date: 2012-06-20 07:53 Message: I don't understand it all that well, but it looks very fishy to me. Take this line: selection handle .f1 ERROR errHandler The man page says: selection handle ?-selection s? ?-type t? ?-format f? window command I see no -selection, -type, or -format options, so .f1 is window and... well, what's ERROR supposed to be? The intended command is [errHandler], since that generates the expected output string. Maybe ERROR should be the value for the -type option, but that's a guess; wouldn't that also cause the result to *not* include an error about ERROR not being defined? I'm not sure why this test ever worked, why my patch broke it, or what it's even trying to accomplish. Some of the other tests contain this same [selection handle] line, though select-5.15 is the only one that tests for the "selection handler aborted" failure, so if this one line of code is faulty, it'll likely only matter for select-5.15. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-06-10 11:40 Message: Donal, Thanks! I believe you. (personally, I never used the selection function, so I don't consider myself an expert in this) So, after doing what Donal suggests, there is one more failure: ==== select-5.15 Tk_GetSelection procedure FAILED ==== Contents of test case: proc ::bgerror msg {lappend ::bgerrors $msg} selection handle .f1 ERROR errHandler list [catch {selection get ERROR} msg] $msg [update] {*}$::bgerrors ---- Result was: 1 {PRIMARY selection doesn't exist or form "ERROR" not defined} {} ---- Result should have been (exact matching): 1 {PRIMARY selection doesn't exist or form "ERROR" not defined} {} {selection handler aborted} ==== select-5.15 FAILED What should be done about this one? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-06-09 09:37 Message: Those tests look like bystanders, with the thing tested for (equality of target list) being a proxy for what is interesting (TEST present in target list or not). Just update the expected test results. :-) ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-06-09 00:33 Message: committed to branch bug-2443069 Looks good to me. The only problem with the patch is that it causes two test failures (see below), so those have to be fixed first before it can be merged to trunk. ==== clipboard-5.1 Tk_ClipboardClear procedure FAILED ==== Contents of test case: clipboard append -t TEST "test" set result [lsort [clipboard get TARGETS]] clipboard clear list $result [lsort [clipboard get TARGETS]] ---- Result was: {MULTIPLE TARGETS TEST TIMESTAMP TK_APPLICATION TK_WINDOW UTF8_STRING} {MULTIPLE TARGETS TIMESTAMP TK_APPLICATION TK_WINDOW UTF8_STRING} ---- Result should have been (exact matching): {MULTIPLE TARGETS TEST TIMESTAMP TK_APPLICATION TK_WINDOW} {MULTIPLE TARGETS TIMESTAMP TK_APPLICATION TK_WINDOW} ==== clipboard-5.1 FAILED ==== clipboard-5.2 Tk_ClipboardClear procedure FAILED ==== Contents of test case: clipboard append -t TEST "test" set result [lsort [clipboard get TARGETS]] selection own -s CLIPBOARD . lappend result [lsort [clipboard get TARGETS]] clipboard clear clipboard append -t TEST "test" lappend result [lsort [clipboard get TARGETS]] ---- Result was: MULTIPLE TARGETS TEST TIMESTAMP TK_APPLICATION TK_WINDOW UTF8_STRING {MULTIPLE TARGETS TIMESTAMP TK_APPLICATION TK_WINDOW} {MULTIPLE TARGETS TEST TIMESTAMP TK_APPLICATION TK_WINDOW UTF8_STRING} ---- Result should have been (exact matching): MULTIPLE TARGETS TEST TIMESTAMP TK_APPLICATION TK_WINDOW {MULTIPLE TARGETS TIMESTAMP TK_APPLICATION TK_WINDOW} {MULTIPLE TARGETS TEST TIMESTAMP TK_APPLICATION TK_WINDOW} ==== clipboard-5.2 FAILED ---------------------------------------------------------------------- Comment By: Andy Goth (andygoth) Date: 2012-06-08 16:12 Message: This has been hanging out for three to four years. Please, somebody have a look at it. I still need to manually apply this patch to get my application to work. ---------------------------------------------------------------------- Comment By: Andy Goth (andygoth) Date: 2011-04-17 18:38 Message: I fixed some typos in the patch. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2010-08-18 01:16 Message: It would be nice to be able to do this. ---------------------------------------------------------------------- Comment By: Andy Goth (andygoth) Date: 2009-01-06 10:05 Message: Tiny amendment to the patch: Replace "but" with "that" in the line "existing STRING handler is also being replaced but has a different" in the patch for doc/CrtSelHdlr.3 . ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=112997&aid=2443069&group_id=12997 |