26 April 2009

Writing a nice API: comments needed

In the race to the 0.3 development release, we are reviewing the API to see it is nice to use, bindable and most of all intuitive to g* coders.  But sometimes, it is hard to find out how to do it well: comments on this particular issue would be appreciated.

ChamplainView is the map view that displays the map.  It needs a ChamplainMapSource from which it gets the map.  There are specialised objects that inherit from ChamplainMapSource such as ChamplainNetworkMapSource and the upcoming ChamplainLocalMapSource (or what ever it will be named by the end of the SoC).

champlainNetworkMapSource* champlain_network_map_source_new_full (const gchar *name,
    const gchar *license,
    const gchar *license_uri,
    guint min_zoom,
    guint map_zoom,
    guint tile_size,
    ChamplainMapProjection projection,
    const gchar *uri_format);

As you don’t want to fill all this information each time you create a new map source, libchamplain currently provides helper constructions:

ChamplainMapSource * champlain_map_source_new_osm_mapnik (void);
ChamplainMapSource * champlain_map_source_new_osm_cyclemap (void);
ChamplainMapSource * champlain_map_source_new_osm_osmarender (void);
ChamplainMapSource * champlain_map_source_new_oam (void);
ChamplainMapSource * champlain_map_source_new_mff_relief (void);

We think it could be interesting to replace these by a Factory to which you pass an enum value to get the constructed ChamplainMapSource.  You would probably be able to add your own map source constructor (as you can implement your own map sources).  You would probably be able to get the list of available map source too.

The question is: is this overkill? the best way to do it? is there something similar in glib or gtk to get inspired from? We base most of the API decisions by looking at other Gtk+ widgets, but this particular object seems to be a different case.

We are asking for your ideas.

Comments (11)

  1. 26 April 2009
    Vudentz said...

    what about a va_list alternative:

    ChamplainMapSource * champlain_map_source_new (enum first, …)

    Afaik either gobject and gtk uses va_list so that is not really new in gnome.

  2. 26 April 2009
    Pierre-Luc Beaudoin said...

    In fact, the question is more about: are the constructor functions just fine or is a factory needed to manage it?

  3. 27 April 2009
    Alexander Larsson said...

    I don’t think the its a good idea that apps call champlain_network_map_source_new_full themselves. However, this is not mostly because it is a lot to type, but rather that it hardcodes configuration in the app code. For instance, if the openstreetmap uri format ever changes any app compiled with the old version is broken, and updating the library would not fix it.

    I also think its a bad idea to encode what is more or less configuration settings in the API symbol names like “champlain_map_source_new_osm_mapnik”. This makes it hard to expand and impossible to change during runtime (via modules, config files, etc). Furthermore apps that actually use this are not likely to have the exact map hardcoded in the code, but rather have some sort of config option, so they need to call this with something like:

    if (config == osm)
    source = champlain_map_source_new_osm_mapnik ();
    else

    So, I think a better Idea is to have
    ChamplainMapSource * champlain_map_source_new_from_name (char *name);
    Where you ship with a few hardcoded names, and also could allow config files to specify other names (which then would automatically work on all apps using the lib.
    For UI configuration to work you also need a function to enumerate the available map names.

  4. 27 April 2009
    Ploum said...

    I can see a lot of benefits to a factory (like enabling/disabling some maps at compile time, easily implementing a new map, …).

    I don’t see any drawback so I don’t really understand your hesitation. I vote for the factory (and I’m really not a Factory pattern fan).

  5. 27 April 2009
    Guyou said...

    I’m not really involved with Gtk+ designs, but your “design problem” seems near to GtkTreeView & co. I think their solution is more… inheritance oriented.

    But there is an other way to study this problem. IMHO, the devel API is not really the matter. I’m quite sure that most of GUI using libchamplain will delegate the choice of the MapSource to the user.
    So, IMHO, the most important is an introspectable model, in which the software can enumerate any available (loaded?) MapSource.

    Furthermore, an other important aspect is to be able to add our own MapSource (for example, if I run my own OSM instance).

  6. 27 April 2009
    thp said...

    I think the best way would be to have some config dir in /etc/ where there is a file for each map source and these entries can then be read by the library. This way, a change in Web API URLs is not really a problem. Also, it allows for providing a “additional maps” package that simply installs some config files in the config dir in /etc/.

    Hardcoding URLs and services with API functions like champlain_map_source_new_osm_mapnik() seems like a bad idea to me – if some services change it would mean re-compiling the library.

  7. 27 April 2009
    Ignacius said...

    The only one I can remember of is the one from gstreamer. You can take a look at it.

  8. 27 April 2009
    Potyl said...

    The GDK PixBuf API has a factory that returns the proper loader based on the mime-type or the image type to load [1]. There’s also a mechanism for querying the existing formats [2].

    [1] http://library.gnome.org/devel/gdk-pixbuf/stable/GdkPixbufLoader.html#gdk-pixbuf-loader-new-with-type
    [2] http://library.gnome.org/devel/gdk-pixbuf/stable/gdk-pixbuf-Module-Interface.html#gdk-pixbuf-get-formats

  9. 27 April 2009
    anon said...

    maybe do it like alsa does:

    obj_params *p;
    p = obj_params_new();
    obj_params_set_some_parameter (p, 42);
    obj_params_set_something_else (p, “asdf”);

    obj *o;
    o = obj_new (p);

  10. 4 May 2009
    Guyou said...

    Following what thp wrote, I also plan to add to viking a feature to build and register MapSource elements from an XML description (in the spirit of GtkBuilder). By this way, the user is fully capable to add the MapSource it really needs.

  11. 4 May 2009
    Pierre-Luc Beaudoin said...

    I should point out that a Factory has now been added to libchamplain: http://libchamplain.pierlux.com/doc/unstable/ChamplainMapSourceFactory.html

    Guillem it would still allow you to add your map source elements from an xml file!