classification
Title: IDLE tooltips.py: refactor and add docstrings and tests
Type: behavior Stage: patch review
Components: IDLE Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: 33907 Superseder:
Assigned To: terry.reedy Nosy List: taleinat, terry.reedy
Priority: normal Keywords: patch

Created on 2018-06-11 20:09 by terry.reedy, last changed 2018-06-20 07:36 by taleinat.

Files
File name Uploaded Description Edit
Coverage for Lib_idlelib_tooltip.py.png taleinat, 2018-06-13 19:33
Pull Requests
URL Status Linked Edit
PR 7683 open taleinat, 2018-06-13 14:41
Messages (14)
msg319337 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-06-11 20:09
calltip_w.py code is partly based on tooltip.py.  The latter is currently unused.  But it is needed, with changes, for squeezer (#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.
msg319391 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-12 14:18
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
msg319468 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-13 14:42
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!
msg319483 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-13 19:33
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.
msg319841 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-06-18 00:50
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.
msg319844 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-06-18 00:59
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.
msg319928 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-19 05:59
> 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.
msg320005 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-06-19 23:07
I am going to change calltips.py and CallTips to calltip.py and Calltip in #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.
msg320022 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-06-20 06:21
I just merged 2nd PR for #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.
msg320023 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-20 06:22
>> 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?
msg320024 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-20 06:28
> I [...] would like the popups to respond keyboard (Tab) focus changes as well as mouse movement focus changes.

Should tooltips also be shown automatically when the keyboard focus is on a widget?  Using the same delay as for the mouse hover, perhaps?  I've never seen this in another app that I can recall.

If so, should it be possible for two tooltips to be shown at once, one for the mouse hover and one for the keyboard focus?

If keyboard activation is desired, perhaps we should have a keyboard shortcut for that?  We could use the one currently used for showing calltips in the editor.  Is there a standard/convention for this on any/all OS?
msg320029 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-20 06:53
> I just merged 2nd PR for #33907.

I've merged master into the PR branch for this issue and fixed the merge issues.
msg320030 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-06-20 07:07
> if: versus try: The latter is faster if try: succeeds. On the other hand, we could make the if: fail by mocking it.  Since there is no except: or else: code to test, it hardly seems worth it.  A low priority item.

Generalizing 'show calltip': Great idea, as in "Why didn't I think of that!".  Works if cursor on same line as squeeze box.  Either same delay or calculate from hover delay.  If we have different popup contents, I think 1 at a time would be fine.
msg320031 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-06-20 07:36
>> If so, should it be possible for two tooltips to be shown at once, one for the mouse hover and one for the keyboard focus?

> If we have different popup contents, I think 1 at a time would be fine.

Consider the following example: A window with two buttons, each with a tooltip.  The keyboard focus is on one while the mouse hovers over another.

IMO there should never be more than a single tooltip displayed at once (perhaps not including calltips).  We can have tooltips auto-hide when another is displayed (and no-op if itself it attempted to be re-displayed).

Having keyboard focus changes automatically trigger tooltip display (with or without delay) could interact poorly with the above in some cases, e.g. a mouse-hover tooltip suddenly disappearing due to a keyboard delay kicking in and displaying another tooltip.

I'm also not sure keyboard focus should affect tooltips shown by mouse-hover.  If we have tooltips triggered by the keyboard, however, then the "anchor" widget losing keyboard focus should certainly hide the tooltip.  I lean in favor of simplicity and consistency.  IMO any keyboard focus change should likely hide all tooltips.

I suggest the following:

1. Add ability to show tooltips with the keyboard using the same keyboard shortcut as "<<force-open-calltip>>", which defaults to Ctrl+/.  Perhaps as a separate "<<show-tooltip>>" virtual event, or just rename "<<force-open-calltip>>" to that and use it for both.
2. No auto-display of tooltips triggered by a widget receiving keyboard focus.
3. Any keyboard focus change should hide all tooltips and calltips.
History
Date User Action Args
2018-06-20 07:36:45taleinatsetmessages: + msg320031
2018-06-20 07:07:22terry.reedysetmessages: + msg320030
2018-06-20 06:53:48taleinatsetmessages: + msg320029
2018-06-20 06:28:34taleinatsetmessages: + msg320024
2018-06-20 06:22:04taleinatsetmessages: + msg320023
2018-06-20 06:21:41terry.reedysetmessages: + msg320022
2018-06-19 23:07:35terry.reedysetdependencies: + IDLE: Rename calltips and CallTips as calltip and Calltip.
messages: + msg320005
2018-06-19 05:59:39taleinatsetmessages: + msg319928
2018-06-18 00:59:59terry.reedysetmessages: + msg319844
2018-06-18 00:50:20terry.reedysetmessages: + msg319841
2018-06-13 19:33:29taleinatsetfiles: + Coverage for Lib_idlelib_tooltip.py.png

messages: + msg319483
2018-06-13 14:42:55taleinatsetmessages: + msg319468
2018-06-13 14:41:41taleinatsetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request7296
2018-06-12 21:53:01terry.reedysetmessages: - msg319406
2018-06-12 21:52:41terry.reedysetmessages: + msg319406
2018-06-12 14:18:55taleinatsetmessages: + msg319391
2018-06-11 20:16:10terry.reedylinkissue1529353 dependencies
2018-06-11 20:09:40terry.reedycreate