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

$fish_escape_delay_ms not working as a universal variable #4196

Closed
InspectorMustache opened this Issue Jul 7, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@InspectorMustache

InspectorMustache commented Jul 7, 2017

I have fish_escape_delay_ms set to 10 as a universal variable. However, after upgrading to Fish 2.6.0, it seems that setting isn't honored anymore. echo $fish_escape_delay_ms still gives me 10 but the delay is noticeably longer. If I enter set fish_escape_delay_ms for each new shell instance it works fine.

I'm on ArchLinux (4.11.7) using Fish with Termite.

@krader1961 krader1961 self-assigned this Jul 7, 2017

@krader1961 krader1961 added this to the fish 2.7.0 milestone Jul 7, 2017

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 7, 2017

Contributor

I don't think setting it as a uvar ever worked but it should. The fix is trivial: call update_wait_on_escape_ms() from misc_init() so it is honored at startup.

Contributor

krader1961 commented Jul 7, 2017

I don't think setting it as a uvar ever worked but it should. The fix is trivial: call update_wait_on_escape_ms() from misc_init() so it is honored at startup.

@InspectorMustache

This comment has been minimized.

Show comment
Hide comment
@InspectorMustache

InspectorMustache Jul 7, 2017

Hmmm… no idea, why I'm only seeing this issue with 2.6 then. Anyway, thank you very much for putting this in 2.7.0. In the meantime, it should be very easy to work around.

InspectorMustache commented Jul 7, 2017

Hmmm… no idea, why I'm only seeing this issue with 2.6 then. Anyway, thank you very much for putting this in 2.7.0. In the meantime, it should be very easy to work around.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 7, 2017

Contributor

Turns out that this did work prior to commit 6d02bec. That change included suppressing reacting to variable changes until the environment subsystem was initialized. I'll need to ponder how to get the react_to_variable_change() function to run when loading uvars.

Contributor

krader1961 commented Jul 7, 2017

Turns out that this did work prior to commit 6d02bec. That change included suppressing reacting to variable changes until the environment subsystem was initialized. I'll need to ponder how to get the react_to_variable_change() function to run when loading uvars.

@krader1961 krader1961 added bug regression and removed enhancement labels Jul 14, 2017

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 15, 2017

Contributor

This change in behavior is because prior to commit 6d02bec what was then called the input_init() function (now named init_input()) was called as a side-effect of actually reading commands to run. That is, after 99% of the fish initialization logic had run. That function called input_common_init() which explicitly calls update_wait_on_escape_ms(). All of that happened after the universal variables had been loaded because it was executed lazily rather then explicitly. In other words, too much spooky action at a distance behavior that wasn't obvious when commit 6d02bec was merged was the reason this used to work.

The post commit 6d02bec code explicitly initializes the input subsystem as part of initializing the run-time environment before the uvars are loaded. So the the reason this magic uvar doesn't have any effect is because input_common_init() is no longer run after uvars are loaded.

Note that this exposes more serious problems with the initial loading of the universal variables. While investigating this problem it became obvious that no callback events were performed during the initial loading of the uvars. The only reason that fish_escape_delay_ms happened to work as a uvar was because it was explicitly special-cased. Any other variable handled by react_to_variable_change() is not handled during the initial loading of uvars. Note that env_universal_t::load() discard the callbacks that result from loading the uvars. Which is itself a big red flag that the logic isn't correct.

Contributor

krader1961 commented Jul 15, 2017

This change in behavior is because prior to commit 6d02bec what was then called the input_init() function (now named init_input()) was called as a side-effect of actually reading commands to run. That is, after 99% of the fish initialization logic had run. That function called input_common_init() which explicitly calls update_wait_on_escape_ms(). All of that happened after the universal variables had been loaded because it was executed lazily rather then explicitly. In other words, too much spooky action at a distance behavior that wasn't obvious when commit 6d02bec was merged was the reason this used to work.

The post commit 6d02bec code explicitly initializes the input subsystem as part of initializing the run-time environment before the uvars are loaded. So the the reason this magic uvar doesn't have any effect is because input_common_init() is no longer run after uvars are loaded.

Note that this exposes more serious problems with the initial loading of the universal variables. While investigating this problem it became obvious that no callback events were performed during the initial loading of the uvars. The only reason that fish_escape_delay_ms happened to work as a uvar was because it was explicitly special-cased. Any other variable handled by react_to_variable_change() is not handled during the initial loading of uvars. Note that env_universal_t::load() discard the callbacks that result from loading the uvars. Which is itself a big red flag that the logic isn't correct.

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 15, 2017

fix regression in how uvars are handled at startup
Fish 2.6.0 introduced a regression that keeps setting
`fish_escape_delay_ms` as a uvar from working. This also fixes a related
problem: callbacks generated from the initial loading of universal vars
were not being acted on.

Fixes fish-shell#4196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment