Re: [Geany-devel] encoding combo boxes bug - utility functions
On 26/01/2012 05:21, Lex Trotman wrote: On Thu, Jan 26, 2012 at 3:41 PM, Matthew Brushmbr...@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. ___ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] encoding combo boxes bug - utility functions
On 24/01/2012 03:30, Matthew Brush wrote:
I probably don't know 40%+ of Geany's code after casually hacking on it
for well over a year. When reading the code, I have to refer to the
source file for each function called to see what it does, with GTK+ I've
already done this for many cases, and know what it does. When writing
the code, I have to first write it in normal GTK+ and then go through
and make sure I haven't used any functions that are wrapped in the Geany
API/headers and even other static functions in the same file. It sounds
trivial if you are intimate with the source code, but if you aren't it
can make understanding the code you need to understand in order to fix a
bug or add a feature that much harder to follow.
If the function and its parameters are well named this isn't a big problem.
then that's a significant benefit in avoiding temporary variables or
nested expressions, which are harder to read. As I said, if the function
is obvious, there's no harm.
You don't really avoid temp vars, you just put them in another file. And
Eh? You have *one* copy of the temp var, not e.g. 10.
if an expression is only nested one or two levels deep, it's easier (at
least for me) in many cases to read if the code is inline.
For a (fictional) example:
void some_func(void)
{
GError *err=NULL;
if (!g_some_func(..., err))
{
printf (error: %s\n, err-message);
g_error_free (err-message);
}
...
}
is easier for me to read than:
/* misc.h */
#define EZG(...) { ... actual code from above ... }
separate files
/* some.c */
void some_func(void)
{
EZG(g_some_func, ...);
...
}
Even if it saves you repeating that same 5-6 line idiom a thousand times
throughout the source, unless you wrote both pieces of code, or unless
EZG() is in a well know API like GTK+, then it makes the code harder to
read, IMO, which many more people do many more times than writing it.
When have I ever suggested doing that?
___
Geany-devel mailing list
Geany-devel@uvena.de
https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] encoding combo boxes bug - utility functions
[...] Hi Matthew, While in general I agree with you, your examples are of mixed accuracy, see below: [1] Just at a very quick scan through utils.c, things like utils_slist_remove_next() - local static used one place, agree no reason to exist utils_is_uri() - good utility function, well named utils_string_replace() - probably should be static, only used several times in utils itself utils_spawn_async() - I think was used more than one place in the past, also hides the messy #ifdef windoze which is good utils_build_path() - g_build_filename() has better portability semantics, should replace utils_build_path() utils_make_filename() - reasonable utility function, probably should be used in more places where filename.ext concat is done explicitly Cheers Lex ___ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] encoding combo boxes bug - utility functions
On 01/25/2012 06:45 PM, Lex Trotman wrote: [...] Hi Matthew, While in general I agree with you, your examples are of mixed accuracy, see below: [1] Just at a very quick scan through utils.c, things like utils_slist_remove_next() - local static used one place, agree no reason to exist 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. utils_string_replace() - probably should be static, only used several times in utils itself 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). utils_build_path() - g_build_filename() has better portability semantics, should replace utils_build_path() Yep, why I listed it. 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. Having this seems weird to me, despite it being mildly useful and having good doc comment, since the name isn't great and the two things it does are so commonly available separately already in GLIB. But anyway, I made this list in 2 minutes scanning utils.c, so possibly not the best examples. Cheers, Matthew Brush ___ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] encoding combo boxes bug - utility functions
On Thu, Jan 26, 2012 at 3:41 PM, Matthew Brush mbr...@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 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? [...] 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. [...] 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. See point above. Having this seems weird to me, despite it being mildly useful and having good doc comment, since the name isn't great and the two things it does are so commonly available separately already in GLIB. But they are still two things not just one :) But anyway, I made this list in 2 minutes scanning utils.c, so possibly not the best examples. Sure, there are certainly some that could go, thats evolution of code (or code rot if you like) but it is often going to be a judgement call, so I guess the right path is so you and Nick are equally happy or unhappy :-D Cheers Lex ___ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] encoding combo boxes bug - utility functions
On 01/23/2012 08:30 AM, Nick Treleaven wrote:
On 22/01/2012 22:00, Lex Trotman wrote:
When working with a common well known library like GTK it is better to
use the well known interface directly rather than creating private
partial wrappers.
Contributors who know GTK don't have to learn the private interface
(or complain about what is missing, or just use GTK directly anyway
since they don't know about the private interface) and contributors
who don't know GTK can learn an interface that is useful to them
elsewhere, rather than one that just works in Geany.
You make a valid point, but most contributions are from the core team
that know our utility functions. In this case we're discussing a fairly
trivial function, but if it gets used 10 or 50 times in the code base
I probably don't know 40%+ of Geany's code after casually hacking on it
for well over a year. When reading the code, I have to refer to the
source file for each function called to see what it does, with GTK+ I've
already done this for many cases, and know what it does. When writing
the code, I have to first write it in normal GTK+ and then go through
and make sure I haven't used any functions that are wrapped in the Geany
API/headers and even other static functions in the same file. It sounds
trivial if you are intimate with the source code, but if you aren't it
can make understanding the code you need to understand in order to fix a
bug or add a feature that much harder to follow.
then that's a significant benefit in avoiding temporary variables or
nested expressions, which are harder to read. As I said, if the function
is obvious, there's no harm.
You don't really avoid temp vars, you just put them in another file. And
if an expression is only nested one or two levels deep, it's easier (at
least for me) in many cases to read if the code is inline.
For a (fictional) example:
void some_func(void)
{
GError *err=NULL;
if (!g_some_func(..., err))
{
printf (error: %s\n, err-message);
g_error_free (err-message);
}
...
}
is easier for me to read than:
/* misc.h */
#define EZG(...) { ... actual code from above ... }
separate files
/* some.c */
void some_func(void)
{
EZG(g_some_func, ...);
...
}
Even if it saves you repeating that same 5-6 line idiom a thousand times
throughout the source, unless you wrote both pieces of code, or unless
EZG() is in a well know API like GTK+, then it makes the code harder to
read, IMO, which many more people do many more times than writing it.
In other cases we have functions that save 10 lines of code per call.
This is a massive help that outweighs having to work out what the
function does.
+1.
Another point is that exposing Matt's ui_get_builder before we actually
have code that needs it seems premature. We already know we need to
lookup objects though, and that a short syntax is needed.
It's the same thing, you still expose one function to use, but with
ui_get_builder() you get the entirety of the GtkBuilder API for free and
never have to add another wrapper function for it. If you have
ui_builder_get_object(), if you need another function from the
GtkBuilder API, then you need to add another function, like
ui_builder_add_object(). Now you have two specialty functions functions,
both wrapping ones that are already included with gtk.h.
But anyway, the current function is so trivial, and I know everyone has
a different preference about these things, it's just my two cents...
Cheers,
Matthew Brush
___
Geany-devel mailing list
Geany-devel@uvena.de
https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
