On 05/17/11 08:30, Colomban Wendling wrote:
Le 16/05/2011 05:45, Matthew Brush a écrit :
I was wondering if anyone had time to review some changes I've been working on?
No really as you see, times is rare these days :/ But I'm trying to do my best...
Thanks a lot for taking the time to review these changes, and also for using GitHub since it makes it *very* easy for me. I will review them all and make the appropriate fixes after I finish checking my emails.
I did a fast and inaccurate review of the patch, see my comments on it: https://github.com/codebrainz/geany/commit/81c89cce736c88f235761ce5765f2361f...
yeah, I'm trying GitHub to see what it actually gives. But I'm afraid it'll make my comments less visible for the ML readers, so unlikely fr them to comment back…
I guess if more people were using it and watching each others repositories, everyone would be on the same page by getting notifications of such things.
afterwards (though it's not such a big deal). Also, I'm not completely satisfied with the name of the sci_add_text2() function added to the sciwrappers files, but there was already a function named sci_add_text() which truncates the text at the first NUL, and the new function is a more direct wrapper of the SCI_ADDTEXT message.
sci_add_text_full() ?
having counterpart for set_text() would probably be good, too.
I like the _full idea like GLib uses. Regarding set_text(), it's actually one of the Scintilla functions that expects NUL termination. The reason for the add_text2() one is that add_text() was wrapped "wrong" (it's supposed to take a length param like in Scintilla) and I didn't want to clobber it for existing code. That's not to say there couldn't be a set_text_full() which internally handles the case of embedded nulls by doing a clear_all() then a add_text() in the same way my changes do in document.c
These changes break the plugin API, and would require two small changes to the geany-plugins GeanyVC: geanyvc.c:480 and geanyvc.c:498 as well as GeanyGenDoc: ggd-plugin.c:343. It should be obvious why this breakage was required I hope.
yeah, sounds reasonable for such a change. However, prefer breaking the API once (or at least it a single row), so I wouldn't see these committed before further checking and other checks to whether other APIs have to be updated.
Agreed, if I understand you correctly.
The following bug reports are related to the first set of changes: [1][2][3][4]. A few of the comments seem to suggest that it's not desired to support text files containing NUL control bytes, but IMO, it's nice to be able to show these anyway, especially since Scintilla has a nice way of dealing with these. IMO, as long as the file gets marked read-only, displaying as much as possible is better than truncating at the first NUL byte.
As you discussed with Lex, it's something that may please some, but supporting this partially may simply move the user complaints :D
Agreed, although I think an appropriate warning message change when such a file is opened will go a little further to help with user complaints. Still, you're both right on this.
The second one is a simple refactoring of the code around the regex-based encoding detection and adding a few more patterns to detect a few more types of files. Especially needing review are the actual pattern strings I used since, quite frankly, I suck at regexps. One thing I noticed looking at the bug report here[5], after creating these patches/commits, is that it seems the<?xml ... encoding="" ?> is already supported by the CODING regex, so you can ignore the new XML one I added if that truly is the case (I'll remove it from the proper patch I send in the future). Like I said, my regex skills aren't great :)
As said on GitHub, I'm puzzled with this one.
When I write regex code it looks more like my cat walking across the keyboard than it does me actually writing code :)
Cheers, Matthew Brush