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

A worth while commit #8

Closed
wants to merge 8 commits into from
Closed

A worth while commit #8

wants to merge 8 commits into from

Conversation

greduan
Copy link

@greduan greduan commented Dec 5, 2012

It will now recognize a [Command Line] buffer and always show static lines for it, instead of relative.

+ Updated the .gitignore file.
+ Added ./after/ftplugin/vim.vim which will recognize command line buffers and always use permanent line numbers on them.

... Which means that it will now recognize a [Command Line] buffer
and always show static lines for it, instead of relative.

+ Added a .gitignore file.
+ Added ./after/ftplugin/vim.vim which will recognize command line
  buffers and always use permanent number on them.
@jeffkreeftmeijer jeffkreeftmeijer mentioned this pull request Dec 5, 2012
@jeffkreeftmeijer
Copy link
Owner

Hey Eduan,

I understand this is a problem we need to fix, but I'm unsure about your solution. Can't we check the buftype before activating the plugin, instead of deactivating it later? I don't have a great answer to this myself, but it might be something we need to look into.

Also; could you remove the .gitignore file? I'm okay with .gitignore files, if they're there to ignore any files the project generates, but this one just seems like a list of system files. Correct me if I'm wrong, but I don't think this solves any problem.

@greduan
Copy link
Author

greduan commented Dec 5, 2012

Also, there's no need to try hijack my project by telling others to post their questions about this project to your fork. Let's continue this conversation in #8.

I'm sorry, I didn't mean to hijack your or anything. But since the last activity was a while ago, which is over 3 months now, I thought you might have been inactive, and so that people can still find this plugin, I took it upon myself to support it, at least until you showed signs of doing so yourself.

I hope you can understand. Again, I meant no ill anything.

I understand this is a problem we need to fix, but I'm unsure about your solution. Can't we check the buftype before activating the plugin, instead of deactivating it later? I don't have a great answer to this myself, but it might be something we need to look into.

I do see your point, but this is a buffer type that I've only managed to recognize this way, it works perfectly, but it may be worthwhile to check it out, as you say.

I will do my best to find another solution which recognized it before opening it. 😄

Also; could you remove the .gitignore file? I'm okay with .gitignore files, if they're there to ignore any files the project generates, but this one just seems like a list of system files. Correct me if I'm wrong, but I don't think this solves any problem.

In my opinion it does, let me explain. Basically, it matches all the "system files" that Windows or Mac may impose on the repo. Instead of ignoring them manually, I'm doing it automatically. 😄

Although, as you asked me, I will remove it on my next commit. 😄

- Eduan

~ I changed the folders to be from "after/ftplugin/vim.vim" to
  "ftplugin/vim.vim", which will make it load faster.
- Deleted all the changes to the .gitignore file.
@greduan
Copy link
Author

greduan commented Dec 5, 2012

OK, I changed the .gitignore file as you asked me. I also changed my addition's folder from after/ftplugin/vim.vim to ftplugin/vim.vim.

I thought about what you said, and I think the only way to do this would be to make the whole plugin not load unless called. In other words, put the whole plugin on the autoload Vim folder, and use a plugin folder which will only contain a method to call the plugin.

But since this is a plugin that's meant to be called every time, the easiest way is to simply deactivate it, instead of only activating it when it is needed.

What are your thoughts?

@greduan
Copy link
Author

greduan commented Dec 5, 2012

I took the liberty of fixing issue #6 with this commit. 😄
And it worked BTW. 😉

Eduan Lavaque added 2 commits December 6, 2012 07:40
WinLeave. In other words, made the difference clear.
I re-wrote a lot in order for it to have be able to be turned off,
or on etc. It just solves the issue that issue #5 expressed.
@greduan
Copy link
Author

greduan commented Dec 6, 2012

Even though commit b2a1ea7 expresses that I re-wrote stuff, I really didn't. I just put some ifs, elses, and modified the order of stuff.

But! This fixes issue #5, for providing a way of disabling numbertoggle by default, and it also adds the option to fix the issue you expressed, of disabling it, instead of not enabling it. I will try to make it work with the [Command Line] buffers.

EDIT:
I tried, but the logic wouldn't allow it. I still think the current solution is good though. It doesn't matter that much for such a small buffer though, I mean, it's just a buffer that most people don't know about.

And if I use the current logic I pushed, I gotta either enable it completely, or disable it completely. But this fix works.

Made the whole functionality of numbertoggle be put on "autoload/".
Which would make it more light weight if the user defaults it to off.
And also make it easy to modify the functions, instead of modifying it
over several places.
@greduan
Copy link
Author

greduan commented Dec 6, 2012

Made the whole thing a lot more light weight, and easier to maintain, and still has current functionality. Haven't figured out way to solve first problem that rised in your first comment.

Eduan Lavaque added 2 commits December 6, 2012 13:16
* Fixed bug where it would check if Vim was compiled with 'autocmd'
  support, instead of checking if it did NOT have 'autocmd' support.
@greduan
Copy link
Author

greduan commented Dec 7, 2012

Just a minor change, that's very useful. But with that I created a bug, which I now fixed. 😄

This is turning out to be almost a whole rewrite, mostly of the structure, but for the better. So I hope you like it and accept it. 😉

@jeffkreeftmeijer
Copy link
Owner

Hey Eduan,

Could you open a pull request per change, with the change in a single commit? It seems like you fixed a lot of stuff in this pull request, and I'm losing track a bit. If we can't find a better way, let's deactivate the plugin if there's no file open (like you did in the first place) for now.

Thanks for helping out! :)

@greduan
Copy link
Author

greduan commented Dec 7, 2012

OK, I'll close this pull request. And make a new one with all the changes in one go, so that it's clearer. :) Otherwise you would have a lot of open requests, and it would be a lot of extra work just for each single pull request. ;)

Right now I'm implementing a way to turn it on or off, so expect that. :)

@greduan greduan closed this Dec 7, 2012
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