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

tkinter: integrate TkDND support #85070

Open
E-Paine mannequin opened this issue Jun 6, 2020 · 9 comments
Open

tkinter: integrate TkDND support #85070

E-Paine mannequin opened this issue Jun 6, 2020 · 9 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir topic-tkinter type-feature A feature request or enhancement

Comments

@E-Paine
Copy link
Mannequin

E-Paine mannequin commented Jun 6, 2020

BPO 40893
Nosy @terryjreedy, @ned-deily, @zware, @serhiy-storchaka, @zooba, @E-Paine
PRs
  • bpo-40893: Add TkDND support to tkinter #20896
  • Files
  • tkdnd.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2020-06-06.20:09:50.629>
    labels = ['expert-tkinter', 'type-feature', 'library', '3.10']
    title = 'tkinter: integrate TkDND support'
    updated_at = <Date 2020-06-30.17:08:45.130>
    user = 'https://github.com/E-Paine'

    bugs.python.org fields:

    activity = <Date 2020-06-30.17:08:45.130>
    actor = 'epaine'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)', 'Tkinter']
    creation = <Date 2020-06-06.20:09:50.629>
    creator = 'epaine'
    dependencies = []
    files = ['49217']
    hgrepos = []
    issue_num = 40893
    keywords = ['patch']
    message_count = 9.0
    messages = ['370852', '371445', '371554', '371577', '371593', '372110', '372111', '372421', '372705']
    nosy_count = 7.0
    nosy_names = ['terry.reedy', 'gpolo', 'ned.deily', 'zach.ware', 'serhiy.storchaka', 'steve.dower', 'epaine']
    pr_nums = ['20896']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue40893'
    versions = ['Python 3.10']

    @E-Paine
    Copy link
    Mannequin Author

    E-Paine mannequin commented Jun 6, 2020

    For years, the Python docs for the tkinter.dnd module (and prior Tkdnd module) have said that it will become deprecated once it has been replaced by TkDND bindings (I can find it back in the Python 2.2 docs – https://docs.python.org/2.2/lib/node508.html). Despite this, I cannot find anywhere that the changes are actually proposed (have I missed something?!).

    Attached is a git diff of a draft of proposed changes for the tkinter library, tkinter docs and Windows installer. The changes to the tkinter library are designed so that TkDND is not required where tcl/tk is installed, and in particular I check before any call is made to the library that it has been successfully loaded so as to give the user a more helpful error message.

    I don’t feel it is ready to be a PR yet, unless that would be helpful, as I have not been able to extensively test on MacOS (I think I have sufficiently tested on Windows and Linux, though someone is bound to find a problem!). I also don’t feel it is ready for a PR because I would really appreciate if someone could take a serious look at the changes to the installer. I have got it working but I don’t really understand how the installer works and it is quite likely that the TkDND externals should have their own group and bindpath.

    On a similar note to the Windows installer, how do we declare to the Linux package maintainers that TkDND is now an optional dependency of Python? I cannot find a file where optional dependencies are mentioned, so is there some mechanism to notify them of the changes?

    I want to give credit where it is due and so it should be noted that new docs are very nearly just a “translation” of the TkDND man page and that the changes in the tkinter library are (loosely) based on the TkinterDND bindings.

    I also have the relevant files for the cpython-source-deps and cpython-bin-deps repos, but would need someone with write permissions to create a ‘tkdnd’ branch in each of those repos before I create the PRs for them.

    I am very new to contributing and so have a few questions on what is expected of me:

    1. What is required of a news entry, and where would it go (I am assuming Misc\NEWS.d\next\Library)?
    2. Could such a change, though not particularly large for code, be considered a “major new feature” (which would require a PEP)?

    If people don’t generally want to add TkDND bindings to the tkinter library, I would instead propose that we remove the note that the tkinter.dnd module will be deprecated (but leave the note about it being experimental).

    @E-Paine E-Paine mannequin added type-feature A feature request or enhancement topic-tkinter 3.10 only security fixes labels Jun 6, 2020
    @terryjreedy
    Copy link
    Member

    Easiest is to install 'blurb' and use that to write news entries. See
    https://docs.python.org/3/whatsnew/changelog.html#changelog
    for examples. The coredev who merges may changes whatever you propose.

    I don't think that this needs a PEP, unless Serhiy decides otherwise. I don't think that there are very many details to thrash out.

    @zooba
    Copy link
    Member

    zooba commented Jun 15, 2020

    Is there any reason for TkDND to be optional apart from the rest of Tkinter? If not, your installer changes are probably fine, but why not combine the two and just treat it as part of the Tcl/Tk build? (Primarily for Windows, I'd guess. Not sure how that'd work for macOS.)

    Submitting a PR helps by running checks, and you can mark it as draft to prevent anyone from merging it. Though since it needs to pull down prebuilt binaries, it's not going to work until we sort out how to build the separate pieces.

    @E-Paine
    Copy link
    Mannequin Author

    E-Paine mannequin commented Jun 15, 2020

    Terry:

    Thank you, I have updated my local version with a short descriptor and am relieved that I probably don't need to write a PEP as I am not known for my writing skills!

    Steve:

    Also thank you. I cannot think of a reason for it to be optional on Windows, however I think it would be best for it to remain a separate external (though, for obvious reasons, you should get the final say). There are two benefits, I think, that being a separate external brings:

    1. Easier to update - I guess this depends on your experience of Tix (as an example), but for maintainability, I think it would be easier to update TkDND separate to updating tcl/tk.

    2. Old Python builds don't included unneeded binaries - For older python versions, for which the changes to the tkinter library are not going to be backported (3.6 - 3.9), it seems pointless to package the TkDND binaries.

    3. Easy to make optional in future - Less important, but if we change our mind about making TkDND optional (as an installer build flag, etc.), the change would be made much easier (whether that be during this issue/PR or as another issue).

    I will go about creating a PR for this, though if I am feeling particularly incompetent, may need you to set it as a draft!

    Ned:

    What changes would be required to the MacOS installer to declare TkDND as an optional dependency (I really don't have a clue how MacOS works so would really appreciate some help)? I presume, based on the READMEs that it doesn't use the source- & bin-deps repos, but it also doesn't have a package manager like Linux distros to which to specify the optional dependencies.

    Anyone!

    Could the person responsible for the Linux builds please be added to the nosy, so that I can ask the question about notifying the package maintainers for each of the distros. I would also like to ask if TkDND could be declared as an optional dependency, as many distros do not have it in their main repos (taking Arch Linux as an example, the TkDND package is in the AUR, while tk is in the "extra" repo).

    @ned-deily
    Copy link
    Member

    What changes would be required to the MacOS installer to declare TkDND ...

    Let's get Windows settled first and then I will look at adding it to the macOS installer.

    Regarding other platforms: we don't provide binaries for anything other than our own Windows and macOS installers. So I think there are a dew issues here:

    1. What to do about third-party distributors of Python? (That includes macOS and Windows).
      The best we can do, probably, is to make sure the recommended dependency
      is mentioned in the What's New in 3.10 document. There may be some
      appropriate mailing lists or groups to discuss further; I vaguely recall
      there being one someone that was focused on distributors of Python.
      But there will be a *lot* of variability here since not all Python
      distributors also distribute their own version of Tcl/Tk for use
      with Python.

    2. We would also want to make sure that our buildbot fleet is updated to
      include TkDND for those buildbots that attempt to do gui tests.

    @codin-math codin-math mannequin added stdlib Python modules in the Lib dir and removed topic-tkinter 3.10 only security fixes type-feature A feature request or enhancement labels Jun 16, 2020
    @codin-math codin-math mannequin changed the title tkinter integrate TkDND support None Jun 16, 2020
    @terryjreedy terryjreedy added 3.10 only security fixes topic-tkinter labels Jun 16, 2020
    @terryjreedy terryjreedy changed the title None tkinter: integrate TkDND support Jun 16, 2020
    @terryjreedy terryjreedy added the type-feature A feature request or enhancement label Jun 16, 2020
    @E-Paine
    Copy link
    Mannequin Author

    E-Paine mannequin commented Jun 22, 2020

    Addressing Ned's issues:

    I have emailed round "Linux-sig" about adding an optional dependency and Guido recommended I put it on "Python-dev" instead (which I hope to do in the coming days). As for the buildbots, is this something I could help with, or does it just require someone with permission to remote-logon and change as necessary?

    As a side-note, I currently looking at better ways of getting binding returns to TkDND as the current method (9a27b14#diff-3eeb1d6bdd82965287a553385c879693R1449) is not that clear ('dndrtn' does nothing - the set method simply returns the variable contents without trying to evaluate it).

    @ned-deily
    Copy link
    Member

    As for the buildbots, is this something I could help with

    @zach.ware is the best person to work with on any buildbot requirements. I've Nosyed him here.

    For the record, I haven't yet tried to test TkDND with the various current macOS Tk on current versions of macOS; there are a lot of variables there and a lot of ways it may have issues. I hope to take a first look soon.

    @E-Paine
    Copy link
    Mannequin Author

    E-Paine mannequin commented Jun 26, 2020

    I have now marked the PR ready for review.

    Terry:

    Could you please take a quick look to double-check that the changes proposed are fully backwards-compatible? I have run the IDLE tests and have tried to pay particular attention to this problem when developing, but would appreciate a second opinion (to ensure we won't break existing tkinter scripts - such as IDLE).

    Serhiy:

    I would really appreciate your opinion on the patch, particularly the change to the _bind method. I am also concerned about the getlist_event method as it can split dropped text data (for example) but I don't know how to fix it (unless we give the user the raw data for them to fix it?).

    @E-Paine
    Copy link
    Mannequin Author

    E-Paine mannequin commented Jun 30, 2020

    I think I have fixed the problem with the 'data' attribute (and related getlist_event method) by exposing two event data attributes. One of these is a 'data_raw' attribute intended for use with text-based drops and the other is a 'data_split' intended for file drops (see the new docs for details).

    My original idea was to expose the 'tkapp.splitlist' method and just give them the content of what is now the 'data_raw' attribute. However,
    I decided that should be avoided and so settled on the current implementation of two new attributes.

    Thanks everyone for the effort you have already put into this and I know that there is a lot added in this PR but I would really appreciate some reviews to get it to a more commit-ready state (there is inevitably lots to be improved - I also apologise for saying 'appreciate' in almost every message :-).

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir topic-tkinter type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants