TCOD_sys_clipboard_get segfaults on Mac

Issue #81 open
HexDecimal
created an issue

Discovered this when I finally got my libtcod-cffi tests to run on Mac. This doesn't seem Python related.

At a quick glance it looks like it might be easy to refactor libtcod's clipboard implementation to use SDL2's new clipboard functions.

Comments (29)

  1. RMTEW FULL NAME

    I've reworked the clipboard support to use the SDL2 functions, and also added the clipboard functons to libtcodpy.

    Commit: 2b922fa https://bitbucket.org/libtcod/libtcod/commits/2b922fa3fd1e49bf5eae83d18abad8daee29451c

    I tested it with the following change to the tutorial code (tested on Windows only):

    # .... 8< .....
    import time
    # .... >8 .....
    .....
    elif key.vk == libtcod.KEY_ESCAPE:
        return 'exit'  #exit game
    .....
    # .... 8< .....
    if chr(key.c) == "c" and key.lctrl:
        ts = time.ctime()
        libtcod.sys_clipboard_set(ts)
        print "copying time", ts
    elif chr(key.c) == "v" and key.lctrl:
        ts = libtcod.sys_clipboard_get()
        print "pasting time", ts
    # .... >8 .....
    .....
    if game_state == 'playing':
    .....
    

    Look okay?

  2. HexDecimal reporter

    The current commit has a few issues:

    • I can no longer access the clipboard without initializing the libtcod window.
    • SDL_GetClipboardText is called without later calling SDL_Free, this will cause a memory leak.
    • libtcodpy isn't handling utf-8 string conversion.

    The Python string conversion is best handled like this:

    def sys_clipboard_set(text):
        return _lib.TCOD_sys_clipboard_set(text.encode('utf-8'))
    
    def sys_clipboard_get():
        return _lib.TCOD_sys_clipboard_get().decode('utf-8')
    

    Your tutorial code is using print as a statement which will break when run on Python 3. This code is more portable: print("clipboard is %r" % ts)

  3. RMTEW FULL NAME

    The tutorial code is irrelevant as it is just for illustration. The tutorial itself is Python 2, in any case.

    I've exposed TCOD_sys_startup() as sys_startup() in Python. It needs to be called to access the clipboard or various other additional functions without the window.

    I am not sure about the utf-8 in the libtcodpy file. This is Python 3 specific, and at the moment, while libtcodpy should be compatible with both 2 and 3, .. this needs to be clarified. There's already some special handling, but it;s been a while since I looked into it.

    Check out afba132 for the SDL_free call.

  4. HexDecimal reporter

    If there are no negative side effects of TCOD_sys_startup then it should be called during the import step of libtcodpy rather than exposing it.

    The code I gave works across Python versions. This article should clear up any confusion about Python and Unicode: http://nedbatchelder.com/text/unipain.html

    libtcod itself doesn't handle Unicode very well. wchar's width depends on what compiler you're using and still has to be handled like it's UTF rather than accepting the data as is. The common approach in C is to always work with UTF-8, like what SDL is doing. There are more problems, so maybe I'll raise an issue explaining them in more detail.

    The new get_clipboard_text code looks good.

  5. RMTEW FULL NAME

    There's this:

    / case DLL_PROCESS_ATTACH : TCOD_sys_startup(); break; -- not safe, locks up in SDL2/RegisterClass call /

    Uncommenting it on Windows, locks up in the timer subsystem where it does some threading.

    However, we don't seem to use the SDL AddTimer/RemoveTimer functions, which I assume are the extent of the timer subsystem. Removing the flag SDL_INIT_TIMER from SDL_Init seems to work fine on Windows.

    If you can confirm that doing the same works for you, then maybe we can check it in.

  6. HexDecimal reporter

    I can confirm that DllMain works on Windows with SDL_INIT_TIMER removed. I ran my libtcod-cffi unit tests with DllMain enabled on Mac and Linux and they've passed too.

    Closing the root console with TCOD_console_delete uninitialized SDL's video subsystem needed by the clipboard, when I really only wanted to close the window. I assume this is a minor issue. I'm just noting it.

  7. RMTEW FULL NAME

    Lol. I've just finished working on a refactoring myself, a little more extensive than your own.

    I was just writing the commit command when your email arrived. I'll add your atexit change and test it locally, and then commit.

  8. RMTEW FULL NAME

    If we can't get consistent behaviour on all platforms with sdl, we might have to keep the old code and use it when the window is not open for those platforms (assuming it's not mac).

    I get:

    got from clipboard 1: ['sys_clipboard_get', 'sys_clipboard_get', 'sys_clipboard_get']
    24 bits font.
    key color : 0 0 0
    24bits greyscale font. converting to 32bits
    wait:
    got from clipboard 2: ['sys_clipboard_get', 'sys_clipboard_get', 'sys_clipboard_get']
    wait:
    got from clipboard 3: ['', '', '']
    

    From the following:

    import libtcodpy as libtcod
    
    l = []
    for i in range(3):
        l.append(libtcod.sys_clipboard_get())
    print "got from clipboard 1:", l
    
    libtcod.console_set_custom_font('arial10x10.png', libtcod.FONT_TYPE_GREYSCALE | libtcod.FONT_LAYOUT_TCOD)
    libtcod.console_init_root(20, 20, 'python/libtcod tutorial', False)
    raw_input("wait:")
    libtcod.console_delete(None)
    
    l = []
    for i in range(3):
        l.append(libtcod.sys_clipboard_get())
    print "got from clipboard 2:", l
    raw_input("wait:")
    
    libtcod.sys_shutdown()
    
    l = []
    for i in range(3):
        l.append(libtcod.sys_clipboard_get())
    print "got from clipboard 3:", l
    

    So, SDL on Windows seems to just return an empty string after full shutdown.

  9. RMTEW FULL NAME

    Well, it turns out the person who tested on Linux encountered a known SDL2 limitation to do with clipboards.

    https://bugzilla.libsdl.org/show_bug.cgi?id=3222

    I've asked chaosdev to retry with the old support. It may be that we can get away with the SDL clipboard support on Windows and MacOS and on Linux we stick with the old clipboard support (which seems to be a lot simpler than SDL2's Linux clipboard support). Or it may be that we have to enforce no clipboard access outside of root window presence.

  10. HexDecimal reporter

    I was able to test your code sample on MacOS:

    clipboard1: 222
    clipboard2: [u'222', u'222', u'222']
    clipboard3: [u'', u'', u'']
    

    I also tested with a Unicode symbol, which works correctly:

    clipboard2: [u'\u2603', u'\u2603', u'\u2603']
    
  11. Brian Bruggeman

    I also have MacOS and I have run these changes successfully; they were not successful previous to the changes.

    FWIW, it's probably relatively easy to setup a travis-ci build to test out linux in an automated fashion (similar to the appveyor). I've only setup travis on github, but I'm assuming that Atlassian provides the hooks as well. See: https://travis-ci.org/ Often, a Mac build tend to work if the Linux build does. So if there was an automated check, you might have more confidence when you run.

    Additionally, it shouldn't be too hard to setup a local vagrant box for testing linux if you want to go that route.

  12. RMTEW FULL NAME

    Yes, it's probably easy to set up lots of things lol. But libtcod is at best tenth on my list of priorities, and the only way to get things done is to do known achievables in small time allotments. So, things get done how they have to get done.

    If someone wants to work out how to set up travis testing, then by all means. Otherwise, it's going to have to wait.

  13. RMTEW FULL NAME

    Will travis test with an X server present so things like the clipboard functionality can be tested? Probably not. We'll still have to outsource testing, or set up a vm to do those sorts of things.

    If you want to get travis working on a forked repository, and send a pull request, is that something you'd be interested in helping with?

  14. Brian Bruggeman

    Will travis test with an X server present so things like the clipboard functionality can be tested?

    Yes. That's at least partially why I mentioned Travis. You can setup a headless version for testing things.

    If you want to get travis working on a forked repository, and send a pull request, is that something you'd be interested in helping with?

    Definitely.

    or set up a vm to do those sorts of things

    Is a vm something you'd naturally use in your workflow? Have you used vagrant previously?

  15. Log in to comment