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

filter function restore on session load #1970

Merged

Conversation

doremiyeon
Copy link
Contributor

loading a session or switching to a different context with a different filter mode setting (regex vs string) will not update the filter function and can lead to crashes

  • on session load, restore correct filter function
  • also switch filterfn on ctx changes

src/nnn.c Outdated
@@ -4542,6 +4543,7 @@ static bool load_session(const char *sname, char **path, char **lastdir, char **
*path = g_ctx[cfg.curctx].c_path;
*lastdir = g_ctx[cfg.curctx].c_last;
*lastname = g_ctx[cfg.curctx].c_name;
setcfg(cfg);
Copy link
Owner

Choose a reason for hiding this comment

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

How are the other 2 working without calling setcfg()? Is savecurtx() being called in some way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the other 2 contexts? savecurtx() should be called on any context switch, I think. I don't really like calling setcfg here, because it doesn't change the cfg assignment and set_sort_flags() already sets the other two function pointers. the important point is that the filter function pointer is set for the current context after the session is loaded. i think one of the individual commits has some more information on that

Copy link
Owner

Choose a reason for hiding this comment

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

No, entrycmpfn and namecmpfn.

Copy link
Owner

Choose a reason for hiding this comment

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

If I am not clear, I wanted to understand how entrycmpfn and namecmpfn are being set without setcfg not being called in the current code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I am not clear, I wanted to understand how entrycmpfn and namecmpfn are being set without setcfg not being called in the current code?

Okay, just tested it out and the answer is that they're not being set. To reproduce:

  1. Open nnn
  2. Switch to regex filtering
  3. Save session (e.g name it "regex")
  4. Close nnn
  5. Open nnn again (with string filter being default)
  6. Load "regex" session
  7. Try to search with regex, e.g ^s and notice that there's no match even though it should match src/ folder.

image

With the current PR, if I repeat steps 5-7 then the regex works properly.

image

I think the patch looks fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wanted to understand how entrycmpfn and namecmpfn are being set without setcfg not being called in the current code?

Ah, wait. I didn't read carefully. entrycmpfn and namecmpfn are being set inside the set_sort_flags('\0');.

I don't really like calling setcfg here, because it doesn't change the cfg assignment and set_sort_flags() already sets the other two function pointers.

Me neither after looking a bit more. I've changed it back to only doing filterfn now.

@N-R-K
Copy link
Collaborator

N-R-K commented Jan 5, 2025

this should fix more crashes related to switching between contexts with
different types of filters active. particularly when switching from a regex
filtered context to a string filtered one.

I can reproduce this by compiling with ASan:

$ make CFLAGS_OPTIMIZATION='-g3 -fsanitize=address,undefined'

And then switching between string and regex filtered context.

Commit 0beb2b8 fixes the issue and looks good to me. I don't know about the other commit since I don't use sessions and haven't tested it.

@N-R-K

This comment was marked as outdated.

this should fix more crashes related to switching between
contexts with different types of filters active. particularly
when switching from a regex filtered context to a string
filtered one.
@N-R-K N-R-K force-pushed the filter_function_restore_on_session_load branch from 0beb2b8 to a9093c6 Compare January 5, 2025 18:08
@jarun jarun merged commit e131f5f into jarun:master Jan 6, 2025
7 checks passed
@jarun
Copy link
Owner

jarun commented Jan 6, 2025

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants