bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683

taleinat · 2018-06-13T14:41:40Z

The test coverage is at ~92% (when ignoring the manual "htest" test code).

https://bugs.python.org/issue33839

* remove ListboxToolTip (unused and ugly) * slightly refactor base class from concret ToolTip class * whitespace

* make CallTip and ToolTip sub-classes of a common abstract base class * split logic out into methods, move generic common logic to base-class * more code cleanup

taleinat · 2018-06-14T06:26:31Z

@terryjreedy, do changes like this need a NEWS entry?

terryjreedy · 2018-06-14T06:51:34Z

Yes. If there is a bpo-number, and all non-trivial issues should, it should have a blurb. An exception would be if a PR is a follow-up on the same issue, rather than a new issue, especially if done before closing it.

terryjreedy · 2018-06-14T06:52:56Z

If you don't do it first, I would try to remember to add one when I pull to run the tests.

terryjreedy · 2018-06-14T06:55:15Z

I am sometime not sure what to write until the issue is complete. Unlike commit messages, it is relatively easy to edit blurbs later, just as with other documentation.

# Conflicts: Lib/idlelib/calltip_w.py, fixed.

…get'.

terryjreedy · 2018-06-17T21:42:10Z

Sorry for typos in commit message. It should be
Change 'widget', changed to '_text_widget' in calltip_w, to 'text_widget'.
See coming review comment.

but is required (on Windows) for proper coverage.

terryjreedy

First, high-level review:

As near as I can tell, calltips are working fine after the refactoring. Experiment appears to be successful.
More docstrings are needed for both calltips and tooltip. Test_calltip_w needs much more coverage. I expect to add some when I review individual methods.
See comments for general style issues.

terryjreedy · 2018-06-17T21:46:54Z

Lib/idlelib/calltip_w.py

-HIDE_VIRTUAL_EVENT_NAME = "<<calltipwindow-hide>>"
+from idlelib.tooltip import ToolTipBase
+
+HIDE_EVENT = "<<calltipwindow-hide>>"


I am not a fan of (sometimes) using CAPS NAMES for meaningful values, as opposed to arbitrary labels like error 'numbers, which are never imported any where else. The are harder to type and, to me, read. This one was especially over-verbose. Since we are editing the module anyway, I condensed this.

-Done at top and must be consistent as tests pass.

terryjreedy · 2018-06-17T21:55:51Z

Lib/idlelib/calltip_w.py

+
+ def __init__(self, text_widget):
+ super(CallTip, self).__init__(text_widget)
+ self.text_widget = self.anchor_widget


Since all idlelib implementation modules are now private (PEP 434, put into effect in 3.6), I don't see any reason (anymore) to sprinkle privatizing leading underscores here and there. (One module even has mangling double underscores that are probably unneeded.) I removed the one you added on 'text_widget' since I use that in the test. I would like the leading underscores in tooltips either removed or justified.

It appears that text_widget and anchor_widget are always the same thing, so why two names?

At one point during my development it made things clearer, but at this point it is rather pointless.

Re. leading underscores, I find them useful with classes to mark methods as not being part of their external API. Would you like them removed in those cases too, e.g. _bind_events()?

I want to defer a complete discussion, which means 'not for now'.

terryjreedy · 2018-06-17T23:16:04Z

Lib/idlelib/calltip_w.py

+class CallTip(ToolTipBase):
+ """a call-tip widget for tkinter text widgets"""
+
+ def __init__(self, text_widget):


Docstring for init should briefly explain attributes initialized.

I will try to do this.

terryjreedy · 2018-06-17T23:22:20Z

Lib/idlelib/tooltip.py

 def showtip(self):
+ """display the tooltip"""


In the tests, self.tipwindow is always true, so 'return' is missed. This should be guarding against something that can happen, such as cntl-\ (open calltip) with calltip open. For me, window blinks, suggesting 2nd request in not ignored as well as it should be. If opening a calltip does not properly cancel a pending 'open' callback, the latter could issue a repeat 'open'.

I've added a test to exercise this.

terryjreedy · 2018-06-17T23:26:14Z

