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

Duplicate entries in suggestions list #55

Open
krackers opened this issue Feb 18, 2021 · 4 comments
Open

Duplicate entries in suggestions list #55

krackers opened this issue Feb 18, 2021 · 4 comments

Comments

@krackers
Copy link

krackers commented Feb 18, 2021

I'm seeing an issue where there are duplicate entries in the suggestions list.

I can replicate this with a minimal example of having

51.9615242269   /Users/foo/bar

in the autojump.txt and running

autojump --complete bar

which returns

1__/Users/foo/bar
2__/Users/foo/bar
3__/Users/foo/bar

I added some debug statements in query:

Entries [Entry { path: "/Users/foo/bar", weight: 51.9615242269 }] 
Query Result ["/Users/foo/bar", "/Users/foo/bar", "/Users/foo/bar"] 

and isolated the issue to somewhere in the matcher (src/matcher/).

@krackers
Copy link
Author

krackers commented Feb 18, 2021

Oh wait I see in the unit test that this is expected behavior for the matcher because we have 3 different match types?


    let actual: Vec<_> = matcher.execute(&haystack).collect();
    let expected = vec![
        // consecutive matcher
        path::Path::new("/moo/foo/baz"),
        path::Path::new("/foo/baz"),
        // fuzzy matcher
        path::Path::new("/foo/bar/baz"),
        path::Path::new("/moo/foo/baz"),
        path::Path::new("/baz/foo/bar"),
        path::Path::new("/foo/baz"),
        // anywhere matcher
        path::Path::new("/foo/bar/baz"),
        path::Path::new("/moo/foo/baz"),
        path::Path::new("/foo/baz"),
    ];

I think we would want to add a unique to make sure we don't double count the matches though. I think doing

results.sort();
results.dedup();

in query.rs is sufficient.

Also unrelated but I think the order should be changed to consecutive, then anywhere, then fuzzy.

@xen0n
Copy link
Owner

xen0n commented Feb 18, 2021

Much time has passed after the initial porting, I can't recall the fine details but I thought this behavior is consistent with original autojump so I even wrote the test case in the first place.

Now it's evident the "upstream" is dead, is not reviving, improving this behavior is okay for me. It's instead a problem of finding free time and motivation to revitalize this project though...

krackers added a commit to krackers/autojump-rs that referenced this issue Feb 18, 2021
@krackers
Copy link
Author

krackers commented Feb 18, 2021

I thought this behavior is consistent with original autojump

Yes I think you're right. wting/autojump#348

For some reason I can't replicate it myself, but it seems it's still present. I don't think fixing it will break compatibility.

@krackers
Copy link
Author

krackers commented Feb 18, 2021

Actually no it doesn't occur in upstream. They use set to dedup before printing:

for i, entry in enumerate(set(tab_entries)):

Edit: No wait that's apparently only in my local copy? 🤔 Maybe I added this myself long time back.

krackers added a commit to krackers/autojump-rs that referenced this issue Feb 18, 2021
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

No branches or pull requests

2 participants