Export user-options on change

[raslop]

If one has an external previewer that uses an lf user-option (e.g. "preview-mode") and some key-kindings that change this user-option (e.g. to "hex", "simple", "fancy") and send a "reload" command to lf, then the expectation after pressing the keys is, that the prewiever is called with an updated value of the user-option. Though this did not happen before this change.

[joelim-work]

The exportOpts call actually used to exist, but I removed it in #1354 because it was causing lf to crash. Correct me if I'm wrong, but setting environment variables (i.e. setenv) isn't thread-safe, and since it's already done in the main thread (app.runShell), doing this in the previewer thread as well is likely to cause issues.

I guess you can try changing your config to have the keybinding set the user-defined option, and in addition export it by adding some dummy shell call (e.g. $true), though it does sound quite hacky. Otherwise another option might be to export a user-defined option immediately after setting it or changing its value.

[raslop]

The exportOpts call actually used to exist, but I removed it in #1354 because it was causing lf to crash.

Ah, ok. Thanks.

Correct me if I'm wrong, but setting environment variables (i.e. setenv) isn't thread-safe, and since it's already done in the main thread (app.runShell), doing this in the previewer thread as well is likely to cause issues.

You are right, setenv is not thread-safe.

I guess you can try changing your config to have the keybinding set the user-defined option, and in addition export it by adding some dummy shell call (e.g. $true), though it does sound quite hacky.

I tried, but didn't succeed. Maybe I got the syntax wrong.

Otherwise another option might be to export a user-defined option immediately after setting it or changing its value.

That sounds nice. I've implemented this in the eval function in eval.go it and it works for me. I hope the eval is always called from the main thread.
If reverted the previous change and force-pushed the branch.

[joelim-work]

Thanks for revisiting this, The change looks fine to me, perhaps it might be better to leave a comment in the code to explain the purpose of doing this.

I believe the original design of the previewer script was that all the necessary information required to display a preview (i.e. filename and dimensions) would be passed in directly as arguments, and the script would not need to depend on anything else. Of course this doesn't work nicely with user-defined variables, AFAIK most users don't have such a requirement.

I will leave this PR open for now, @gokcehan will want to review this and might even come up with another solution. BTW we are planning a new release soon, so if you're lucky this change can be squeezed in.

[raslop]

Thanks.

I've added an explaining comment.

[gokcehan]

Thank you @raslop for the patch and @joelim-work for the review. I can't say I understand much about the usecase for this, but the change itself looks good to me. Even though we're currently in a feature-freeze for r31, I think it is better to merge this since I think this is somehow related to the previous change mentioned. I have tried to add this to the release notes in #1407. Let me know if there is a better wording for this change.

[raslop]

Thanks a lot.
Regarding the release notes (3rd "Breaking" item: I think the reference to PR 1418 and my name should be removed from the first sentence. I would formulate the last sentence probably more like: User options (i.e. user_{option}) are now exported whenever they are changed (1418)

[gokcehan]

@raslop I have now changed the notes accordingly. Thanks again.