Tk Source Code

View Ticket
Login
Ticket UUID: b68710aed673d0bb25d9c63a29eefd3b3a57d9e3
Title: Minor fixes to library/text.tcl bindings
Type: Patch Version: revised_text, core-8-6-branch
Submitter: kjnash Created on: 2018-01-13 15:54:05
Subsystem: 18. [text] Assigned To: fvogel
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2018-01-29 03:33:45
Resolution: Fixed Closed By: kjnash
    Closed on: 2018-01-29 03:33:45
Description:
Patch against commit [fa928fb21c] supplied.

1. Add test of -state normal to <Meta-d>.
2. Don't add autoseparators when doing <<Cut>> if the widget is disabled and the operation is therefore only a <<Copy>>.
3. Fix to iteration in ::tk::TextPrevPos.
4. Generate <<Selection>> event if <<PasteSelection>> modifies the selection itself.
User Comments: kjnash added on 2018-01-29 03:33:45:

revised_text [c410f6f045] tested again for all <<Selection>> issues, and works correctly.

Many thanks for your effort!


fvogel added on 2018-01-28 16:52:00:
Merged to core-8-6-branch and trunk.
Also merged to revised_text after fixing conflicts (all new tests pass in the revised_text branch).

kjnash added on 2018-01-24 09:51:46:
Your code works as intended: I can't find any cases where it does the wrong thing.

Many thanks for your work!

fvogel added on 2018-01-20 16:55:46:
I believe all cases of <<Selection>> triggering are handled now (no fix refinement was needed after all).

Keith, can you still find cases of missing (or spurious) <<Selection>> event?

Otherwise I'll merge and close this ticket.

Thanks!

kjnash added on 2018-01-18 04:06:56:
Yes, you're right about the independence from -exportselection. Well spotted!

fvogel added on 2018-01-18 00:51:14:

Fix for #4 (at the C code level) is now in the bugfix branch bug-b68710aed6. All tests I have added regarding <<Selection>> event do pass now with [6e14c473].

Keith, could you please have a look and test whether this is OK for you?

Two points to keep in mind while testing:

- I still need to refine the fix for <<Selection>> generation upon text insertion. The edge case is not yet dealt with.

- According to the man page, <<Selection>> should fire whenever the text widget selection is changed. This may or may not impact the PRIMARY (X) selection (this depends on -exportselection). The fix you originally proposed for #4 triggered <<Selection>> only if -exportselection is true. If I read the man page correctly this was incorrect.

Thanks for your help!


fvogel added on 2018-01-15 21:32:11:

Thanks for your detailed answer. Very clear and makes a lot of sense to me.

So far I did the following:

- #1 and #3 are in the revised_text branch only

- #2 is in the bug-b68710aed6 branch only

When merging forward normally (that is, the bugfix branch into core-8-6-branch, then into trunk and finally into revised_text), #2 will land as desired in the revised version as well.

I'll now have a look at implementing #4 in the C code. If implemented at the right level I believe it will take care at once of all the three cases of text widget selection changes that you have identified (<<PasteSelection>>, <Delete> and <<Cut>>), plus any other that we are perhaps still overlooking.


kjnash added on 2018-01-15 03:33:14:
Thank you for your comments.  In reply:

1. This change may not be appropriate in Tk 8.6.  In Tk 8.6, bindings do not test for "-state normal" (because the widget's "insert" and "delete" commands do nothing to a disabled widget).  In "revised_text", the commands still do nothing, but on first use in a disabled widget, "insert" or delete" writes a warning to stderr:

"tk::text: Attempt to modify a disabled widget is deprecated."

As part of this deprecation, all the Text bindings in "revised_text" that modify the widget have been altered to test whether the widget is disabled.  Only <Meta-d> was omitted, possibly because no common platforms use the Meta keyboard modifier.


2. Yes, in 8.6 the separators should not be added if the widget is not modified because it is disabled.


3a. ::tk::TextNextPos and ::tk::TextPrevPos set the value of variable "text" in the "while" loop.  In 8.6 they do this by concatenating values; in "revised_text" they use "append".  In ::tk::TextPrevPos there seems to be a mistake in the conversion, and the value of "text" is not the same as before.

To use the "while" loop in ::tk::TextPrevPos more than once, the search result must be on an earlier logical line than the insert mark.  To demonstrate the error, in a text widget try using <<PrevWord>> repeatedly: it works correctly until it has to jump to an earlier logical line.


3b. There is no error in the conversion of ::tk::TextNextPos from concatenation (8.6) to "append" ("revised_text").


4. Yes, I agree that <<Selection>> should be generated by the C code, so my code is really a workaround rather than a bugfix.

I have found two other cases where the text widget fails to notice that the selection has changed, and I have attached a second patch for these.  These cases are when the selection is deleted by either <Delete> or <<Cut>>.  Again, ideally these changes would be implemented in the C code.


So, in "revised_text", perhaps it is best to keep fixes 1, 2 and 3, and implement 4 in the C code.

In 8.6, I believe fixes 1 and 3 are unnecessary.  Fix 2 could be applied, and 4 could be implemented in the C code.

fvogel added on 2018-01-14 15:55:10:

Thanks for these improvements!

Since I would like to make the legacy text widget benefit from this also, I plan to fix it first, and then propagate forward to the revised version.

Items 1. and 2. look perfectly correct to me. These are now in a bugfix branch bug-b68710aed6.

Regarding 3., could you please elaborate on the following:

3.a. Why is the current algorithm in ::tk::TextPrevPos incorrect? Can you show a scenario where it currently fails?

3.b. Why are you not proposing a similar fix for ::tk::TextNextPos?

Regarding 4., proc ::tk::TextInsertSelection is specific to the revised text widget (it is a refactoring of legacy code) and is called, in the case we're interested in in your patch, by proc ::tk::TextPasteSelection, so the place where your proposed fix would go in text.tcl is different in the legacy and revised versions. But this is a detail I will handle if needed. What's more important to me is that I think generation of <<Selection>> if <<PasteSelection>> modifies the text widget selection should not happen in text.tcl. The <<Selection>> event should rather be generated by C code in text.c. I believe the C code should detect that the text widget selection is touched by any insert (or replace) operation and then generate the required <<Selection>> event. Currently generation of <<Selection>> is not made in text.tcl, in any case. I think you have correctly found a case when this event should be generated, however this generation should take place in text.c, probably by making TextInsertCmd() call TkTextSelectionEvent() whenever required. Don't you think so?


Attachments: