-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-66136: IDLE: Fix use of unsaved indicator in titlebar of shell window #3701
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
base: main
Are you sure you want to change the base?
Conversation
In general, patches should have a specification posted on the issue that detail what the patch is intended to do. In this case, how IDLE should behave after the patch. |
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.
Given that EditorWindows and Outwins use .. to indicate 'changed since last save', the derived Shell window must be doing something different to use ... differently. The request on the issue is to remove that difference. So I expect the patch to remove whatever makes Shell act differently, not add more difference on top of the existing difference.
That said, the patch seems to work, but I don't know why. Can you explain a bit?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I just wrote a really long description and lost it. :-( |
So, here's the shorter description. The difference between shell and editor windows is that the shell clears the undo/redo every time enter is pressed. So, you can only undo/redo on the current prompt line and not anything prior to that. In editor, even after Save, you can undo/redo as much as you want (up to the stored limit). The change I made clarified this difference and didn't add more of a difference. In Going back to the difference between shell and editor, that functionality is added in My patch simply sends a flag from Note: |
I will probably have to read that a couple of times while looking at the code, but I think it will be enough. The short story is that the handling of undo in Shell is so different that the dirty marking cannot operate the same. Aside: I wonder if clearing undo in the Shell could be delayed until the statement is submitted. |
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.
After looking into the code, I think the approach here is overly complex. Why not do what IOBinding.loadfile()
does, i.e. just call set_saved(False)
after calling reset_undo()
?
I also think that the lengthy doc-strings added aren't a good idea. They describe in great detail how the methods work, which should be irrelevant to the callers of those methods, and is prone to get out of sync with their code.
If you're willing to rework this a bit, I'd be happy to review again so we can get this merged.
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 suggested much shorter docstring additions. I have not yet re-reviewed in light of Tal's implementation suggestion.
status for the window based on the parameter. If set_saved_flag | ||
is True, then the window will look like it's been saved. If | ||
it's false, it will not be flagged as saved. | ||
""" |
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 believe replacing the 4 lines with "If set_saved_flag, clear the unsaved markers on the title bar." gives the essential additional information needed.
self.interp.restart_subprocess(with_cwd=True) | ||
|
||
def showprompt(self): | ||
"""Display the prompt in the shell window. |
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.
"""Display the prompt in the shell window. | |
"Display the prompt." |
and delete rest. I will soon propose changing handing of prompt.
This PR is stale because it has been open for 30 days with no activity. |
This PR is stale because it has been open for 30 days with no activity. |
This PR is stale because it has been open for 30 days with no activity. |
This PR is stale because it has been open for 30 days with no activity. |
In IDLE, the title in an editor or shell window is bookended by asterisks when changes are made to the text, indicating that the file needs to be saved. However, the shell window was adding the asterisks as text was typed, but upon pressing the enter key, the asterisks disappeared. This patch retains the asterisks after enter is pressed.
https://bugs.python.org/issue21937