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

Fix input multibyte characters #183

Closed

Conversation

siefca
Copy link
Contributor

@siefca siefca commented Nov 23, 2013

@siefca
Copy link
Contributor Author

siefca commented Nov 23, 2013

Wait, I'm moving some portions to other files and refactoring CharCode interface a bit.

def form_driver_w form, status, c
form_driver form, c
end
module_function :form_driver_w
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a:

warn "Your Ncursesw does not have a form_driver_w function (wide char aware), non-ASCII chars may not work on your system."

here I think (so that it is shown at start up).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@gauteh
Copy link
Member

gauteh commented Nov 23, 2013

Nice, we need a test which enters stuff using form_driver_w and checks whether the buffer that is returned is sane.

"non-ASCII chars may not work on your system."
warn msg
print msg
sleep 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only warn (most users won't have a form_driver_w), if you set SUP_LOG_LEVEL=debug output is flushed when logging functions are used and it will be displayed immediately, print should not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Done.

@siefca
Copy link
Contributor Author

siefca commented Nov 24, 2013

Ok, changes for #180 seem quite usable.
AFAIR we need some Asian environments to test ltr and charsets.

@gauteh
Copy link
Member

gauteh commented Nov 24, 2013

Yes, definitely. CJK letters should all be covered by UTF-8, but we might have problems with input in other CJK-charsets. We should, however, be able to work internally in UTF-8.

It appears that Ncurses (and Sup) doesn't really support RTL at all.. and users have relied on terminals that have swapped text for them. Anyway, we use REQ_NEXT_CHAR in form_driver_w and not REQ_RIGHT_CHAR.

@siefca
Copy link
Contributor Author

siefca commented Nov 24, 2013

It's ugly but it's working. Is it working on your system?

@gauteh
Copy link
Member

gauteh commented Nov 24, 2013

Testing with patched ncurses, it works, partially 😬 when multiple sigwinches happen the window is redrawn for every sigwinch. Have you checked with the existing code? It used to handle that...

(I know, you have discovered some parts of Sup that were not supposed to see daylight again)

@siefca
Copy link
Contributor Author

siefca commented Nov 24, 2013

How about now?

I placed it (ungetch) within a mutex synchronizer with condition. From what I saw using other console software (mc, vim), the multiple refreshes are happening sometimes too; to avoid that totally we should implement some delay in sigwinch_happened! just after it sets the flag.

@gauteh
Copy link
Member

gauteh commented Nov 24, 2013

Pretty good, sometimes a few refreshes happen, but I can't remember if it was any different before :) (also; #111 might be of interest)

@gauteh
Copy link
Member

gauteh commented Nov 24, 2013

Yup, works on both original ncurses and patched, the slight refreshing is the same as before.

@siefca
Copy link
Contributor Author

siefca commented Nov 24, 2013

Just out of curiosity: where error method comes from?

@gauteh
Copy link
Member

gauteh commented Nov 25, 2013

@gauteh
Copy link
Member

gauteh commented Nov 26, 2013

If, in Sup, I do:

m # new mail
To: asdfadfasdfad
Subject: asdfasdfad <ctrl-c> # abort new mail

Sup (one thread) spins up to 100% CPU usage, seems to work fine otherwise though.

@siefca
Copy link
Contributor Author

siefca commented Nov 26, 2013

Here is the problem with CPU. If there wasn't empty singleton returned by generate there would also be a problem with memory. :]

    def self.get handle_interrupt=true
      begin
        status, code = nonblocking_getwch        # <---- returns [Ncurses::ERR, 0]
        generate code, status
      rescue Interrupt => e
        raise e unless handle_interrupt
        keycode Ncurses::KEY_CANCEL
      end
    end

Ncurses.get_wch returns Ncurses::Err after CTRL+C had been pressed and it returns it continuously.
I'm not sure but the problem might be in ncurses itself or in a wrapper.

It looks like the internal select() or something is not waiting for socket to be ready or some internal flag is never cleared.

[Ncurses::ERR, 0] pair means that there is no data to read, but it behaves like it needed a select wrapper again.

@gauteh
Copy link
Member

gauteh commented Nov 26, 2013

Hm, ok. As far as I can understand, get_wch or getch can never return
ERR. Perhaps we still need the IO.select ?

  • gaute

@siefca
Copy link
Contributor Author

siefca commented Nov 26, 2013

IO.select helps but there is another possibility that something on our level stops sleeping for a while.

This code runs like crazy after CTRL+C is hit:

Redwood::exceptions.nonempty?
[]
  if c.empty?
    []
    next  # loops back since empty

Before CTRL+C is pressed it ticks in some intervals.

@siefca
Copy link
Contributor Author

siefca commented Nov 26, 2013

Non-blocking get_wch returns immediatelly as designed. But why it worked before? Probably because there was select making 0.5 second waits to check if there even is data to be read. Our code does the same thing but is not going to sleep if there is no data (instead it enters the loop that utilizes 100% CPU). I'll try conditional select.

@siefca
Copy link
Contributor Author

siefca commented Nov 26, 2013

Fixed.

@gauteh
Copy link
Member

gauteh commented Nov 26, 2013

Nice catch.

@gauteh
Copy link
Member

gauteh commented Dec 8, 2013

The form_driver_w routine has now been included in the latest ncurses-patch version: http://lists.gnu.org/archive/html/bug-ncurses/2013-12/msg00010.html.

@siefca
Copy link
Contributor Author

siefca commented Dec 8, 2013

😄 Thx Gaute!

@@ -97,39 +101,26 @@ def handle_input c
reset_completion_state
@value = nil

d =
case c
ctrl_c =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does ctrl_c mean here (and below)? Not C-c? Maybe add some comment (and change var name) so that it is understandable for the next guy :) It is pretty messy stuff (kudos for figuring it out)!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll change it since it only has meaning for me (control character) and may mislead others.

