Skip to content
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

Jump to bottom

bpo-34864: warn if "Prefer tabs when opening documents" set to "Always" #10464

Merged
merged 6 commits into from Dec 7, 2018
Merged

bpo-34864: warn if "Prefer tabs when opening documents" set to "Always" #10464

merged 6 commits into from Dec 7, 2018

Conversation

@taleinat
Copy link
Contributor

@taleinat taleinat commented Nov 11, 2018 •
edited

The warning is displayed in new shell windows, except when running a command or a script. (This is identical to the existing behavior of the warning about problematic Tcl/Tk versions on macOS.)

https://bugs.python.org/issue34864
@taleinat
bpo-34864: warn if "Prefer tabs when opening documents" set to "Always"
4a3f7b8
@taleinat taleinat requested a review from terryjreedy as a code owner Nov 11, 2018
@taleinat
add NEWS entry
283dc54
ned-deily
ned-deily requested changes Nov 11, 2018
View changes
Copy link
Member

@ned-deily ned-deily left a comment

The idea is good in principle, thanks. But the code cannot assume that the preference is always available.

Lib/idlelib/macosx.py Outdated Show resolved Hide resolved
Lib/idlelib/macosx.py Outdated Show resolved Hide resolved
Lib/idlelib/macosx.py Outdated Show resolved Hide resolved
Lib/idlelib/macosx.py Outdated Show resolved Hide resolved
Lib/idlelib/macosx.py Outdated Show resolved Hide resolved
Lib/idlelib/macosx.py Outdated Show resolved Hide resolved
Lib/idlelib/pyshell.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 11, 2018

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

@taleinat
address code review comments
deb0ca9
@taleinat
Copy link
Contributor Author

@taleinat taleinat commented Nov 11, 2018

Thanks for the review @ned-deily!

I have made the requested changes; please review again.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 11, 2018

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

ned-deily
ned-deily approved these changes Nov 11, 2018
View changes
Copy link
Member

@ned-deily ned-deily left a comment

LGTM, thanks for making the changes. BTW, I notice that, with macOS 10.13 at least, if a change to "Always" is made while IDLE is running, the change is honored on the next window open; no need to restart IDLE. So perhaps the message can be simplified. Also, just an FYI, if a tab (rather than a window) is created, it can be converted to a window by control-clicking on the tab title bar and selecting the "Move Tab to New Window" option; this is a standard macOS feature with tabs and seems to work OK with IDLE.

terryjreedy
terryjreedy requested changes Nov 12, 2018
View changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Thank you both for PR and review. IDLE still start normally on Windows with patch.
I removed 2.7 backport only because it will have to be done by hand.
Besides CI tests, I think manually testing it on Mac should be enough.

Lib/idlelib/macosx.py Show resolved Hide resolved
Lib/idlelib/outwin.py Show resolved Hide resolved
Lib/idlelib/pyshell.py Show resolved Hide resolved
Lib/idlelib/macosx.py Show resolved Hide resolved
Misc/NEWS.d/next/IDLE/2018-11-11-17-13-50.bpo-34864.cw0PvO.rst Outdated
@@ -0,0 +1 @@
On macOS, warn if "Prefer tabs when opening documents" is set to "Always".
Copy link
Member

@terryjreedy terryjreedy Nov 12, 2018

For the idlelib/News.txt entry, I am thinking about the more specific

When opening IDLE with Shell on macOS, warn if System
Preference "Prefer tabs when opening documents" is set to "Always".

Do what you like best for the blurb.

Copy link
Contributor Author

@taleinat taleinat Nov 12, 2018

I prefer not mentioning the shell, since that's an implementation detail of how we currently show warnings, and could change in the future.

Copy link
Member

@terryjreedy terryjreedy Nov 15, 2018 •
edited

The issue I addressed is when the check is made, which appears to at at startup if Shell is opened and no code is run by command line options. If 'always' is set after IDLE is open, there will be no warning until the next start.

Where messages go is a different issue, except that not using Shell will allow checks when IDLE is opened to edit a file. config._warn prints to stderr (the console if IDLE started from one). I am thinking all messages from IDLE should go to a message log (output) window that can be saved or printed.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 12, 2018

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

And if you don't make the requested changes, you will be put in the comfy chair!

@taleinat
address second code review comments
81975d8
@taleinat taleinat force-pushed the bpo-34864/idle_warn_macos_prefer_tabs_setting branch from ae5a09c to 81975d8 Nov 12, 2018
@taleinat
Copy link
Contributor Author

