[Geany-devel] [CODE REVIEW] Changes to encodings

[Matthew Brush]

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/81c89cce736c88f235761ce5765f2361fe2bedc8
>
> 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