ameyp / CscopeSublime

A Cscope plugin for Sublime Text 2 and 3
89 stars 37 forks source link

Use universal line endings to fix issue with cscope in Cygwin #40

Closed ncmiller closed 8 years ago

ncmiller commented 10 years ago

With a Windows version of Sublime and a Cygwin installation of cscope, only the first result was being returned because of line-ending expectations of the plugin. Adding universal_newlines to Popen allows for any type of line ending. The encoding of stdout and stderr will be dictacted by locale.getpreferredencoding(False).

vanrijn commented 8 years ago

Thanks for the patch, @ncmiller!!! I have a couple of questions that I asked on the diff itself. Also, it looks like we have some conflicts now with my recent commit. Would you be able to download the latest from master and rebase and re-post the patch? Thanks!! =:)

ncmiller commented 8 years ago

For the original PR in 2014:

Cscope was giving erroneous results for files with Windows-style newlines. I added universal_newlines to Popen and that seemed to fix the problem, but created another. As it turns out, that param causes the stream to be opened in text mode instead of binary. As a result, I was getting decode errors similar to the following:

Exception in thread Thread-31:
Traceback (most recent call last):
  File "C:\Users\nmille01\AppData\Roaming\Sublime Text 3\Packages\CscopeSublime\cscope.py", line 299, in run_cscope
    output = str(output, encoding="utf8")
TypeError: decoding str is not supported

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./threading.py", line 901, in _bootstrap_inner
  File "C:\Users\nmille01\AppData\Roaming\Sublime Text 3\Packages\CscopeSublime\cscope.py", line 321, in run
    matches = self.run_cscope(self.mode, self.symbol)
  File "C:\Users\nmille01\AppData\Roaming\Sublime Text 3\Packages\CscopeSublime\cscope.py", line 301, in run_cscope
    output = unicode(str(output), encoding="utf8")
NameError: global name 'unicode' is not defined

I removed the decoding step since it was no longer needed with the text-based stream.

Now

I moved universal_newlines to if (self.platform == "windows") as suggested and added an if statement to perform the decode only if not using the universal_newlines option.

However, I'm a little worried that the cscope newline issue may also be applicable to non-Windows platforms. I don't have another platform to test on at the moment - does the plugin operate correctly on other platforms when used on a file with Windows-style newlines?

vanrijn commented 8 years ago

Hey @ncmiller, thanks for the reply! =:)

Hmmmmm. Actually, I think maybe I misunderstood earlier. But IIUC, it looks like universal_newlines means that anything that is returned over the Popen call will have only \n as the newline character. So all CscopeSublime needs to look for is a \n newline separator, regardless of platform. Cool! I just tried this on OS X by always putting universal_newlines = True into popen_arg_list and removed the try/except block entirely, and it seemed to work fine. So if you're saying that works correctly on Windows, I think I'm fine with doing that. I'll comment on your diff directly, but assuming I'm understanding correctly, I think that's an elegant way of cleaning this up for all platforms.

vanrijn commented 8 years ago

Sorry for the confusion on this, @ncmiller. Completely my fault. I now realize that the way you did this at first was what looks to be the right way. =:) Thanks for your patience.

Also, FWIW, IIRC we had to add the try/except block for an error condition we hit in ST2. I'm not sure if we'll hit whatever problem that was again with ST2 after this, or if the universal_newlines=True will fix it via stdout being opened as a text file, but I'm willing to give it a shot without the try/except. =:)

vanrijn commented 8 years ago

And now I'm talking to myself.... I found where this try/except was added: https://github.com/ameyp/CscopeSublime/commit/ebf292c1c552632167784dc7b5b13b868c1c21b1

I think it was when we ported CscopeSublime from ST2 (python2) to ST3 (python3). We were using the python3 str(object=b, encoding='utf-8') method, which python2 doesn't have. So the except basically only got hit if we were in ST2/python2.

Anyway, it still looks like universal_newlines=True was an option with ST2/python2 and would have solved the problem in a more elegant way.

ncmiller commented 8 years ago

No worries :) I went back to the original implementation.

vanrijn commented 8 years ago

Thanks Nick!!