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

fix issue #30, take care of uppercase #31

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

chenyukang
Copy link
Contributor

No description provided.

@chenyukang
Copy link
Contributor Author

chenyukang commented Nov 26, 2021

I'm not sure about why we use this function to compare:

/**
 * This function uses case-sensitive logic if a second argument has an upper case. Otherwise, uses case-insensitive logic.
 */
export function caseIncludes(one: string, other: string): boolean {
  const lowerOther = other.toLowerCase();
  return lowerOther === other
    ? one.toLowerCase().includes(lowerOther)
    : one.includes(other);
}

I think it will get some confused results, for example in demo gif:
image

When we input a word in lowercase, some capitalized candidate will come out, that's usually not we wanted.

A consistent behavior is, when we input capitalized prefix, candidates should be all in capitalized, when we input lowercase prefix, candidates should be in lowercase prefix. Make candidate list short and accurate will help on speed up input.

Maybe others may have different preference on this, so adding an option is also helpful.

@@ -23,12 +23,20 @@ import { AppHelper } from "../app-helper";

function suggestWords(words: Word[], query: string, max: number): Word[] {
return Array.from(words)
.filter((x) => x.value !== query)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove this to L27, because I think it will cost more time when words list in very long, in my case, 2w words 😂

Copy link
Owner

Choose a reason for hiding this comment

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

As you said, it looks more faster, thanks 👍

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A similar improvement can be done at https://github.com/tadashi-aikawa/obsidian-various-complements-plugin/blob/main/src/ui/AutoCompleteSuggest.ts#L41.

This will iterate the words again.
A better ways is just use a list, and iterate the word list one time, put valid candidate to the list.

Copy link
Owner

@tadashi-aikawa tadashi-aikawa Nov 26, 2021

Choose a reason for hiding this comment

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

This will iterate the words again.

What does that mean? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After you merge the code, the code line number is not right now.
It's now https://github.com/tadashi-aikawa/obsidian-various-complements-plugin/blob/main/src/ui/AutoCompleteSuggest.ts#L49.

we may remove this line for performance, because it will iterate the long list again.

.filter((x) => x.value !== undefined)

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for your reply :) I understood.

image

On the other hand, It might be slow in the 10000case instead of 10000000case.

image

So I prior to readability for now.

@chenyukang
Copy link
Contributor Author

In my local, I actually change caseIncludes to this:

export function caseIncludes(one: string, other: string): boolean {
    return one.startsWith(other);
}

This will complete in strict prefix.
Otherwise, we may have such kind of scenario:
I input a prefix some', candidate list contains 'handsome', I only want words starting with some` ...

Any special reason we use includes instead of startwith?

@tadashi-aikawa
Copy link
Owner

I'm not sure about why we use this function to compare:

Because I am inspired by Vim search mode. Vim search works as the caseIncludes function.

On the other hand, This plugin is for complements, not search. There are many different complements logics, but famous editors and IDEs suggest first what we input. (ex: a -> [aaa, AAA], A -> [AAA, aaa])

However, Obsidian is not IDE for a coder. Therefore, I think it would be suitable as you said.

Maybe others may have different preference on this, so adding an option is also helpful.

I agree. It is a bit of another issue, so I might implement it after merging your PR.

@tadashi-aikawa
Copy link
Owner

tadashi-aikawa commented Nov 26, 2021

Any special reason we use includes instead of startwith?

I had thought it depended on how we use it or who uses it.

However, now I think they might be noisy in most cases. So I'll try it soon, thanks! :)

※ If no one uses partial match, it seems possible to complement more fast by indexing optimized. (in exchange for indexing speed)

@tadashi-aikawa
Copy link
Owner

By the way, this issue includes breaking changes as I didn't expected. So I'll merge it and then release v3.0.0 :)

@tadashi-aikawa tadashi-aikawa merged commit 2b3990a into tadashi-aikawa:main Nov 26, 2021
@chenyukang
Copy link
Contributor Author

I'm not sure about why we use this function to compare:

Because I am inspired by Vim search mode. Vim search works as the caseIncludes function.

On the other hand, This plugin is for complements, not search. There are many different complements logics, but famous editors and IDEs suggest first what we input. (ex: a -> [aaa, AAA], A -> [AAA, aaa])

However, Obsidian is not IDE for a coder. Therefore, I think it would be suitable as you said.

Maybe others may have different preference on this, so adding an option is also helpful.

I agree. It is a bit of another issue, so I might implement it after merging your PR.

Yes, if we want to execute a command line, fuzzy search is fine. Like VSCode, it also use 'includes' to match:
image

If we use to typing text, strict prefix matching will be better.

@tadashi-aikawa
Copy link
Owner

As an exception, It would be better for Internal Link Search to use insensitive-match. (at least in my case..)

image

image

@chenyukang
Copy link
Contributor Author

As an exception, It would be better for Internal Link Search to use insensitive-match. (at least in my case..)

image

image

Yes, in this scenario it may useful. But I think we can control it only do fuzzy match if word is internal link, starts with [[.

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

Successfully merging this pull request may close these issues.

2 participants