alex wrote:
> Hmm, slight bug in src/ui/gtk/gtk2/bitzi.c, the table was missing some
> comas :-(
To prevent such bugs, it's best to use STATIC_ASSERT(). I guess the
number of elements depends on some magic value?
> - g_message("on_search_meta_data_active: called");
> + "Unknown", /* UNKNOWN */
> + "Dangerous/Misleading", /* DANGEROUS_MISLEADING */
> + "Incomplete/Damaged", /* INCOMPLETE_DAMAGED */
> + "Substandard", /* SUBSTANDARD */
> + "Overrated", /* OVERRATED */
> + "Normal", /* NORMAL */
> + "Underrated", /* UNDERRATED */
> + "Complete", /* COMPLETE */
> + "Recommended", /* RECOMMENDED */
> + "Best Version" /* BEST_VERSION*/
> +};
As these strings are user-visible they should should be marked translatable
which is is simple as this: N_("Recommended"). The same applies to any
other strings (except debug messages or warnings) presented to the user.
> - /* collect the list of files selected */
> +#define BITZI_MAX_FJ (sizeof(bitzi_fj_table)/sizeof(char *))
There's already the G_N_ELEMENTS() macro which can be used for the
above expression. This better because the meaning is more obvious
and you don't forget the '*' or mess with the types. Using (char *)
instead of sizeof bitzi_fj_table[0] is a bad idea anyway.
>
> - search = search_gui_get_current_search();
> - g_assert(search != NULL);
> +gchar * bitzi_fjtostring(bitzi_fj_t fj)
> +{
> + g_assert(fj<BITZI_MAX_FJ);
Is bitzi_fj_t signed or unsigned? It's best to also assert that fj
is equal or greater than zero.
> + return bitzi_fj_table[fj];
> +}
If you don't need the table anywhere else, it should be put into the
function. You should use a const qualifier for the table as long as you
don't want to modify it at run-time. In the complete patch I've noticed that
you use a similar table elsewhere. It's better to re-use this one here
instead of using another one.
> -#endif /* HAS_LIBXML2 */
Hmm, if memory serves, it was decided that libxml2 is actually mandatory,
so HAS_LIBXML2 is obsolete.
Looking at your complete patch, I've noticed the following:
- There's a bug in bitzi_heartbeat().
- Please, stick to the style rules especially with respect to spaces
before and after operators but also the position of curly braces. If
you don't fix those, someone else has to this which is no fun.
- IMHO, it's better to nest code as little as possible i.e., it's usually
easier to read and you safe space by not using this
void
blah() {
if (x) {
if (y) {
...
}
}
}
but rather this:
void
blah() {
if (!x) {
...
return;
}
if (!y) {
...
return;
}
...
}
There are exceptions to every rule, of course.
- Use G_FREE_NULL() instead of g_free() even if it might look ridiculous
in straight-forward code. It's better to laugh now than crying later.
- You use guint32 for the filesize. This is in line with the rest of the
code but it's bad, very bad. There are several options for gtk-gnutella:
- Use guint64 for filesizes: I don't want to and won't fix the code in
20 years.
- Use off_t: Somewhat odd because it refers to offsets but actually
the Right Thing (TM) as it's used by lseek() for example and has
exactly the right bit widths. Of course, this requires large file
support but this should really be mandatory in this millennium.
- Use uintmax_t to be on the safe side.
- Use something own like filesize_t which can "easily" adjusted
whenever necessary.
- No, size_t is the wrong type.
For printing you either want to force 64-bit that is cast to guint64
and use "%" PRIu64 or use an appropriate macro like FILESIZE_FMT which
has to be defined to whatever matches the variable type.
- Writing "No data yet" into the treestore might look nice from a user
perspective but you should keep in mind that GTK+ will g_strdup() this
string and will not use atoms. Thus you waste a couple of memory and
what's worse performance for drawing the string (Pango is sloooow).
As long as gtk-gnutella doesn't use a dedicated treemodel which uses
atoms (which doesn't solve the Pango performance brake), you should
rather pass a NULL pointer.
- You don't need g_strdup_printf() for "%s", "blah %s" or "blah". Just
use g_strdup() which is faster and safer.
- In search_gui_metadata_update() you shouldn't copy the default string.
As above, I would recommend using NULL instead. If you still want to
use a default string, initialize "text" to NULL and pass a string
literal to g_tree_store_set() like this:
text ? text : "blah"
- You use atom_.*_get() without using its return value. This might
incidentially work in your case iff the passed pointer is already
atomized but you should really use the return value. If you somehow
rely that both pointers are identical you should use g_assert().
- In bitzi_data_create() you allocate a new memory chunk and then
initialize each single element. First, this looks like a good
candidate for walloc(). Second, you should rather use
"static const bitzi_data_default = { ... }" and make use of
C's struct copy feature (or a plain memcpy - whatever floats your
boat). This way, you can benefit from compiler diagnostics i.e.,
"missing initializer" whenever you add more elements but might
forget to update the constructor.
- For bitzi_cache_hashtable (could be a little shorter like
bitzi_cache_ht), I think you should rely on the sha1 being an atom
and simply pass NULL as hash and comparison function.
- The bitzi URL shouldn't be inline in the code but rather at the
top of some file as macro or static const string literal. Since
the URL is rather short and has a fixed length, I would vote for using
a fixed size buffer instead of g_strdup_printf().
- Not horribly important, but I mention it anyway: It looks like bitzi_rq
should rather be a hashlist_t instead of a GSList as you make use of
g_slist_append() which is O(n). This might be overkill and just caching
the last element might be completely sufficient. gtk-gnutella needs
more high-level lists anyway.
Nonetheless, the rest of your patch looks fine. I haven't tried it yet
but I'm sure it'll be greatly appreciated by all users once integrated.
--
Christian
pgpvdDFyZm1aG.pgp
Description: PGP signature