gauteh pushed a commit that referenced this pull request Dec 16, 2013
form_driver does not work correctly on all systems (specifically Mac OS X),
it is difficult or impossible to determine whether the input char is a
char or key code resulting from a char sequence in a multibyte char.

If no form_driver_w is present we fall back to form_driver.
form_driver_w is included in ncursesw-1.4.2 with the Ncurses 5.9 20131207
patchlevel.

http://lists.gnu.org/archive/html/bug-ncurses/2013-12/msg00010.html

original patch:

http://lists.gnu.org/archive/html/bug-ncurses/2013-11/msg00015.html

Author: Paweł Wilk <siefca@gnu.org>

Squashed commit of the following:

commit 7a513f2
Author: Paweł Wilk <siefca@gnu.org>
Date:   Tue Nov 26 19:49:30 2013 +0100

    Added conditional select in Ncurses::nonblocking_getwch to prevent erroneous CharCode spawning

commit 0b6859e
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 21:14:52 2013 +0100

    BufferManager.sigwinch_happened! modified to run once if it already happened

commit 857cb60
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 20:44:21 2013 +0100

    Added workaround for screen redrawing when sigwinch happened

commit d72d72f
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 20:43:46 2013 +0100

    Optimizations in Ncurses::CharCode.get

commit c2139b6
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 18:19:57 2013 +0100

    Fixed uninitialized instance variable warning

commit 72fb7e4
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:43:16 2013 +0100

    Added critical patches for Ncurses::Form::DriverHelpers#form_driver and Ncurses.get_wch

commit 340fb7f
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:42:24 2013 +0100

    Ncurses::Form::DriverHelpers improved; now all wrappers are reusing core wrapper

commit ca362c2
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:40:38 2013 +0100

    Ncurses::CharCode#enc_char improved

commit 380079e
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:40:01 2013 +0100

    Added Ncurses::CharCode::Empty.empty override

commit 8da83d9
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:39:28 2013 +0100

    Cleanups

commit de9f7bc
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:39:02 2013 +0100

    Added Ncurses::CharCode.dumb! switch to control behavior when CharCode cannot detect keycodes