Lib/idlelib/tooltip.py

+
+ def __del__(self):
+ try:
+ self.anchor_widget.unbind("<Enter>", self._id1)


In test, this raises TclError, so next two unbinds do not happen.

That's intended, since in that case the next two unbind() calls would just raise the same exception.

I will take another look at the test.

bedevere-bot · 2018-06-18T00:26:34Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

# Conflicts: # Lib/idlelib/calltip_w.py

Code changes are just minor restructuring while preserving logic.

taleinat · 2018-06-29T18:47:41Z

I have made the requested changes; please review again.

bedevere-bot · 2018-06-29T18:47:43Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

taleinat · 2018-06-29T18:50:48Z

@terryjreedy, your analysis of the refactoring of Calltip and Tooltip to have a common base class is spot-on.

Can the hover class be more or less used as is for squeezer?

Yes.

terryjreedy · 2018-08-03T23:54:58Z

The master merge was the merge conflict resolution, which restored patched calltip_w to what it was.

Adding self.tipwindow.update_idletasks() to the baseclass show_tip should make the following work on MacOS with 8.6.??:
python3 -m idlelib.calltip_w # the htest part, in particular
python3 -m idlelib.tooltip # the htest part, in particular
python3 -m idlelib # calltips should work
where 'python3' starts your repository build.

A tooltip, for tkinter and IDLE, is a Toplevel with a Label with tip text. Calltips are Text-widget based tooltips. Hovertip better describes what you want for Squeezer. Hence Texttip > Hovertip.

I want to make more changes, including folding Hovertip into its base class, as I will explain on the issue, but you only need a somewhat stable API, not the final implementation, to proceed with squeezer. So merge whenever you want after tests pass.

terryjreedy · 2018-08-03T23:56:36Z

Whoops, need to edit test for class name change. I will do it after dinner if you don't first.

miss-islington · 2018-08-05T06:21:11Z

Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-08-05T06:21:36Z

GH-8675 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2018-08-05T06:21:43Z

GH-8676 is a backport of this pull request to the 3.6 branch.

…sts (pythonGH-7683) * make CallTip and ToolTip sub-classes of a common abstract base class * remove ListboxToolTip (unused and ugly) * greatly increase test coverage * tested on Windows, Linux and macOS (cherry picked from commit 87e59ac) Co-authored-by: Tal Einat <[email protected]>

…sts (GH-7683) * make CallTip and ToolTip sub-classes of a common abstract base class * remove ListboxToolTip (unused and ugly) * greatly increase test coverage * tested on Windows, Linux and macOS (cherry picked from commit 87e59ac) Co-authored-by: Tal Einat <[email protected]>

taleinat added 6 commits June 12, 2018 17:09

bpo-33839: cleanup tooltip.py

41c1e43

* remove ListboxToolTip (unused and ugly) * slightly refactor base class from concret ToolTip class * whitespace

bpo-33839: refactor CallTip and ToolTip classes

b0e9a84

* make CallTip and ToolTip sub-classes of a common abstract base class * split logic out into methods, move generic common logic to base-class * more code cleanup

bpo-33839: add doc-string and comments and rename a few variables

263117c

bpo-33839: avoid Tcl exceptions upon closing IDLE with a call-tip shown

187e0e5

bpo-33839: add automated tests for tooltip.py (92% coverage)

6fb9ba9

bpo-33839: add another tooltip test and improve existing tests

f84a2b9

taleinat requested a review from terryjreedy as a code owner June 13, 2018 14:41

the-knights-who-say-ni added the CLA signed label Jun 13, 2018

bedevere-bot added the awaiting merge label Jun 13, 2018

taleinat and others added 5 commits June 14, 2018 13:24

bpo-33839: add NEWS entry

ef721ca

Merge remote-tracking branch 'origin/master' into pr_7683

f2857dd

# Conflicts: Lib/idlelib/calltip_w.py, fixed.

Change 'widget', changes to '_text_widget' in calltip_w, to 'test_wid…

eeed210

…get'.

Quote string arg in htest.

5afa8e1

