search

numToStr / FTerm.nvim

:fire: No-nonsense floating terminal plugin for neovim :fire:
MIT License
743 stars 23 forks source link

adding fix for ensuring fterm filetype is set #37

Closed beauwilliams closed 3 years ago

beauwilliams commented 3 years ago

this fixes this issue here: https://github.com/beauwilliams/focus.nvim/issues/49

I found sometimes the filetype changes from fterm to term.

beauwilliams commented 3 years ago

We can see now after we toggle fterm closed the filetype is still set to fterm

numToStr commented 3 years ago

Thanks for the fix. Although I am not sure about setting filetype twice. First being inside the :create_win method. Thoughts?

Edit: For more context see #13 #14

beauwilliams commented 3 years ago

I just removed the first call and it seems perfectly okay so now we have only one.

One other thing I noticed is that either way, the filetype initially (first run only) was never set to "fterm" in either case.

So I also went and pushed a fix for that. 2ea07ce

It should now have 'fterm' filetype set both on first run, and on close. It seems there are two events we need to handle so we might need to set ft for the both of them.

beauwilliams commented 3 years ago

Thanks for the fix. Although I am not sure about setting filetype twice. First being inside the :create_win method. Thoughts?

Edit: For more context see #13 #14

My last commit should help if not hopefully solve #13. They can use the filetype event to set options.

I think it was not working correctly earlier because 'fterm' filetype was not set on first run

numToStr commented 3 years ago

Thanks for making the changes. I don't use filetype event myself but It seems fine to me.

Also, I am thinking of exposing win hl and blend option anyway. So, we should be fine.

beauwilliams commented 3 years ago

Okay sounds good then. I think exposing those options sounds like a good idea. Probably less room for error

numToStr commented 3 years ago

@beauwilliams I left a comment can you address it? After that it should be fine to merge.

beauwilliams commented 3 years ago

@beauwilliams I left a comment can you address it? After that it should be fine to merge.

sorry I couldn't find what comment you are referring to

numToStr commented 3 years ago

I mean the review suggestion above.

beauwilliams commented 3 years ago

Do you mean add the option for hl and blend to the pr?

numToStr commented 3 years ago

Nope, this one image

beauwilliams commented 3 years ago

Weird. I really can't see that suggestion.. I'll remove that line now

beauwilliams commented 3 years ago

All done

numToStr commented 3 years ago

@beauwilliams Thanks for your contribution :)

beauwilliams commented 3 years ago

No worries 👍