Re: [Geany-devel] encoding combo boxes bug - utility functions
On Thu, Jan 26, 2012 at 3:41 PM, Matthew Brush 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/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
[...] 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 08:01 AM, Nick Treleaven wrote:
On 25/01/2012 13:19, Nick Treleaven wrote:
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.
BTW I think you don't need to worry about not using the utility
functions, if the equivalent code is not too bad.
Good to know :)
Out of interest, which functions are the most annoying, any in particular?
It's mostly the little ones in utils/ui_utils[1] that wrap common C/GTK+
stuff, like the last two I whined about lately, or as we discussed a
while back, single use static functions that some people find harder to
read compared to putting that code into the function that uses it.
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?
I may have overreacted there, sorry ;-)
Not at all, it was a bad example indeed, I just wanted some code to show
where the code is obscured by being in another function and file, that's
the only purpose of the example.
I think your EZG macro was a bad example, because:
Yeah, it was just a quick example off the top of my head, I truly
shouldn't have put a macro in the example, since a function would've
shown what I meant without being absurd.
* it has a bad name (I accept NZV has this problem)
Heh, that's why I chose that name :)
Cheers,
Matthew Brush
[1] Just at a very quick scan through utils.c, things like
utils_slist_remove_next(), utils_is_uri(), utils_string_replace(),
utils_spawn_async()*, utils_build_path(), utils_make_filename(), and so on.
* might be needed for win32 or something (shouldn't though)?
___
Geany-devel mailing list
Geany-devel@uvena.de
https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] GIT commit mails format
On Wed, 25 Jan 2012 16:09:53 + Nick Treleaven wrote: > On 22/01/2012 15:50, Enrico Tröger wrote: > >> > Ok, so I'll change the hook at Github soon and we get some kind > >> > of live test:). > > Done. > > Any new commit mails will be sent from the script (which I will > > publish in some repository). > > Just pushed two commits - they got split into two mails and include > the diffs - thanks :-) Yepp. Liked that too. Cheers, Frank -- http://frank.uvena.de/en/ pgpwpsi7Ut4kL.pgp Description: PGP signature ___ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] GIT commit mails format
On 22/01/2012 15:50, Enrico Tröger wrote: > Ok, so I'll change the hook at Github soon and we get some kind of live > test:). Done. Any new commit mails will be sent from the script (which I will publish in some repository). Just pushed two commits - they got split into two mails and include the diffs - thanks :-) Nick ___ 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 25/01/2012 13:19, Nick Treleaven wrote:
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.
BTW I think you don't need to worry about not using the utility
functions, if the equivalent code is not too bad.
Out of interest, which functions are the most annoying, any in particular?
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?
I may have overreacted there, sorry ;-)
I think your EZG macro was a bad example, because:
* it has a bad name (I accept NZV has this problem)
* it would only work for functions like g_some_func - it's not general
enough
Something like this would be better:
#define HANDLE_ERROR(err)\
printf ("error: %s\n", err->message);\
g_error_free (err);
But it would have to be used frequently to be justifiable, and I
wouldn't put it in the plugin API, probably not even in a header. Also
usually the error format string would be more specific - rather than add
another parameter I would say at that point HANDLE_ERROR becomes not
worthwhile.
I realize now that in general we should probably avoid macros.
Nick
___
Geany-devel mailing list
Geany-devel@uvena.de
https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Re: [Geany-devel] glade diffs - Re: encoding combo boxes bug
On 22/01/2012 13:01, Nick Treleaven wrote: On 20/01/2012 18:39, Colomban Wendling wrote: @Nick: how did you generate the Glade file for 21cd7bb2139fd67644e5777bb8e6387d34473d09 "Add Project overrides for 'Saving files' checkbox options"? When I do use Glade 3.8.1 do modify the file it keeps reordering prefs/project dialogs... Just wondering, actually committing the move isn't really harmful -- apart that it renders the diff unreadable, stupid Glade. You're right, sadly my idea of using the same version of Glade doesn't prevent huge unreadable diffs. I committed 'regenerate with Glade 3.8.1' as an experiment, which failed. BTW unnecessarily large diffs are a real problem for another reason - they make a merge conflict much more likely. Ideally Glade would fix this reordering behaviour as it's bound to cause us trouble sooner or later. ___ 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