commit ffad7e5
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 17:35:30 2013 +0100

    Added Ncurses::CharCode.generate to save objects from spawning when ERR is returned by get(w)ch

commit ad6f6f2
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 03:02:01 2013 +0100

    Removed printing of warning about a missing form_driver_w() function

commit ea277c1
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 02:55:23 2013 +0100

    Cleanups

commit 505532e
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:24:08 2013 +0100

    TextField now uses form_driver helpers from Ncurses::Form::DriverHelpers

commit 9103729
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:23:22 2013 +0100

    Added Form::DriverHelpers module with methods that ease usage of form_driver

commit 05c2b56
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:22:08 2013 +0100

    Improved Ncurses::CharCode#replace method (detects if input is a charcode or a character)

commit a2f0dac
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:21:11 2013 +0100

    Removed previous wrapper for form_driver_w()

commit 3a5cbdc
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:20:12 2013 +0100

    Cleanups

commit a958553
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:19:51 2013 +0100

    Replacement wrapper for form_driver_w() moved to Ncurses::prepare_form_driver and called after initscr

commit 7766cbf
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sun Nov 24 01:17:37 2013 +0100

    Multiple typos fixed

commit 8bf705c
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sat Nov 23 20:58:54 2013 +0100

    Key for continuing search in buffer in Buffer#handle_input is now compared directly

commit 1dcfabd
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sat Nov 23 20:57:22 2013 +0100

    Moved ncurses tweaks to lib/sup/util/ncurses.rb, added CharCode::Empty singleton class

commit ed85b52
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sat Nov 23 17:37:50 2013 +0100

    TextField class is now making use of Ncurses::Form.form_driver_w

commit 9ce6134
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sat Nov 23 17:37:10 2013 +0100

    Cleanups in buffer.rb

commit f8f7116
Author: Paweł Wilk <siefca@gnu.org>
Date:   Sat Nov 23 17:36:49 2013 +0100

    Added compat proxy for the Ncurses::Form.form_driver_w module method

commit 171eada
Author: Paweł Wilk <siefca@gnu.org>
Date:   Fri Nov 22 02:36:27 2013 +0100

    Fixed TextField#handle_input (keysyms taken into account) and prepared for form_driver workaroud

commit 3217a65
Author: Paweł Wilk <siefca@gnu.org>
Date:   Fri Nov 22 02:32:28 2013 +0100

    Added Ncurses::curyx module method (helper for reading whole cursor position)

commit 60b02d6
Author: Paweł Wilk <siefca@gnu.org>
Date:   Thu Nov 21 19:08:51 2013 +0100

    Ncurses::CharCode::get now uses chr to create character based on code returned by get_wch

commit 58088c4
Author: Paweł Wilk <siefca@gnu.org>
Date:   Thu Nov 21 17:55:45 2013 +0100

    Typo fixed

commit ca8e984
Author: Paweł Wilk <siefca@gnu.org>
Date:   Thu Nov 21 17:21:49 2013 +0100

    Ncurses::CharCode class refactored, code adapted to use its instances

    Details:

    - changed Ncurses::CharCode to work with wide characters,
    - removed select wrapper when reading input,
    - moved input reading into Ncurses::CharCode class,
    - implemented passing Ncurses::CharCode objects as far as possible.

commit f0168be
Author: Paweł Wilk <siefca@gnu.org>
Date:   Tue Nov 19 23:20:13 2013 +0100

    Adapted Keymap, Buffer and TextField classes to make use of CharCode objects

commit dfea936
Author: Paweł Wilk <siefca@gnu.org>
Date:   Tue Nov 19 23:17:11 2013 +0100

    Added Ncurses::CharCode helper class for storing and transferring wide characters
@gauteh
Copy link
Member

gauteh commented Dec 16, 2013

Merged.

@gauteh gauteh closed this Dec 16, 2013
eMBee added a commit to eMBee/sup that referenced this pull request Feb 14, 2015
eMBee added a commit to eMBee/sup that referenced this pull request Feb 14, 2015
eMBee added a commit to eMBee/sup that referenced this pull request Feb 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants