GNOME Bugzilla – Bug 765032
Check for existing factory in registered_sources
Last modified: 2016-04-20 12:09:58 UTC
check for existing factory with that name before appending in registered_sources to avoid duplicate sources.
Created attachment 325898 [details] [review] Check for existing factory in registered_sources check for existing factory with that name before appending in registered_sources
Why not. But there are MANY problems in the implementation (please, please read your patches - the ratio of problems per modified line is really high): diff --git a/champlain/champlain-map-source-factory.c b/champlain/champlain-map-source-factory.c index 6f9a089..f349d4c 100644 --- a/champlain/champlain-map-source-factory.c +++ b/champlain/champlain-map-source-factory.c @@ -556,16 +556,32 @@ champlain_map_source_factory_create_error_source (ChamplainMapSourceFactory *fac * * Since: 0.10 */ You must leave the docstring above the function to which it refers - this will break documentation generation and introspection. +gint +idcmp (ChamplainMapSourceDesc *a, + ChamplainMapSourceDesc *b) Use some nicer name like compare_id() - also both parameters can be placed on a single line. +{ + const gchar *id_a, *id_b; + + id_a = champlain_map_source_desc_get_id (a); + id_b = champlain_map_source_desc_get_id (b); + + if (id_a == id_b) return 0; This is wrong - you are comparing pointers instead of strings. Use g_strcmp0(). + else return 1; +} + Use two blank lines between functions gboolean champlain_map_source_factory_register (ChamplainMapSourceFactory *factory, ChamplainMapSourceDesc *desc) { - /* FIXME: check for existing factory with that name? */ - factory->priv->registered_sources = g_slist_append (factory->priv->registered_sources, desc); + if(!g_slist_find_custom (factory->priv->registered_sources, desc, (GCompareFunc) idcmp)) + { + factory->priv->registered_sources = g_slist_append (factory->priv->registered_sources, desc); + } return TRUE; I guess you should return FALSE if the source already exists because the registration doesn't succeed - see the docstring. } + Remove the extra line you added. static ChamplainMapSource * champlain_map_source_new_generic (ChamplainMapSourceDesc *desc) { diff --git a/champlain/champlain-map-source-factory.h b/champlain/champlain-map-source-factory.h index ff4f9fc..9963a39 100644 --- a/champlain/champlain-map-source-factory.h +++ b/champlain/champlain-map-source-factory.h @@ -87,6 +87,8 @@ ChamplainMapSource *champlain_map_source_factory_create_memcached_source (Champl ChamplainMapSource *champlain_map_source_factory_create_error_source (ChamplainMapSourceFactory *factory, guint tile_size); +gint idcmp (ChamplainMapSourceDesc *a, + ChamplainMapSourceDesc *b); Why did you put it into the header? There's no need to export this function. gboolean champlain_map_source_factory_register (ChamplainMapSourceFactory *factory, ChamplainMapSourceDesc *desc); GSList *champlain_map_source_factory_get_registered (ChamplainMapSourceFactory *factory); -- 2.5.5
(In reply to Jiri Techet from comment #2) > Why not. But there are MANY problems in the implementation (please, please > read your patches - the ratio of problems per modified line is really high): Sorry for the mess, I will check for the problems from now onwards. I have not contributing to any open-source projects this is my first so I am still in the learning phase. I will submit the corrected patch > diff --git a/champlain/champlain-map-source-factory.c > b/champlain/champlain-map-source-factory.c > index 6f9a089..f349d4c 100644 > --- a/champlain/champlain-map-source-factory.c > +++ b/champlain/champlain-map-source-factory.c > @@ -556,16 +556,32 @@ champlain_map_source_factory_create_error_source > (ChamplainMapSourceFactory *fac > * > * Since: 0.10 > */ > > > You must leave the docstring above the function to which it refers - this > will break documentation generation and introspection. > > > +gint > +idcmp (ChamplainMapSourceDesc *a, > + ChamplainMapSourceDesc *b) > > Use some nicer name like compare_id() - also both parameters can be placed > on a single line. > > +{ > + const gchar *id_a, *id_b; > + > + id_a = champlain_map_source_desc_get_id (a); > + id_b = champlain_map_source_desc_get_id (b); > + > + if (id_a == id_b) return 0; > > > This is wrong - you are comparing pointers instead of strings. Use > g_strcmp0(). > > > + else return 1; > +} > + > > > Use two blank lines between functions > > > gboolean > champlain_map_source_factory_register (ChamplainMapSourceFactory *factory, > ChamplainMapSourceDesc *desc) > { > - /* FIXME: check for existing factory with that name? */ > - factory->priv->registered_sources = g_slist_append > (factory->priv->registered_sources, desc); > + if(!g_slist_find_custom (factory->priv->registered_sources, desc, > (GCompareFunc) idcmp)) > + { > + factory->priv->registered_sources = g_slist_append > (factory->priv->registered_sources, desc); > + } > return TRUE; > > > I guess you should return FALSE if the source already exists because the > registration doesn't succeed - see the docstring. > I thought in both the cases the source is registered, so we should send TRUE which indicates the presence of source in the array. > } > > > + > > Remove the extra line you added. > > static ChamplainMapSource * > champlain_map_source_new_generic (ChamplainMapSourceDesc *desc) > { > diff --git a/champlain/champlain-map-source-factory.h > b/champlain/champlain-map-source-factory.h > index ff4f9fc..9963a39 100644 > --- a/champlain/champlain-map-source-factory.h > +++ b/champlain/champlain-map-source-factory.h > @@ -87,6 +87,8 @@ ChamplainMapSource > *champlain_map_source_factory_create_memcached_source (Champl > ChamplainMapSource *champlain_map_source_factory_create_error_source > (ChamplainMapSourceFactory *factory, > guint tile_size); > > +gint idcmp (ChamplainMapSourceDesc *a, > + ChamplainMapSourceDesc *b); > > > Why did you put it into the header? There's no need to export this function. > > > gboolean champlain_map_source_factory_register (ChamplainMapSourceFactory > *factory, > ChamplainMapSourceDesc *desc); > GSList *champlain_map_source_factory_get_registered > (ChamplainMapSourceFactory *factory); > -- > 2.5.5
Created attachment 326003 [details] [review] Check for existing factory in registered_sources check for existing factory with that name before appending in registered_sources
> Sorry for the mess, I will check for the problems from now onwards. I have not contributing to any open-source projects this is my first so I am still in the learning phase. I will submit the corrected patch No problem, sorry for being too strict. In general, it's always best to read the existing code and follow the style as close as possible and not introduce any unnecessary changes. To the patch: +gint Add "static" before gint - this way the function will be local to the file. +compare_id (ChamplainMapSourceDesc *a, ChamplainMapSourceDesc *b) +{ + const gchar *id_a, *id_b; + + id_a = champlain_map_source_desc_get_id (a); + id_b = champlain_map_source_desc_get_id (b); + + if (g_strcmp0 (id_a, id_b) == 0) return 0; + else return 1; These two lines can be replaced by return g_strcmp0 (id_a, id_b);
Created attachment 326075 [details] [review] Check for existing factory in registered_sources check for existing factory with that name before appending in registered_sources
(In reply to Jiri Techet from comment #5) > No problem, sorry for being too strict. In general, it's always best to read > the existing code and follow the style as close as possible and not > introduce any unnecessary changes. You were right in doing so, that patch was really bad. But I learnt from that and won't repeat the same mistakes again.
Looks good now, thanks.