Shorten HIDE_VIRTUAL_EVENT_NAME to HIDE_EVENT.

1198a0c

terryjreedy added 2 commits June 17, 2018 18:27

Add unittest invocation. This not only allows running test from editor,

d9bcc1f

but is required (on Windows) for proper coverage.

Improve message for calltip htest.

bae3bb2

terryjreedy requested changes Jun 18, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jun 18, 2018

taleinat added 5 commits June 20, 2018 09:08

bpo-33839: change capitalization: "ToolTip" -> "Tooltip"

dd677b5

bpo-33839: add doc-strings for __init__ methods in tooltip.py

1c990f3

Merge remote-tracking branch 'upstream/master' into bpo-33839

c55ca31

# Conflicts: # Lib/idlelib/calltip_w.py

bpo-33839: remove duplication of .anchor_widget as .text_widget

a1b22e4

bpo-33839: improve doc-strings and comments plus minor code cleanup

3ecbd65

Code changes are just minor restructuring while preserving logic.

bpo-33839: remove Tooltip.is_active()

3847011

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 29, 2018

terryjreedy added 2 commits August 3, 2018 19:17

Merge branch 'master' into bpo-33839

5d92ba0

update_idletasks in show_tip, Texttip > Hovertip

8adaa77

terryjreedy added type-feature A feature request or enhancement needs backport to 3.6 labels Aug 3, 2018

terryjreedy approved these changes Aug 3, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 3, 2018

terryjreedy and others added 2 commits August 3, 2018 20:58

Make tests pass with Hovertip.

23d57c8

bpo-33839: document hover_delay parameter and fix indentation

98f4ae4

taleinat merged commit 87e59ac into python:master Aug 5, 2018

bedevere-bot removed awaiting merge needs backport to 3.7 labels Aug 5, 2018

bedevere-bot removed the needs backport to 3.6 label Aug 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683

bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683

taleinat commented Jun 13, 2018 •

edited by bedevere-bot

Loading

taleinat commented Jun 14, 2018

terryjreedy commented Jun 14, 2018

terryjreedy commented Jun 14, 2018

terryjreedy commented Jun 14, 2018

terryjreedy commented Jun 17, 2018

terryjreedy left a comment

terryjreedy Jun 17, 2018 •

edited

Loading

terryjreedy Jun 17, 2018

terryjreedy Jun 17, 2018

taleinat Jun 20, 2018

taleinat Jun 20, 2018

terryjreedy Jun 29, 2018

terryjreedy Jun 17, 2018

terryjreedy Jun 29, 2018

terryjreedy Jun 17, 2018

taleinat Jun 22, 2018

terryjreedy Jun 17, 2018

taleinat Jun 20, 2018

terryjreedy Jun 29, 2018

bedevere-bot commented Jun 18, 2018

taleinat commented Jun 29, 2018

bedevere-bot commented Jun 29, 2018

taleinat commented Jun 29, 2018

terryjreedy commented Aug 3, 2018

terryjreedy commented Aug 3, 2018

miss-islington commented Aug 5, 2018

bedevere-bot commented Aug 5, 2018

bedevere-bot commented Aug 5, 2018

bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683

bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683

Conversation

taleinat commented Jun 13, 2018 • edited by bedevere-bot Loading

taleinat commented Jun 14, 2018

terryjreedy commented Jun 14, 2018

terryjreedy commented Jun 14, 2018

terryjreedy commented Jun 14, 2018

terryjreedy commented Jun 17, 2018

terryjreedy left a comment

Choose a reason for hiding this comment

terryjreedy Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 18, 2018

taleinat commented Jun 29, 2018

bedevere-bot commented Jun 29, 2018

taleinat commented Jun 29, 2018

terryjreedy commented Aug 3, 2018

terryjreedy commented Aug 3, 2018

miss-islington commented Aug 5, 2018

bedevere-bot commented Aug 5, 2018

bedevere-bot commented Aug 5, 2018

taleinat commented Jun 13, 2018 •

edited by bedevere-bot

Loading

terryjreedy Jun 17, 2018 •

edited

Loading