@taleinat taleinat commented Nov 12, 2018

@ned-deily

BTW, I notice that, with macOS 10.13 at least, if a change to "Always" is made while IDLE is running, the change is honored on the next window open; no need to restart IDLE. So perhaps the message can be simplified.

Thanks, I've updated the warning message.

@taleinat
Copy link
Contributor Author

@taleinat taleinat commented Nov 12, 2018

Thanks for the review @terryjreedy!

I have made the requested changes; please review again.

Regarding the 2.7 backport, I'll do it manually after this goes in.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 12, 2018

Thanks for making the requested changes!

@ned-deily, @terryjreedy: please review the changes made to this pull request.

@terryjreedy
Copy link
Member

@terryjreedy terryjreedy commented Nov 15, 2018

If people changes to 'alway' while IDLE is running, there will be no warning until IDLE is started again. It is up to them to connect 'I changed the setting' to 'IDLE changed behavior'.

terryjreedy added 2 commits Dec 7, 2018
@terryjreedy
Merge remote-tracking branch 'origin/master' into pr_10464
3d49cf6
@terryjreedy
Add entry for idlelib/NEWS.txt.
a523b54
@terryjreedy
Copy link
Member

@terryjreedy terryjreedy commented Dec 7, 2018

Since this was written, there has been a question (report) about this on Stackoverflow. I added a note to idlelib/NEWS.txt and will merge when CI passes to get this in the upcoming rcs.

@terryjreedy terryjreedy merged commit 9ebe879 into python:master Dec 7, 2018
5 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Dec 7, 2018

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

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 7, 2018

@terryjreedy: Please replace # with GH- in the commit message next time. Thanks!

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 7, 2018

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

miss-islington added a commit to miss-islington/cpython that referenced this issue Dec 7, 2018
@taleinat @miss-islington
bpo-34864: warn if "Prefer tabs when opening documents" set to "Alway…
4239e0d 
…s" (pythonGH-10464)

* bpo-34864: warn if "Prefer tabs when opening documents" set to "Always"

* add NEWS entry

* address code review comments

* address second code review comments

* Add entry for idlelib/NEWS.txt.
(cherry picked from commit 9ebe879)

Co-authored-by: Tal Einat <taleinat+github@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 7, 2018

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

miss-islington added a commit to miss-islington/cpython that referenced this issue Dec 7, 2018
@taleinat @miss-islington
bpo-34864: warn if "Prefer tabs when opening documents" set to "Alway…
d865151 
…s" (pythonGH-10464)

* bpo-34864: warn if "Prefer tabs when opening documents" set to "Always"

* add NEWS entry

* address code review comments

* address second code review comments

* Add entry for idlelib/NEWS.txt.
(cherry picked from commit 9ebe879)

Co-authored-by: Tal Einat <taleinat+github@gmail.com>
miss-islington added a commit that referenced this issue Dec 7, 2018
@taleinat @miss-islington
bpo-34864: warn if "Prefer tabs when opening documents" set to "Alway…
2db190b 
…s" (GH-10464)

* bpo-34864: warn if "Prefer tabs when opening documents" set to "Always"

* add NEWS entry

* address code review comments

* address second code review comments

* Add entry for idlelib/NEWS.txt.
(cherry picked from commit 9ebe879)

Co-authored-by: Tal Einat <taleinat+github@gmail.com>
miss-islington added a commit that referenced this issue Dec 7, 2018
@taleinat @miss-islington
bpo-34864: warn if "Prefer tabs when opening documents" set to "Alway…
1066554 
…s" (GH-10464)

* bpo-34864: warn if "Prefer tabs when opening documents" set to "Always"

* add NEWS entry

* address code review comments

* address second code review comments

* Add entry for idlelib/NEWS.txt.
(cherry picked from commit 9ebe879)

Co-authored-by: Tal Einat <taleinat+github@gmail.com>
vstinner added a commit that referenced this issue Dec 7, 2018
@vstinner
Revert "bpo-34864: warn if "Prefer tabs when opening documents" set t…
72c2cf3 
…o "Always" (#10464)"

This reverts commit 9ebe879.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srinivasreddy srinivasreddy

@ned-deily ned-deily

@terryjreedy terryjreedy

Assignees
No one assigned
Labels
CLA signed OS-mac type-enhancement
Projects
None yet
Milestone
No milestone
7 participants
@taleinat @bedevere-bot @terryjreedy @miss-islington @srinivasreddy @ned-deily @the-knights-who-say-ni