-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-33839: IDLE tooltips.py: refactor and add docstrings and tests #7683
Conversation
* 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
@terryjreedy, do changes like this need a NEWS entry? |
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. |
If you don't do it first, I would try to remember to add one when I pull to run the tests. |
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.
Sorry for typos in commit message. It should be |
but is required (on Windows) for proper coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Lib/idlelib/calltip_w.py
Outdated
HIDE_VIRTUAL_EVENT_NAME = "<<calltipwindow-hide>>" | ||
from idlelib.tooltip import ToolTipBase | ||
|
||
HIDE_EVENT = "<<calltipwindow-hide>>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Lib/idlelib/calltip_w.py
Outdated
|
||
def __init__(self, text_widget): | ||
super(CallTip, self).__init__(text_widget) | ||
self.text_widget = self.anchor_widget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that text_widget and anchor_widget are always the same thing, so why two names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point during my development it made things clearer, but at this point it is rather pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to defer a complete discussion, which means 'not for now'.
Lib/idlelib/calltip_w.py
Outdated
class CallTip(ToolTipBase): | ||
"""a call-tip widget for tkinter text widgets""" | ||
|
||
def __init__(self, text_widget): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring for init should briefly explain attributes initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to do this.
Lib/idlelib/tooltip.py
Outdated
def showtip(self): | ||
"""display the tooltip""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test to exercise this.
|
||
def __del__(self): | ||
try: | ||
self.anchor_widget.unbind("<Enter>", self._id1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In test, this raises TclError, so next two unbinds do not happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's intended, since in that case the next two unbind()
calls would just raise the same exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take another look at the test.
When you're done making the requested changes, leave the comment: |
# Conflicts: # Lib/idlelib/calltip_w.py
Code changes are just minor restructuring while preserving logic.
I have made the requested changes; please review again. |
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
@terryjreedy, your analysis of the refactoring of Calltip and Tooltip to have a common base class is spot-on.
Yes. |
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.??: 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. |
Whoops, need to edit test for class name change. I will do it after dinner if you don't first. |
Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
GH-8675 is a backport of this pull request to the 3.7 branch. |
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 (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]>
…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]>
The test coverage is at ~92% (when ignoring the manual "htest" test code).
https://bugs.python.org/issue33839