Skip to content

Add set --no-event to change a variable without triggering an event#10480

Merged
faho merged 2 commits into
fish-shell:masterfrom
faho:set-no-event-rs
May 14, 2024
Merged

Add set --no-event to change a variable without triggering an event#10480
faho merged 2 commits into
fish-shell:masterfrom
faho:set-no-event-rs

Conversation

@faho

@faho faho commented May 6, 2024

Copy link
Copy Markdown
Member

Description

As mentioned in #9030 this adds a --no-event option to set, so you can set or erase a variable without triggering the event.

Example:

function on-my-var --on-variable myvar
    echo myvar was changed!
end

set --no-event myvar foo # <- this will not print "myvar was changed!"

There is no short option because this is an advanced option that should be used with care.

This does what a potential use of the block builtin would do in a simpler way, and it can be used to remove one of the gnarliest parts of our key binding functions: They set the variable, causing the handler to be run, which then runs the function again.

This upgrades our functions to use --no-event, but notably in a way that it'll handle being run by older fishes. This is necessary because otherwise we'd be left with no bindings at all. Tho the binding functions have basically been rewritten to handle the new bind notation, I'm not entirely sure how an old fish accidentally running the new version would work?

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

faho added 2 commits May 6, 2024 17:00
This allows running `set` without triggering any event handlers.

That is useful, for example, if you want to set a variable in an event
handler for that variable - we could do it, for example, in the
fish_user_path or fish_key_bindings handlers.

This is something the `block` builtin was supposed to be for, but it
never really worked because it only allows suppressing the event for
the duration, they would fire later. See fish-shell#9030.

Because it is possible to abuse this, we only have a long-option so
that people see what is up.
This is how we can use it in a backwards-compatible way. Eventually we
would remove the compatibility guff.
@faho faho added this to the fish next-3.x milestone May 6, 2024

@mqudsi mqudsi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tho the binding functions have basically been rewritten to handle the new bind notation, I'm not entirely sure how an old fish accidentally running the new version would work?

I'd rather not introduce backwards compat workarounds into the codebase if we're not sure they're necessary. The only case I think we have to support is "user upgrades fish, is still running old fish because they haven't started a new session."¹ I don't think there's a way to re-exec the old fish binary (we always load it from the disk) so the new version of startup-only code (like the key bindings) should theoretically never be seen/executed by the older fish install, no?

If there's a situation I'm not seeing, then by all means, please enlighten me. Otherwise, we have never supported multi-versioning of fish and standalone, unpackaged builds of fish should be using the share/ directory from the source code distribution and not from $(PREFIX)/usr/share, right?

¹ For the record, we have shipped upgrades in the past that broke this scenario. It's obvious what's happened if you are intimately familiar with fish, but I pity those that had an OS auto-update in the background and then started seeing random script errors when sourcing various completions or autoloading now-incompatible functions.

@faho

faho commented May 8, 2024

Copy link
Copy Markdown
Member Author

I'd rather not introduce backwards compat workarounds into the codebase if we're not sure they're necessary

To be clear: Working around a lack of --no-event is necessary.

I was talking about how an old fish would interpret the new bind sequences - it would probably error out a lot, but I think there's enough there that the bindings should still be enough to at least reexec fish?

If there's a situation I'm not seeing, then by all means, please enlighten me.

The problem is that fish reloads functions that have changed on-disk.

So you have a long-running fish session, you upgrade fish, and in that long-running fish session something triggers a binding change. If that happens after it reloaded the function, you've now run bind --erase --all --preset, but not set the new bindings. You have completely empty bindings. Nothing does anything. (with the exception of any user-specified bindings)

You now have a hanging fish that is impossible to kill from inside.

I believe that's egregious and should not be allowed. We could improve this in other ways, like e.g. removing the auto-reloading of functions because it's a major PITA on upgrade. But the important part is that we sometimes need to do something to not make it horrible when people upgrade.

(of course I have also tried to accomodate people who get newer functions for older fishes, within reason - that won't happen here because the function's been 90% rewritten already)

@mqudsi

mqudsi commented May 9, 2024

Copy link
Copy Markdown
Contributor

The case you describe would indeed by terrible, but there's something I'm missing:

The problem is that fish reloads functions that have changed on-disk.

When and where does this happen? I always have to constantly source /path/to/foo.fish any time I edit the declaration of foo on disk directly instead of using funced (especially when working on new/updated completions).

EDIT: We actually had to add a workaround for this to the funced definition when the function isn't changed in the editor but was already silently changed on-disk in the background before funced was executed but after the function was sourced or autoloaded.

@faho

faho commented May 9, 2024

Copy link
Copy Markdown
Member Author

Weird, that's supposed to happen.

Regardless, even if it didn't, the failure mode here is sufficiently terrible that a bit of effort is warranted - even if someone ran an old fish and switched to vi-mode they'd trigger this, and it would kill their bindings. It's also just a couple of lines, so I feel like we're already having more of a discussion about it than it really warrants.

Are you okay with the feature itself?

@mqudsi

mqudsi commented May 12, 2024

Copy link
Copy Markdown
Contributor

Yeah, I think the feature itself makes sense and simplifies a lot of the trickier cases involving events. It's a straightforward escape hatch.

@faho faho merged commit 24836f9 into fish-shell:master May 14, 2024
@faho faho deleted the set-no-event-rs branch May 14, 2024 15:31
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants