-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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-17535: IDLE: increase line number horizontal padding by 2 pixels #14959
bpo-17535: IDLE: increase line number horizontal padding by 2 pixels #14959
Conversation
@aeros167 |
The Azure pipelines is a timeout failure of the Mac test. This happens on and off. Not related. |
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.
@taleinat Thanks for the PR, I definitely like the look horizontal padding of 2 better than before. However, I think 4 might be a slight further improvement. To compare the two, I did a side-by-side screenshot. The one on the left has a padding of 2 and the one on the right has a padding of 4:
Tested on Linux using a large font size of 29 with the Arial font style so that the gap is easier to compare between the two.
Since it's a bit subjective, it might be a bit difficult to "objectively" determine which one looks better, but I personally have a preference towards the one of the right with the horizontal padding of 4. Let me know what you think. I would definitely approve of the PR either way though (:
@@ -51,7 +51,7 @@ def __init__(self, editwin): | |||
|
|||
_padx, pady = get_widget_padding(self.text) | |||
self.sidebar_text = tk.Text(self.parent, width=1, wrap=tk.NONE, | |||
padx=0, pady=pady, | |||
padx=2, pady=pady, |
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.
padx=2, pady=pady, | |
padx=4, pady=pady, |
@terryjreedy If the mac tests are periodically timing out, would it be helpful to increase the timeout duration beyond the present 60 to something like 80? I'm not sure what that would involve, I don't have much experience with regards to modifying integration test settings or know where the CPython testing configuration file would be located. Also, I did the review without reading the comments (besides the top one taleinat made), so I didn't see your instructions, my apologies. Would you like me to do another review following your instructions or is the one that I did adequate? Between 1 and 2, I would lean towards two, but I personally have preference of padx=4. However, as I mentioned in the review, this might be quite subjective. I would consider any increase to be an improvement though. |
@aeros167 If there is not an issue for Mac tests timing out, and I would search first, there should be, with Victor Stinner and Steve Dower nosey. I suspect that the hung tests will not finish with 20 more minutes. You images lack letters in the text for comparison. A total padding of 3 on the number side is already more than the total 2 on the text side. It may be the the absolute padding in real units (mm) should depend somewhat on the absolute height, in which case the pixel padding would depend on the pixels/inch or cm. At least in some places, numbers can be given with real units. Right now, I am concerned with getting merges done for a3, scheduled Monday. So I am going to merge this as is. It is better than no change. Anything more sophisticated should be a new issue, not this closed one. |
Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
I'm having trouble backporting to |
…nGH-14959) (cherry picked from commit 46ebd4a) Co-authored-by: Tal Einat <taleinat@gmail.com>
GH-14973 is a backport of this pull request to the 3.7 branch. |
Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
…nGH-14959) (cherry picked from commit 46ebd4a) Co-authored-by: Tal Einat <taleinat@gmail.com>
GH-14974 is a backport of this pull request to the 3.8 branch. |
@terryjreedy Oh okay, that makes sense. Personally, I think just having line numbers in general is a huge improvement, and adding this change to slightly adjust the margin provides a significant improvement. Also, with regards there not being letters, I was primarily referring to the gap between the line numbers and the dividing line in my feedback in the BPO issue, rather than the gap between the numbers and the letters. Apologies if I wasn't clear on that. Is that managed by a separate property? |
) (cherry picked from commit 46ebd4a) Co-authored-by: Tal Einat <taleinat@gmail.com>
) (cherry picked from commit 46ebd4a) Co-authored-by: Tal Einat <taleinat@gmail.com>
I understand. I was saying that in my opinion, spacing should be looked at in context, with real text. But anyway, this is for a followup. |
As suggested on the issue tracker, though I found that adding 2 pixels looks much nicer than adding just 1.
https://bugs.python.org/issue17535