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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support multi-part extensions for file icons #1053

Conversation

mikeroelens
Copy link
Contributor

@mikeroelens mikeroelens commented Feb 23, 2024

nvim-tree/nvim-web-devicons supports "mult-part" file extensions.

E.g. for these 2 files:

  • file1.js
  • file1.test.js

nvim-web-devicons can return different icons for "js" and "test.js". But right now fzf-lua only ever passes "js" as the extension.

This PR parses the filename in order to determine the extension and handles these multi-part extensions.

Before:
before

After:
after

P.s. thank you for this plugin!! 馃敟

-- "file.js" -> "js"
-- "file.test.js" -> "test.js"
-- "file.spec.js" -> "spec.js"
local function get_extension_from_file_name(file_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're going to consider this PR please scrutinize this part. I'm very new to Lua so this was mostly ChatGPT

M.get_devicon = function(file, ext)
-- by default the extension will be derived from `file`, but you can
-- override it by passing `extensionOverride`
M.get_devicon = function(file, extensionOverride)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed unnecessary to pass in the existing ext parameter since we only needed the filename. But there was one case where we need to force "sh" as the extension so I changed this param to be extensionOverride

@ibhagwan
Copy link
Owner

Ty for this PR @mikeroelens, definitely worth considering, I will look into it as well as the code provided, this is called for every single file and has performance implications, I鈥檒l do some testing and if need be use something other than a lua regex, it鈥檚 also worth looking what are the conditions for enabling that in devicons (is it the default?) and respecting the options if such exists.

@ibhagwan
Copy link
Owner

ibhagwan commented Feb 23, 2024

Question, does this PR take into account a fallback icon? For example, the file name is foo.bar.baz.js, according to the code we will test for an icon for bar.baz.js, assuming no icon is found, should we test for baz.js or just .js or do we display the default empty icon (wouldn鈥檛 be my choice)?

@mikeroelens
Copy link
Contributor Author

@ibhagwan wow thanks for the wicked fast reply!

g what are the conditions for enabling that in devicons

These extensions are built right into devicons (example)

When looking through that though I realized this is only a thing for JS projects. So if there's any performance degradation at all likely not worth it. Or at least could make it configurable I suppose.


Question, does this PR take into account a fallback icon?

Great call! Just tested it and it doesn't work as expected, just falls back to the overall default. I can try and fix that

@ibhagwan
Copy link
Owner

ibhagwan commented Feb 23, 2024

When looking through that though I realized this is only a thing for JS projects. So if there's any performance degradation at all likely not worth it. Or at least could make it configurable I suppose.

Yes, I just did the same exploration of the devicons code and got to the same conclusion.

So if there's any performance degradation at all likely not worth it. Or at least could make it configurable I suppose.

I think there is bound to be performance impact as we鈥檒l need to (at the bare minimum) test for 2 different extensions, xxx.js and .js as fallback (you wouldn鈥檛 want to lose the icon for regular js files), I assume we would probably want to test this for every extension not just js (for future devicons changes) which means the double(+) map lookup would happen on every file containing more than one dot, we鈥檇 also need to scan all files for every dot occurrence (vs reverse search from the end as it is now).

Great call! Just tested it and it doesn't work as expected, just falls back to the overall default. I can try and fix that

If we are to do this work, I鈥檇 like to refactor the entire logic and basically stop calling devicons.get_icon and have our own logic do the work against the copy of the icons table we already maintain for the multiprocess queries so don鈥檛 spend too much time on this as I鈥檓 going to rewrite the whole thing differently.

@ibhagwan
Copy link
Owner

Perhaps we can limit the perf impact if we decide to support only 2-part extensions, this way we limit the dot reverse search to 2, this still means we鈥檇 have to perform 2 lookups for files in the format of file.class.obj.ext, one failed looked up for obj.ext and another for ext.

Another idea I have is a list of valid pre-extensions warranting lookups, so we can limit the 2nd lookup only if the second part is test or spec therefore skipping the above example of obj.ext.

All of the above won鈥檛 prevent having to reverse scan for dots but it would alleviate some of the performance loss.

@mikeroelens
Copy link
Contributor Author

Perhaps we can limit the perf impact if we decide to support only 2-part extensions, this way we limit the dot reverse search to 2, this still means we鈥檇 have to perform 2 lookups for files in the format of file.class.obj.ext, one failed looked up for obj.ext and another for ext.

Another idea I have is a list of valid pre-extensions warranting lookups, so we can limit the 2nd lookup only if the second part is test or spec therefore skipping the above example of obj.ext.

All of the above won鈥檛 prevent having to reverse scan for dots but it would alleviate some of the performance loss.

All of this makes a ton of sense to me!

Another thing I was thinking about is personally I don't really care about the different icon for test files. I more so care about a different icon color for tests. Just some quick indicator to distinguish a test file from a regular file. I'm wondering if people feel the same way. If so maybe there's a nice way fzf-lua could expose a mapping like:

icon_mapping = {
   [".test.ts"] = {
       color = "yellow",
   },
   [".spec.ts"] = {
       color = "yellow",
   },
   -- Example where the naming convention doesn't have another '.' but rather just ends with '_test'
   ["_test.go"] = {
       color = "yellow",
   },
}

Essentially a mapping of "file name ends with" - > icon color (could also add other options like icon_override). Would be neat for a lang like Go that has a filename pattern of _test.go for tests.

It would be a little more flexible than strict "extensions". A user could provide anything, and could still keep it as performant as possible by only looking for known mappings. Just food for thought, just throwing it out there in case it also sparks any ideas for you 馃槃

@ibhagwan
Copy link
Owner

Just some quick indicator to distinguish a test file from a regular file. I'm wondering if people feel the same way. If so maybe there's a nice way fzf-lua could expose a mapping like
Essentially a mapping of "file name ends with" - > icon color (could also add other options like icon_override). Would be neat for a lang like Go that has a filename pattern of _test.go for tests.
It would be a little more flexible than strict "extensions". A user could provide anything, and could still keep it as performant as possible by only looking for known mappings. Just food for thought, just throwing it out there in case it also sparks any ideas for you 馃槃

IMHO we should match the behavior devicons, over configuration (fzf-lua is already very customizable) can create confusion and customizing icons is better suited for devicons overrides (which fzf-lua should respect).

@mikeroelens
Copy link
Contributor Author

IMHO we should match the behavior devicons, over configuration

Yep, you're totally right! I just reread the advanced configuration docs and seems like I can make anything happen if I really want to. Well thanks for all your help so far! If there's any way I can continue to help please let me know!

@ibhagwan
Copy link
Owner

IMHO we should match the behavior devicons, over configuration

Yep, you're totally right! I just reread the advanced configuration docs and seems like I can make anything happen if I really want to. Well thanks for all your help so far! If there's any way I can continue to help please let me know!

Ty @mikeroelens, I鈥檒l give this some more thought and do it sometime next week.

@ibhagwan
Copy link
Owner

ibhagwan commented Mar 8, 2024

This is gonna take a tiny bit longer as I'm using this as an opportunity for a much bigger refactor of the way devicons (and its highlighting work) in fzf-lua, you can track the progress in the refactor_devicons branch.

@ibhagwan
Copy link
Owner

ibhagwan commented Mar 17, 2024

@mikeroelens, looks like I'm done with the refactor you're free to give it a run.

I'll do more extensive testing tomorrow and merge it.

A note regarding the logic I came up with for minimal perf impact.
On init, we load all the devicons and generate 4 different maps:

  • by_filename
  • ext_has_2part
  • by_ext_2part
  • by_ext

This way the maps are smaller and lookups are faster as a result, the lookup order is as follows:

  • If path ends in a separator we return a directory icon (only in fzf-lua, devicons has no concept of directory)
  • Check direct match of the filename within by_filename
  • Check if extension has an entry in ext_has_2part (for example js would return true here)
  • If the above returns true we extract the extension prefix (i.e. spec) and test for existence of spec.js within by_ext_2part
  • Finally, we do a simple lookup of the extension in by_ext

With the latest version of nvim-web-devicons there are only 5 extension that would trigger a double lookup:

ext_has_2part

{ js, jsx, ts, tsx, ru }

by_ext_2part

{ config.ru, spec.js, spec.jsx, spec.ts, spec.tsx, test.js, test.jsx, test.ts, test.tsx }

As you can understand from the above we'd only reach the 2nd lookup if we get a hit in the ultra-fast ext_has_2part and even when we do we only have to lookup (currently) a 9-elements map.

I also improved on the ansi highlight generation codes so overall I assume we actually gained performance from this endevaor :-)

ibhagwan added a commit that referenced this pull request Mar 17, 2024
In order to supprot multi-part extensions with minimal
performance impact fzf-lua will now use its  own version
of the devcions lookup tables built from the original plugin
tables.

For more info, see discussion in #1053.
@ibhagwan
Copy link
Owner

@mikeroelens, this is now merged to main, including your original contribution (2ba4a51 - authored by you), closing.

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.

None yet

2 participants