-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
[3.13] gh-124111: Update tkinter for compatibility with Tcl/Tk 9.0.0 (GH-124156) #127364
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
Conversation
…ythonGH-124156) (cherry picked from commit 47cbf03)
The biggest part of this change are tests. |
if isinstance(val, tuple) and len(val) == 0: | ||
return '' |
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'm not sure if this can affect existing code.
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.
This change is a backport from main. If the code doesn't make sense, it should be changed in main first.
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.
Maybe a review from someone who's not a tk expert at all would still be helpful?
Most of the PR is changes to tests, which look reasonable, and should still do their job after this reorganization.
I added comments for the two behaviour changes.
I can't comment on Windows packaging. Unfortunately, @zooba's review on the original PR looks unfinished.
@@ -637,6 +638,7 @@ Tkapp_New(const char *screenName, const char *className, | |||
v->ListType = Tcl_GetObjType("list"); | |||
v->StringType = Tcl_GetObjType("string"); | |||
v->UTF32StringType = Tcl_GetObjType("utf32string"); | |||
v->PixelType = Tcl_GetObjType("pixel"); |
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.
This is NULL
if "pixel" is not registered. I assume that a NULL typePtr
isn't valid, so the later use is OK.
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.
This change is a backport from main. I suggest to enhance the code in the main branch first.
The changes that are there shouldn't impact the Windows builds with Tcl/Tk 8, but without installer changes we aren't able to release with Tcl/Tk 9. IIRC, that wasn't the intent of the original PR (someone wanted to be able to test their own builds), and we can't update 3.13 to Tcl/Tk 9 anyway, so it doesn't particularly matter. |
Downstream distributors and users will want to move Tk 9 if for no other reason than it is new. This is already happening, i.e. with Homebrew for macOS. So it makes sense that we should support Tk 9 in 3.13.x. That said, Tk 9.0.0 was just released 2 months ago and the Tcl/Tk project is working on a 9.0.1 release for the near future so I don't think we need to be in a big hurry to move everything to Tk 9. (I have not yet done any building or testing of tkinter with Tk 9 on macOS.) As far as additional review of the tkinter changes, @serhiy-storchaka wrote most of the existing tkinter tests and made most of the changes to tkinter in recent years so would be my first recommendation for additional review. I don't know that we have had any substantial feedback from 3.14 alpha users who might be using Tk 9 in their own builds. FWIW, the original 3.14 PR was submitted by the Tk core developer who primarily supports Tk on macOS. |
Just to be clear, I do not think we should consider making the 3.13 installers use Tk 9. The question is really whether the non-test changes necessary to make it possible to use Tk 9 are simple and non-invasive enough to backport it to 3.13. |
I have doubts about handling the pixel type. This is a new feature, and there may be better representation of it than str in Python. You can merge backports now if this is needed to fix test failures, but I am planning to revert some changes and replace them with less invasive test-only changes. |
I'm sorry but I don't understand well the feedback on this change. Do you want to change to land into 3.13? If not, why not? Should I remove the PCbuild/ changes? |
I'm not worried about them. As I think I said elsewhere, it's enough to build and test, but not enough to generate our installers with the updated files (and I'd backport those changes if/when we do them if someone wants them). |
Since nobody seems to be eager to review/approve this PR, I plan to abandon it. |
I abandon my PR because of the lack of interest to get it merged. |
Simple backport from main to 3.13 branch with no merge conflict.
(cherry picked from commit 47cbf03)