[Geany-devel] [CODE REVIEW] Changes to encodings
mbrush at xxxxx
Wed May 18 00:05:51 UTC 2011
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:
> 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
> 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:
>> . 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, 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 :)
More information about the Devel