-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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-35196: optimize Squeezer's write() interception #10454
bpo-35196: optimize Squeezer's write() interception #10454
Conversation
After partial first review: Squeezer only applies to Shell, not editor or output windows. So it seems that ii should only be imported into pyshell and the handler only bound in PyShell. This itself could be fixed in another issue. However, some of the existing and proposed code The Squeezer.init check "if isinstance(editwin, PyShell):" seems unneeded as the event handler function can only be called from the Shell write() and context menu. The statement "Tab width is configurable" in Squeezer.count_lines is not true in IDLE and especially not in Shell. Configdialog lines 8 and 9, written in 2003 and 2005, say The linewidth stuff is ugly. Tk initially calculates pixel width from configured width in characters, (average) font character width, padding. Too bad it apparently does not itself do the reverse calculaton need for squeezer. |
When you're done making the requested changes, leave the comment: |
811214a
to
b77de8c
Compare
@terryjreedy, following your comment on the subject, I did some timings with and without the second short-circuit check and found that it had no significant effect even in the most extreme cases it was meant to optimize. I've removed it to avoid the unnecessary complexity. |
This is cleaner and allows simplifying Squeezer.reload().
9275b43
to
485d954
Compare
Done. 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. |
I am reviewing this and making a few changes now. |
That assertion still fails on AppVeyor: What should we do with this?
|
It passes locally on Windows 10, by the way. |
It fails on Travis-CI too :( Do we just drop this assertion? |
Cheryl's Ubuntu off-by-1 rounding failure and the CI no-change failures had different causes. I am experimenting with her suggested changes that avoid idleConf and expect to push something for the CI bots. Whoops, hit wrong comment button. |
Lib/idlelib/squeezer.py
Outdated
def load_font(self): | ||
text = self.base_text | ||
self.zero_char_width = \ | ||
Font(text, name=text.cget('font')).measure('0') |
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.
Bug. The font name is a string added to tk's registry of fonts. The tuple gets turned into a string. Since I cannot accept a suggestion after making myself assignee (contrary to (i) information popup, I will change 'name' to 'font' in the web editor and see if it changes CI bots. (Test still pass on my machine.)
Thank you Cheryl, your suggestion works for the CI bots. After experimentation, I expect the new code should work even if a system does not have 'Courier'. On my machine, an unknown family is replaced by a default system font (Ariel, for me) and I assume that this is universal tk behavior. Tal, go ahead and merge. |
Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
GH-11541 is a backport of this pull request to the 3.7 branch. |
The new functionality of Squeezer.reload() is also tested, along with some general re-working of the tests in test_squeezer.py. (cherry picked from commit 39a33e9) Co-authored-by: Tal Einat <[email protected]>
|
|
The problematic
|
I will open a new issue and PR to fix this, initially by disabling it. I approved the backport since I will follow with the initial fix and I want 3.7 in sync with master. |
The new functionality of Squeezer.reload() is also tested, along with some general re-working of the tests in test_squeezer.py. (cherry picked from commit 39a33e9) Co-authored-by: Tal Einat <[email protected]>
Issue 35730, PR #11543. |
This also cleans up and improves the testing of
count_lines()
, which has been significantly changed as part of this optimization.https://bugs.python.org/issue35196