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

Timeout feature for keys #645

Merged
merged 1 commit into from Mar 11, 2017

Conversation

Projects
None yet
4 participants
@LemonBoy
Member

LemonBoy commented Feb 10, 2017

The timeout can now be configured with the combo_timeout option with millisecond granularity, any value that's less than one disables the timeout.
The behaviour is now similar to the one vim exhibits, if the keys in the combo queue are bound to something then they're happily processed.

@madduck are you happy now? :)

@dequis

This comment has been minimized.

Member

dequis commented Feb 10, 2017

The lack of timeout was a good thing for some sorts of keybindings that are sequences of separate keystrokes. At least default it to 0 and disabled. I don't think the nitpicking that "one keystroke is dropped" is worth making it harder to use sequences of keybindings for the rest of the world.

@madduck

This comment has been minimized.

madduck commented Feb 10, 2017

@LemonBoy this is awesome, and it works pretty exactly as expected. I am with @dequis though in that I think that the timeout should be configurable and probably default to 0, although I don't think I was nitpicking before. ;)

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Feb 15, 2017

didn't follow your discussion, can you give some examples how to use this feature?

@madduck

This comment has been minimized.

madduck commented Feb 15, 2017

@ailin-nemui it's related to multi-key bindings, say meta-a-meta-d. Once you pressed meta-a, there's no way to get rid of it, other than to complete the sequence. Other UIs that have multi-key bindings simply timeout if the sequence isn't completed and/or provide a way to cancel (break) the combo. This is what @LemonBoy's patch adds.

(though I am unsure how one would successfully bind break_combo to a key like escape as it'd be a bit of a catch-22 to break an ongoing combination with a new keybindings?)

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Feb 17, 2017

I think event tags should default to 0, and aren't they unsigned as well

@dequis

This comment has been minimized.

Member

dequis commented Feb 17, 2017

$ ag g_source_remove -B1 | grep '!=' -A1 | grep -e '!=' -e g_source_remove
src/core/pidwait.c:52-  if (id != NULL) {
src/core/pidwait.c:53:          g_source_remove(GPOINTER_TO_INT(id));
src/core/net-sendbuffer.c:47:        if (rec->send_tag != -1) g_source_remove(rec->send_tag);
src/core/write-buffer.c:150-    } else if (timeout_tag != -1) {
src/core/write-buffer.c:151:            g_source_remove(timeout_tag);
src/core/write-buffer.c:180-    if (timeout_tag != -1)
src/core/write-buffer.c:181:            g_source_remove(timeout_tag);
src/core/servers.c:49-  if (server->connect_tag != -1) {
src/core/servers.c:50:          g_source_remove(server->connect_tag);
src/core/servers.c:183-         if (server->connect_tag != -1)
src/core/servers.c:184:                 g_source_remove(server->connect_tag);
src/core/servers.c:193- if (server->connect_tag != -1) {
src/core/servers.c:194:         g_source_remove(server->connect_tag);
src/fe-common/core/fe-queries.c:350-    else if (query_auto_close <= 0 && queryclose_tag != -1) {
src/fe-common/core/fe-queries.c:351:            g_source_remove(queryclose_tag);
src/fe-common/core/fe-queries.c:384:    if (queryclose_tag != -1) g_source_remove(queryclose_tag);
src/fe-common/core/fe-exec.c:182-       if (rec->read_tag != -1)
src/fe-common/core/fe-exec.c:183:               g_source_remove(rec->read_tag);
src/fe-common/core/fe-windows.c:802-    if (daytag != -1) {
src/fe-common/core/fe-windows.c:803:            g_source_remove(daytag);
src/fe-common/core/fe-windows.c:831:    if (daytag != -1) g_source_remove(daytag);
src/fe-common/irc/fe-modes.c:224-       if (mode_tag != -1)
src/fe-common/irc/fe-modes.c:225:               g_source_remove(mode_tag);
src/fe-common/irc/fe-netjoin.c:489-     if (join_tag != -1) {
src/fe-common/irc/fe-netjoin.c:490:             g_source_remove(join_tag);
src/fe-common/irc/fe-netsplit.c:378-    if (split_tag != -1) {
src/fe-common/irc/fe-netsplit.c:379:            g_source_remove(split_tag);
src/fe-text/gui-readline.c:811-                 if (paste_timeout_id != -1)
src/fe-text/gui-readline.c:812:                         g_source_remove(paste_timeout_id);
src/irc/dcc/dcc.c:111-  if (dcc->handle != NULL) net_disconnect(dcc->handle);
src/irc/dcc/dcc.c:112:  if (dcc->tagconn != -1) g_source_remove(dcc->tagconn);
src/irc/dcc/dcc.c:113:  if (dcc->tagread != -1) g_source_remove(dcc->tagread);
src/irc/dcc/dcc.c:114:  if (dcc->tagwrite != -1) g_source_remove(dcc->tagwrite);
src/irc/flood/flood.c:308-      } else if (flood_tag != -1) {
src/irc/flood/flood.c:309:              g_source_remove(flood_tag);
src/irc/flood/flood.c:338-      if (flood_tag != -1) {
src/irc/flood/flood.c:339:              g_source_remove(flood_tag);
src/irc/notifylist/notify-ison.c:321:   if (notify_tag != -1) g_source_remove(notify_tag);
src/irc/core/irc-servers.c:1054-        if (cmd_tag != -1)
src/irc/core/irc-servers.c:1055:                g_source_remove(cmd_tag);
src/irc/core/sasl.c:81- if (server->sasl_timeout != 0) {
src/irc/core/sasl.c:82:         g_source_remove(server->sasl_timeout);
src/irc/core/sasl.c:100-        if (server->sasl_timeout != 0) {
src/irc/core/sasl.c:101:                g_source_remove(server->sasl_timeout);
src/irc/core/sasl.c:115-        if (server->sasl_timeout != 0) {
src/irc/core/sasl.c:116:                g_source_remove(server->sasl_timeout);
src/irc/core/sasl.c:278-        if (server->sasl_timeout != 0) {
src/irc/core/sasl.c:279:                g_source_remove(server->sasl_timeout);
src/irc/core/sasl.c:305-        if (server->sasl_timeout != 0) {
src/irc/core/sasl.c:306:                g_source_remove(server->sasl_timeout);

Not sure why we've done != -1 historically. I think I've seen it in other codebases too. But yeah they are supposed to be unsigned.

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Feb 18, 2017

No reason but that I copy-pasted the block from another file. The documentation for g_timeout_add states that the return value is greater than zero so we're fine with that as a sentinel value.
I'll update the PR accordingly as soon as possible.

@ailin-nemui ailin-nemui changed the title from Miscellaneous improvements for the key handling to Timeout feature for keys Mar 7, 2017

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 11, 2017

@madduck are you using this patch and can vouch for it?

@madduck

This comment has been minimized.

madduck commented Mar 11, 2017

@ailin-nemui I've used it for a while yes, but then went back to running stock Debian irssi. Not sure what you mean with "vouch". It does what it should, indeed.

@ailin-nemui ailin-nemui merged commit e2e0216 into irssi:master Mar 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dequis dequis removed the needs review label Mar 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment