Hi, I'm seeing wrong encoding names in the encoding combo boxes on the Files tab of the Prefs dialog. E.g. where it should say UTF-8 I see Hebrew. Another Glade-related bug?
Nick
On Thu, Jan 19, 2012 at 11:53 PM, Nick Treleaven nick.treleaven@btinternet.com wrote:
Hi, I'm seeing wrong encoding names in the encoding combo boxes on the Files tab of the Prefs dialog. E.g. where it should say UTF-8 I see Hebrew. Another Glade-related bug?
Sort of, according to glade combo_new_encoding, combo_open_encoding and combo_eol all share the same underlying list model.
So when we initialise them, all the entries go in the same list, so all three show the encodings twice and the end of line entries at the bottom.
Two of these need to be pointed to different list models.
I don't have Glade 3.8.1 (need 3.10 for another project) so I shouldn't do it, someone with the right Glade care to create two new list models.
Cheers Lex
Nick _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Fri, Jan 20, 2012 at 9:58 AM, Lex Trotman elextr@gmail.com wrote:
On Thu, Jan 19, 2012 at 11:53 PM, Nick Treleaven nick.treleaven@btinternet.com wrote:
Hi, I'm seeing wrong encoding names in the encoding combo boxes on the Files tab of the Prefs dialog. E.g. where it should say UTF-8 I see Hebrew. Another Glade-related bug?
Sort of, according to glade combo_new_encoding, combo_open_encoding and combo_eol all share the same underlying list model.
So when we initialise them, all the entries go in the same list, so all three show the encodings twice and the end of line entries at the bottom.
Two of these need to be pointed to different list models.
PS the two encodings combos could probably share the same list, so long as we only initialise it once in prefs.c, but eol needs its own list
Cheers Lex
I don't have Glade 3.8.1 (need 3.10 for another project) so I shouldn't do it, someone with the right Glade care to create two new list models.
Cheers Lex
Nick _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Le 20/01/2012 00:07, Lex Trotman a écrit :
On Fri, Jan 20, 2012 at 9:58 AM, Lex Trotmanelextr@gmail.com wrote:
On Thu, Jan 19, 2012 at 11:53 PM, Nick Treleaven nick.treleaven@btinternet.com wrote:
Hi, I'm seeing wrong encoding names in the encoding combo boxes on the Files tab of the Prefs dialog. E.g. where it should say UTF-8 I see Hebrew. Another Glade-related bug?
Sort of, according to glade combo_new_encoding, combo_open_encoding and combo_eol all share the same underlying list model.
So when we initialise them, all the entries go in the same list, so all three show the encodings twice and the end of line entries at the bottom.
Two of these need to be pointed to different list models.
PS the two encodings combos could probably share the same list, so long as we only initialise it once in prefs.c, but eol needs its own list
Thanks for the catch & analysis, it's now fixed -- actually I did broke it in ca922e0ddc8022283ec3c1f49aaa15ab7c5ba213, stupid me.
@All: I added ui_builder_get_object() to be able to fetch a non-widget (list store here) from prefs.c, but I'm not completely sure it's the prefect fix. If you have any idea on how to improve this, spread them! :)
@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.
Regards, Colomban
Cheers Lex
I don't have Glade 3.8.1 (need 3.10 for another project) so I shouldn't do it, someone with the right Glade care to create two new list models.
Cheers Lex
Nick _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On 01/20/2012 10:39 AM, Colomban Wendling wrote:
Le 20/01/2012 00:07, Lex Trotman a écrit :
On Fri, Jan 20, 2012 at 9:58 AM, Lex Trotmanelextr@gmail.com wrote:
On Thu, Jan 19, 2012 at 11:53 PM, Nick Treleaven nick.treleaven@btinternet.com wrote:
Hi, I'm seeing wrong encoding names in the encoding combo boxes on the Files tab of the Prefs dialog. E.g. where it should say UTF-8 I see Hebrew. Another Glade-related bug?
Sort of, according to glade combo_new_encoding, combo_open_encoding and combo_eol all share the same underlying list model.
So when we initialise them, all the entries go in the same list, so all three show the encodings twice and the end of line entries at the bottom.
Two of these need to be pointed to different list models.
PS the two encodings combos could probably share the same list, so long as we only initialise it once in prefs.c, but eol needs its own list
Thanks for the catch & analysis, it's now fixed -- actually I did broke it in ca922e0ddc8022283ec3c1f49aaa15ab7c5ba213, stupid me.
@All: I added ui_builder_get_object() to be able to fetch a non-widget (list store here) from prefs.c, but I'm not completely sure it's the prefect fix. If you have any idea on how to improve this, spread them! :)
IMO, it'd be better to just move the builder object to the header file (maybe in a suitable struct), so that all files can access it. Then there's no need to add 1 line wrapper functions for every function we use from GtkBuilder's API. This isn't unprecedented, I think there's at least a handful of globals like this in Geany already (even in ui_utils.h). Alternatively, we could add a function called `ui_get_builder()` to get access to the builder to use with GtkBuilder API.
Otherwise, it's not too big of a deal, maybe we don't need much more from the GtkBuilder API.
Cheers, Matthew Brush
On 20/01/2012 21:29, Matthew Brush wrote:
@All: I added ui_builder_get_object() to be able to fetch a non-widget (list store here) from prefs.c, but I'm not completely sure it's the prefect fix. If you have any idea on how to improve this, spread them! :)
IMO, it'd be better to just move the builder object to the header file (maybe in a suitable struct), so that all files can access it. Then there's no need to add 1 line wrapper functions for every function we use from GtkBuilder's API. This isn't unprecedented, I think there's at least a handful of globals like this in Geany already (even in ui_utils.h). Alternatively, we could add a function called `ui_get_builder()` to get access to the builder to use with GtkBuilder API.
Otherwise, it's not too big of a deal, maybe we don't need much more from the GtkBuilder API.
I think Colomban's function is fine. I don't understand avoiding adding functions that are obviously useful and cleaner:
obj = ui_builder_get_object("name");
vs.
obj = gtk_builder_get_object(ui_get_builder(), "name");
The former is easier to read and it's obvious what it does.
On 01/22/2012 04:59 AM, Nick Treleaven wrote:
On 20/01/2012 21:29, Matthew Brush wrote:
@All: I added ui_builder_get_object() to be able to fetch a non-widget (list store here) from prefs.c, but I'm not completely sure it's the prefect fix. If you have any idea on how to improve this, spread them! :)
IMO, it'd be better to just move the builder object to the header file (maybe in a suitable struct), so that all files can access it. Then there's no need to add 1 line wrapper functions for every function we use from GtkBuilder's API. This isn't unprecedented, I think there's at least a handful of globals like this in Geany already (even in ui_utils.h). Alternatively, we could add a function called `ui_get_builder()` to get access to the builder to use with GtkBuilder API.
Otherwise, it's not too big of a deal, maybe we don't need much more from the GtkBuilder API.
I think Colomban's function is fine. I don't understand avoiding adding functions that are obviously useful and cleaner:
obj = ui_builder_get_object("name");
vs.
obj = gtk_builder_get_object(ui_get_builder(), "name");
The former is easier to read and it's obvious what it does.
But the latter exposes the full functionality of the GtkBuilder API without us having to maintain but a single function.
For example, consider the following:
GtkBuilder *builder = ui_get_builder(); gtk_builder_add_from_string (builder, TOOLBAR_XML, -1, NULL); ...
I basically just don't think it's worth maintaining a thin wrapper around common C/GTK+ code/idioms making our own "framework" to save a line or two of code here and there. Unless you wrote the wrapper function yourself, it makes the code harder to read, IMO.
Cheers, Matthew Brush
On Mon, Jan 23, 2012 at 1:45 AM, Matthew Brush mbrush@codebrainz.ca wrote:
On 01/22/2012 04:59 AM, Nick Treleaven wrote:
On 20/01/2012 21:29, Matthew Brush wrote:
@All: I added ui_builder_get_object() to be able to fetch a non-widget (list store here) from prefs.c, but I'm not completely sure it's the prefect fix. If you have any idea on how to improve this, spread them! :)
IMO, it'd be better to just move the builder object to the header file (maybe in a suitable struct), so that all files can access it. Then there's no need to add 1 line wrapper functions for every function we use from GtkBuilder's API. This isn't unprecedented, I think there's at least a handful of globals like this in Geany already (even in ui_utils.h). Alternatively, we could add a function called `ui_get_builder()` to get access to the builder to use with GtkBuilder API.
Otherwise, it's not too big of a deal, maybe we don't need much more from the GtkBuilder API.
I think Colomban's function is fine. I don't understand avoiding adding functions that are obviously useful and cleaner:
obj = ui_builder_get_object("name");
vs.
obj = gtk_builder_get_object(ui_get_builder(), "name");
The former is easier to read and it's obvious what it does.
But the latter exposes the full functionality of the GtkBuilder API without us having to maintain but a single function.
For example, consider the following:
GtkBuilder *builder = ui_get_builder(); gtk_builder_add_from_string (builder, TOOLBAR_XML, -1, NULL); ...
I basically just don't think it's worth maintaining a thin wrapper around common C/GTK+ code/idioms making our own "framework" to save a line or two of code here and there. Unless you wrote the wrapper function yourself, it makes the code harder to read, IMO.
Cheers, Matthew Brush
Nick, Matthew, All,
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.
Cheers Lex
Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
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 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.
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.
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.
Nick
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
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?
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
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)?
[...] 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
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
On Thu, Jan 26, 2012 at 3:41 PM, Matthew Brush mbrush@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
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.
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.
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.