IDLE tooltips.py: refactor and add docstrings and tests

[terryjreedy]

BPO	33839
Nosy	@terryjreedy, @taleinat, @miss-islington
PRs	bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683 [3.7] bpo-33839: refactor IDLE's tooltips & calltips, add docstrings and tests (GH-7683) #8675 [3.6] bpo-33839: refactor IDLE's tooltips & calltips, add docstrings and tests (GH-7683) #8676
Dependencies	bpo-33907: IDLE: Rename calltips and CallTips as calltip and Calltip.
Files	Coverage for Lib_idlelib_tooltip.py.png

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2018-08-05.06:59:56.624>
created_at = <Date 2018-06-11.20:09:40.604>
labels = ['3.8', 'expert-IDLE', 'type-bug', '3.7']
title = 'IDLE tooltips.py: refactor and add docstrings and tests'
updated_at = <Date 2018-08-05.06:59:56.623>
user = 'https://github.com/terryjreedy'

bugs.python.org fields:

activity = <Date 2018-08-05.06:59:56.623>
actor = 'taleinat'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2018-08-05.06:59:56.624>
closer = 'taleinat'
components = ['IDLE']
creation = <Date 2018-06-11.20:09:40.604>
creator = 'terry.reedy'
dependencies = ['33907']
files = ['47641']
hgrepos = []
issue_num = 33839
keywords = ['patch']
message_count = 17.0
messages = ['319337', '319391', '319468', '319483', '319841', '319844', '319928', '320005', '320022', '320023', '320024', '320029', '320030', '320031', '323130', '323131', '323133']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'taleinat', 'miss-islington']
pr_nums = ['7683', '8675', '8676']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33839'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[terryjreedy]

calltip_w.py code is partly based on tooltip.py. The latter is currently unused. But it is needed, with changes, for squeezer (bpo-1529353). So I would like to see if we can make an improved Tooltip class that can be used or subclassed by both.

The current subclasses should be deleted unless actually used. Docstrings should be added to what remains. Then test_tooltip, with 100% coverage, should be added. If needed, code can be changed to make testing easier.

[taleinat]

I've started work on this. My progress can be followed on the bpo-33839 branch on my GitHub repo: https://github.com/taleinat/cpython/tree/bpo-33839

[taleinat]

The refactoring is done, manual testing with the htests works, and I've added automated tests with ~92% code coverage.

Terry, I'd be happy for your review!

[taleinat]

Attaching coverage report for tooltip.py.

The uncovered lines are required to avoid exceptions when closing a windows with a tooltip shown. These are not currently reproduced by test_tooltip.py because my attempts to exercise them in automated tests have all failed so far.

The TclError exceptions are reproduced by the htest in tooltip.py. The ValueError exception, however, I've only seen happen when actually running IDLE, since it is raised by the MultiCall module.

IMO it's not worth the effort to achieve 100% automated test coverage here. It's Terry's decision to make, though.

[terryjreedy]

What do you think of changing 'ToolTip' to 'Tooltip'? The me, the extra 'T' is over-aggressive camel-casing that impedes rather than aids reading.

test_tooltip.py was missing the unittest invocation at the end, so I could not the tests from an editor. Without it, coverage on my Win 10 was 24%. With it, 91% (same missing coverage).

There are cases where 100% is close enough to impossible to just move on. I agree that the misse here are not necessarily showstoppers. But I like to check whether the failure does not suggest a code change.

TclError from widget.destroy suggests that we *might* be able to cleanup better.

"The TclError exceptions are reproduced by the htest in tooltip.py. ":
With the code as is? Or if the try-except is removed?

ValueError: where?

Other comments in the review.

[terryjreedy]

As I mentioned elsewhere, I can a) imagine using this with dialogs to pop up hints about the particular entry widget, and b) would like the popups to respond keyboard (Tab) focus changes as well as mouse movement focus changes. Looking at the config dialog, keyboard focus and mouse focus seems to be independent and parallel systems, so I don't know how easy it would be to respond to both. In general, keyboard focus is indicated by a black dotted line ring, while mouse focus (activation) is indicated by a color change (blue on Windows), though for entry widgets, key focus in indicated by blue background while mouse focus is indicated by a cursor change.

[taleinat]

What do you think of changing 'ToolTip' to 'Tooltip'?

+1, I'll change it.

TclError from widget.destroy suggests that we *might* be able to cleanup better.

The try/except clauses which are hard to test are used since the order in which widgets are destroyed can vary, and I don't know how to properly check whether a widget still exists (so that e.g. unbinding events won't throw an exception).

For the on-hover tooltip, events are registered on the "anchor" widget. ISTM these events should be unregistered when the tooltip is destroyed, hence I added that to __del__. Any suggestions for a better cleanup mechanism?

> The TclError exceptions are reproduced by the htest in tooltip.py.
With the code as is? Or if the try-except is removed?
Without the try/except.

I [...] would like the popups to respond keyboard (Tab) focus changes as well as mouse movement focus changes.

Looking at the Tk docs, it seems this will require binding properly to the FocusIn and oFocusOut events. This is indeed a separate mechanism. It should be straightforward when bound to a simple widget such as a button. The Calltip class will need to override this since it already implements an appropriate alternative.

[terryjreedy]

I am going to change calltips.py and CallTips to calltip.py and Calltip in bpo-33907, but will not change tooltip.py there as I would expect a merge conflict. Once the change is made elsewhere, change should be made to tooltip and test_tooltip directly.

[terryjreedy]

I just merged 2nd PR for bpo-33907. Combined effect is that
module calltips and its class CallTips are now calltip and Calltip.
In module calltip_w class CallTip is now CalltipWindow.

[taleinat]

> TclError from widget.destroy suggests that we *might* be able to cleanup better.

Any suggestions for a better cleanup mechanism?

(Following up my own question with another question...)

Would it be better to check self.winfo_exists() instead of the try/except clauses?