On 26/01/2012 05:21, Lex Trotman wrote:
On Thu, Jan 26, 2012 at 3:41 PM, Matthew Brushmbrush@codebrainz.ca wrote:
On 01/25/2012 06:45 PM, Lex Trotman wrote:
[...] Hi Matthew,
[...]
utils_is_uri() - good utility function, well named
Indeed well named and probably a little clearer that `strstr(uri, "://") != NULL` but that's probably what I'd write if I didn't know Geany had it's own function for this, or I'd use g_uri_parse_scheme() or something.
You raise a good point, documentation and awareness of utils
I think reading utils.h (and utils.c for specific details) is a reasonable expectation. Often using autocompletion helps if you know the start of the function name.
resources. ATM the only documentation produced is for functions in the API, needs a version for the utils and any other generally useful bits. Can doxygen distinguish two sets of doc comments in the one file so we can make a utils documentation set as well as the plugin API?
I would guess doxygen can't do that. Also it might be less clear as to what's in the plugin API and what's not.
[...]
utils_spawn_async() - I think was used more than one place in the past, also hides the messy #ifdef windoze which is good
If the GLIB functions don't work (ie. on win32), we should send a bug report/patch upstream, just as we'd do with Scintilla if we found an obvious bug. We shouldn't have our own fixed implementation, IMO (unless it does something else I'm not aware of, or upstream refused the fix).
You'll have to ask Enrico (I think) why the win API is used and why builds on windows are synchronous, thankfully most of this was done before I arrived and I only have a vague awareness of the reasons. OTOH I don't know that I like your chances of fixing windows Glib and then it will be in a version we get to a ways in the future so the win interface will have to stay for some time.
I'm pretty sure the glib spawn functions do not work, other projects had this problem too. I think it is a design flaw rather than implementation detail, but feel free to research this.
FWIW I think utils_spawn_async() is a disaster - it tries to combine async and sync parameters in one function, they are fundamentally different.
If we implement async spawn on Windows (like SciTEWin::ExecuteOne()) then the utils_spawn_async() parameters would no longer make sense.
Maybe we should deprecate it now because of these issues (it's in the plugin API).
[...]
utils_make_filename() - reasonable utility function, probably should be used in more places where filename.ext concat is done explicitly
I never would've thought to use this function over g_strjoin() and g_build_filename() or something.
Actually utils_make_filename() is really just g_strconcat without one separator argument. I don't mind if we remove it.
To reply to remaining points from earlier message:
On 26/01/2012 02:45, Lex Trotman wrote:
utils_slist_remove_next() - local static used one place, agree no reason to exist
I decided to remove it. I thought it might get used elsewhere, but it didn't.
utils_string_replace() - probably should be static, only used several times in utils itself
This was used in editor.c but the code got rewritten differently. I'm not sure that it's worth making it static as this will trigger a rebuild of src/*.o and we might use it sometime.
utils_build_path() - g_build_filename() has better portability semantics, should replace utils_build_path()
Good point.