*** Bug 148734 has been marked as a duplicate of this bug. ***

I'd like to add to this bug:
Remember the last used font and perhaps also the last chosen character.

Add the chosen Script to the list of settings to remember.
That'll force gucharmap to use gconf. It doesn't seem to use it now.

IMHO, should remember last window's position and size too.

My gucharmap is linked against libgconf-2.so.4 at least.

Nonsense:  Noah, now that you removed the Back/Forward buttons, how about adding
a "Recently viewed characters"?  I've got a feeling that that can be useful.

Behdad, shouldn't that be a separate bug report? :P (--Noah, hoping for somebody
new to jump in and start submitting patches...)

http://bugzilla.gnome.org/show_bug.cgi?id=163092

Created attachment 35709 [details] [review]
Adds GConf support to gucharmap

Stores the last used font, last chosen character, chosen script and snap to
power of two.

Created attachment 35750 [details] [review]
configure gconf support / schema install

Forgot to update configure.ac in the previous patch. It also prepares
Makefile.am for installation of gconf schema.

Created attachment 35751 [details]
GConf schema

Comment on attachment 35709 [details] [review]
Adds GConf support to gucharmap

>Index: Makefile.am
>===================================================================
>RCS file: /cvs/gnome/gucharmap/gucharmap/Makefile.am,v
>retrieving revision 1.66
>diff -u -3 -p -u -r1.66 Makefile.am
>--- Makefile.am	23 Mar 2004 06:11:33 -0000	1.66
>+++ Makefile.am	9 Jan 2005 04:02:25 -0000
>@@ -21,6 +21,7 @@ AM_CFLAGS = $(GTK_CFLAGS) $(GNOME_CFLAGS
> 
> localedir = $(datadir)/locale
> AM_CPPFLAGS = -I$(top_srcdir) \
>+              -DGCONF_PREFIX=\""/apps/gucharmap"\" \
>               -DDATADIR=\"$(datadir)\" \
>               -DLOCALEDIR=\"$(localedir)\" \
>               -DICON_PATH=\"$(datadir)/pixmaps/gucharmap.png\"
>Index: gucharmap-chapters.c
>===================================================================
>RCS file: /cvs/gnome/gucharmap/gucharmap/gucharmap-chapters.c,v
>retrieving revision 1.5
>diff -u -3 -p -u -r1.5 gucharmap-chapters.c
>--- gucharmap-chapters.c	2 Feb 2004 01:39:05 -0000	1.5
>+++ gucharmap-chapters.c	9 Jan 2005 04:03:56 -0000
>@@ -176,3 +176,63 @@ gucharmap_chapters_previous (GucharmapCh
>       gtk_tree_path_free (path);
>     }
> }
>+
>+/**
>+ * gucharmap_chapter_get_string:
>+ * @chapters: a #GucharmapChapters
>+ *
>+ * Returns the name of the currently selected chapter
>+ * The result must be freed with g_free()
>+ **/
>+char*
>+gucharmap_chapter_get_string(GucharmapChapters *chapters)
>+{
>+  GtkTreeSelection *selection = gtk_tree_view_get_selection (GTK_TREE_VIEW (chapters->tree_view));
>+  GtkTreeModel *model = gtk_tree_view_get_model (GTK_TREE_VIEW (chapters->tree_view));
>+  GtkTreeIter iter;
>+  gchar *name = NULL;
>+
>+  if (gtk_tree_selection_get_selected (selection, NULL, &iter))
>+    {
>+      gtk_tree_model_get(model, &iter, 0, &name, -1);
>+      return name;
>+    }
>+}
>+
>+/**
>+ * gucharmap_chapter_get_string:
>+ * @chapters: a #GucharmapChapters
>+ * @name: Unicode block name
>+ *
>+ * Sets the selection to the row specified by @name
>+ * Return value: %TRUE on success, %FALSE on failure
>+ **/
>+gboolean
>+gucharmap_chapter_set_string(GucharmapChapters *chapters, const char *name)
>+{
>+  GtkTreeModel *model = gtk_tree_view_get_model (GTK_TREE_VIEW (chapters->tree_view));
>+  GtkTreeIter iter;
>+  gboolean match;
>+  gchar *str;
>+
>+  if (gtk_tree_model_get_iter_first (model, &iter))
>+    {
>+      do
>+        {
>+	  gtk_tree_model_get(model, &iter, 0, &str, -1);
>+	  match = strcmp (name, str);
>+	  g_free(str);
>+	  if (0 == match)
>+	  {
>+	    GtkTreePath *path = gtk_tree_model_get_path (chapters->tree_model, &iter);
>+	    gtk_tree_view_set_cursor (GTK_TREE_VIEW (chapters->tree_view), path, NULL, FALSE);
>+	    gtk_tree_path_free (path);
>+	    return TRUE;
>+          }
>+	}
>+      while (gtk_tree_model_iter_next (model, &iter));
>+    }
>+
>+  return FALSE;
>+}
>+
>Index: gucharmap-chapters.h
>===================================================================
>RCS file: /cvs/gnome/gucharmap/gucharmap/gucharmap-chapters.h,v
>retrieving revision 1.4
>diff -u -3 -p -u -r1.4 gucharmap-chapters.h
>--- gucharmap-chapters.h	2 Feb 2004 00:03:49 -0000	1.4
>+++ gucharmap-chapters.h	9 Jan 2005 04:04:30 -0000
>@@ -68,6 +68,9 @@ gboolean                                
>                                                                                     gunichar           wc);
> void                                    gucharmap_chapters_next                    (GucharmapChapters *chapters);
> void                                    gucharmap_chapters_previous                (GucharmapChapters *chapters);
>+char*                                   gucharmap_chapter_get_string               (GucharmapChapters *chapters);
>+gboolean                                gucharmap_chapter_set_string               (GucharmapChapters *chapters,
>+                                                                                    const char *name);
> 
> G_END_DECLS
> 
>Index: gucharmap-window.c
>===================================================================
>RCS file: /cvs/gnome/gucharmap/gucharmap/gucharmap-window.c,v
>retrieving revision 1.53
>diff -u -3 -p -u -r1.53 gucharmap-window.c
>--- gucharmap-window.c	27 Feb 2004 18:11:01 -0000	1.53
>+++ gucharmap-window.c	9 Jan 2005 04:05:04 -0000
>@@ -23,6 +23,7 @@
> #include <string.h>
> #include <gtk/gtk.h>
> #include <gdk/gdkkeysyms.h>
>+#include <gconf/gconf-client.h>
> #if HAVE_GNOME
> # include <gnome.h>
> #endif
>@@ -61,6 +62,9 @@ struct _GucharmapWindowPrivate
> 
>   GtkWidget *file_menu_item;
>   GtkWidget *quit_menu_item;
>+  /* since script is default, only need to be able to set/signal this guy */
>+  GtkWidget *chapters_block_menu_item;
>+  GtkWidget *snap_cols_pow2_menu_item;
>   GtkWidget *next_chapter_menu_item;
>   GtkWidget *prev_chapter_menu_item;
> 
>@@ -80,6 +84,8 @@ struct _GucharmapWindowPrivate
>   gboolean file_menu_visible;
> 
>   ChaptersMode chapters_mode; 
>+
>+  GConfClient *gc;
> };
> 
> static void
>@@ -288,7 +294,12 @@ static void
> snap_cols_pow2 (GtkCheckMenuItem *mi, 
>                 GucharmapWindow  *guw)
> {
>+  GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>+
>   gucharmap_table_set_snap_pow2 (guw->charmap->chartable, gtk_check_menu_item_get_active (mi));
>+
>+  gconf_client_set_bool (priv->gc, GCONF_PREFIX"/snap_cols_pow2",
>+		  gtk_check_menu_item_get_active (mi), NULL);
> }
> 
> #if HAVE_GNOME
>@@ -410,13 +421,38 @@ prev_chapter (GtkWidget       *button,
> }
> 
> static void
>+script_changed (GtkTreeSelection *treeselection,
>+                 GucharmapWindow  *guw)
>+{
>+  GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>+  GucharmapChapters *chapters = gucharmap_charmap_get_chapters (guw->charmap);
>+  gchar *script;
>+
>+  if ((script = gucharmap_chapter_get_string(chapters)))
>+    {
>+      gconf_client_set_string (priv->gc, GCONF_PREFIX"/script", script, NULL);
>+      g_free(script);
>+    }
>+}
>+
>+static void
> view_by_script (GtkCheckMenuItem *check_menu_item,
>                 GucharmapWindow  *guw)
> {
>   if (gtk_check_menu_item_get_active (check_menu_item))
>     {
>       GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>+      GtkTreeSelection *sel;
>+
>+      gconf_client_set_int (priv->gc, GCONF_PREFIX"/chapters_mode",
>+		      CHAPTERS_SCRIPT, NULL);
>       gucharmap_charmap_set_chapters (guw->charmap, GUCHARMAP_CHAPTERS (gucharmap_script_chapters_new ()));
>+      sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (
>+			      (GUCHARMAP_CHAPTERS (
>+				gucharmap_charmap_get_chapters(guw->charmap))->tree_view)));
>+      g_signal_connect (sel, "changed", G_CALLBACK (script_changed), guw);
>+      g_signal_emit_by_name (sel, "changed", guw);
>+
>       gtk_label_set_text (GTK_LABEL (gtk_bin_get_child (GTK_BIN (priv->next_chapter_menu_item))), _("Next Script"));;
>       gtk_label_set_text (GTK_LABEL (gtk_bin_get_child (GTK_BIN (priv->prev_chapter_menu_item))), _("Previous Script"));;
>     }
>@@ -429,7 +465,17 @@ view_by_block (GtkCheckMenuItem *check_m
>   if (gtk_check_menu_item_get_active (check_menu_item))
>     {
>       GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>+      GtkTreeSelection *sel;
>+
>+      gconf_client_set_int (priv->gc, GCONF_PREFIX"/chapters_mode",
>+		      CHAPTERS_BLOCK, NULL);
>       gucharmap_charmap_set_chapters (guw->charmap, GUCHARMAP_CHAPTERS (gucharmap_block_chapters_new ()));
>+      sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (
>+			      (GUCHARMAP_CHAPTERS (
>+				gucharmap_charmap_get_chapters(guw->charmap))->tree_view)));
>+      g_signal_connect (sel, "changed", G_CALLBACK (script_changed), guw);
>+      g_signal_emit_by_name (sel, "changed", guw);
>+
>       gtk_label_set_text (GTK_LABEL (gtk_bin_get_child (GTK_BIN (priv->next_chapter_menu_item))), _("Next Block"));;
>       gtk_label_set_text (GTK_LABEL (gtk_bin_get_child (GTK_BIN (priv->prev_chapter_menu_item))), _("Previous Block"));;
>     }
>@@ -501,6 +547,7 @@ make_menu (GucharmapWindow *guw)
>   gtk_check_menu_item_set_active (GTK_CHECK_MENU_ITEM (menu_item), FALSE);
>   gtk_menu_shell_append (GTK_MENU_SHELL (view_menu), menu_item);
>   g_signal_connect (menu_item, "toggled", G_CALLBACK (view_by_block), guw);
>+  priv->chapters_block_menu_item = menu_item;
> 
>   /* separator */
>   gtk_menu_shell_append (GTK_MENU_SHELL (view_menu), gtk_menu_item_new ());
>@@ -508,6 +555,7 @@ make_menu (GucharmapWindow *guw)
>   menu_item = gtk_check_menu_item_new_with_label (_("Snap Columns to Power of Two"));
>   g_signal_connect (menu_item,  "activate", G_CALLBACK (snap_cols_pow2), guw);
>   gtk_menu_shell_append (GTK_MENU_SHELL (view_menu), menu_item);
>+  priv->snap_cols_pow2_menu_item = menu_item;
> 
>   /* separator */
>   gtk_menu_shell_append (GTK_MENU_SHELL (view_menu), gtk_menu_item_new ());
>@@ -650,9 +698,11 @@ static void
> fontsel_changed (GucharmapMiniFontSelection *fontsel, 
>                  GucharmapWindow            *guw)
> {
>+  GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>   gchar *font_name = gucharmap_mini_font_selection_get_font_name (fontsel);
> 
>   gucharmap_table_set_font (guw->charmap->chartable, font_name);
>+  gconf_client_set_string (priv->gc, GCONF_PREFIX"/font", font_name, NULL);
> 
>   g_free (font_name);
> }
>@@ -780,6 +830,15 @@ status_realize (GtkWidget       *status,
> }
> 
> static void
>+save_last_char (GtkWidget        *widget, 
>+                 gunichar          wc, 
>+                 GucharmapWindow *guw)
>+{
>+  GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>+  gconf_client_set_int (priv->gc, GCONF_PREFIX"/last_char", guw->charmap->chartable->active_cell, NULL);
>+}
>+
>+static void
> pack_stuff_in_window (GucharmapWindow *guw)
> {
>   GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>@@ -852,13 +911,27 @@ static void
> gucharmap_window_init (GucharmapWindow *guw)
> {
>   GucharmapWindowPrivate *priv = GUCHARMAP_WINDOW_GET_PRIVATE (guw);
>+  int w, h;
>+  gunichar last_char, wc;
>+  int font_size;
>+  GucharmapChapters *chapters;
>+  GtkTreeSelection *sel;
>+  char *script;
>+  gboolean snap_cols_pow2;
>+  char *fontname;
> 
>   gtk_window_set_title (GTK_WINDOW (guw), _("Character Map"));
> 
>+  if ((priv->gc = gconf_client_get_default ()) == NULL)
>+  {
>+    g_printerr(_("gucharmap couldn't initialise the configuration engine."), NULL);
>+    exit(1);
>+  }
>+
>   priv->font_selection_visible = FALSE;
>   priv->text_to_copy_visible = FALSE;
>   priv->file_menu_visible = FALSE;
>-  priv->chapters_mode = CHAPTERS_SCRIPT;
>+  priv->chapters_mode = gconf_client_get_int (priv->gc, GCONF_PREFIX"/chapters_mode", NULL);
> 
>   priv->search_dialog = NULL;
> 
>@@ -866,6 +939,48 @@ gucharmap_window_init (GucharmapWindow *
>   gtk_window_set_icon (GTK_WINDOW (guw), priv->icon);
> 
>   pack_stuff_in_window (guw);
>+
>+  if (priv->chapters_mode)
>+  {
>+    gtk_check_menu_item_set_active (GTK_CHECK_MENU_ITEM(
>+			    priv->chapters_block_menu_item), TRUE);
>+    gtk_check_menu_item_toggled (GTK_CHECK_MENU_ITEM(priv->chapters_block_menu_item));
>+  }
>+
>+  chapters = GUCHARMAP_CHAPTERS (gucharmap_charmap_get_chapters(guw->charmap));
>+  sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (chapters->tree_view));
>+  script = gconf_client_get_string (priv->gc, GCONF_PREFIX"/script", NULL);
>+  if (script)
>+    gucharmap_chapter_set_string (chapters, script);
>+  g_signal_connect (sel, "changed", G_CALLBACK (script_changed), guw);
>+
>+  snap_cols_pow2 = gconf_client_get_bool (priv->gc,
>+		  GCONF_PREFIX"/snap_cols_pow2", NULL);
>+  if (snap_cols_pow2)
>+  {
>+    gtk_check_menu_item_set_active (GTK_CHECK_MENU_ITEM(
>+			    priv->snap_cols_pow2_menu_item), TRUE);
>+    gtk_check_menu_item_toggled (GTK_CHECK_MENU_ITEM(priv->snap_cols_pow2_menu_item));
>+  }
>+
>+  last_char = gconf_client_get_int (priv->gc, GCONF_PREFIX"/last_char", NULL);
>+  wc = gucharmap_codepoint_list_get_char (guw->charmap->chartable->codepoint_list, last_char);
>+
>+  if (gucharmap_unichar_isdefined (wc) && gucharmap_unichar_validate (wc))
>+    guw->charmap->chartable->active_cell = last_char;
>+
>+  GucharmapMiniFontSelection *fontsel = GUCHARMAP_MINI_FONT_SELECTION(priv->fontsel);
>+  gint default_size = PANGO_PIXELS (2.0 * pango_font_description_get_size (gtk_widget_get_style (GTK_WIDGET (guw))->font_desc));
>+  gucharmap_mini_font_selection_set_default_font_size (fontsel, default_size);
>+
>+  fontname = gconf_client_get_string (priv->gc, GCONF_PREFIX"/font", NULL);
>+  if (fontname)
>+  {
>+    PangoFontDescription *fd = pango_font_description_from_string (fontname);
>+
>+    gucharmap_mini_font_selection_set_font_name (fontsel, fontname);
>+    /* revert to default font size */
>+    if (0 == pango_font_description_get_size(fd))
>+      gucharmap_mini_font_selection_reset_font_size (fontsel);
>+    pango_font_description_free(fd);
>+  }
>+
>+  /* don't want this to tell gconf to reset last_char to 0 */
>+  g_signal_connect (guw->charmap->chartable, "set-active-char", G_CALLBACK (save_last_char), guw);
> }
> 
> static void
>Index: main.c
>===================================================================
>RCS file: /cvs/gnome/gucharmap/gucharmap/main.c,v
>retrieving revision 1.69
>diff -u -3 -p -u -r1.69 main.c
>--- main.c	2 Feb 2004 00:48:05 -0000	1.69
>+++ main.c	9 Jan 2005 04:06:01 -0000
>@@ -32,11 +32,8 @@
> #include "gucharmap-window.h"
> #include "gucharmap-mini-fontsel.h"
> 
>-static gchar *new_font = NULL;
> static struct poptOption options[] = 
> { 
>-  { "font", '\0', POPT_ARG_STRING, &new_font, 0, 
>-    N_("Font to start with; ex: 'Serif 27'"), NULL },
> #if !HAVE_GNOME
>   POPT_AUTOHELP  /* gnome does this automatically */
> #endif
>@@ -89,15 +86,6 @@ main (gint argc, gchar **argv)
>   gdk_screen_get_monitor_geometry (screen, monitor, &rect);
>   gtk_window_set_default_size (GTK_WINDOW (window), rect.width * 9/16, rect.height * 9/16);
> 
>-  /* make the starting font 50% bigger than the default font */
>-  if (new_font == NULL) /* new_font could be set by command line option */
>-    {
>-      GucharmapMiniFontSelection *fontsel = gucharmap_window_get_mini_font_selection (GUCHARMAP_WINDOW (window));
>-      gint default_size = PANGO_PIXELS (2.0 * pango_font_description_get_size (window->style->font_desc));
>-      gucharmap_mini_font_selection_set_default_font_size (fontsel, default_size);
>-      gucharmap_mini_font_selection_reset_font_size (fontsel);
>-    }
>-
>   g_signal_connect (G_OBJECT (window), "destroy", G_CALLBACK (gtk_main_quit), NULL);
> 
>   gucharmap_table_grab_focus (GUCHARMAP_WINDOW (window)->charmap->chartable);
>

Thanks Jason, this works very nicely. It needs to be compiled out though if
--disable-gnome is specified. 

A couple of warnings you might want to fix:

gucharmap-chapters.c: In function `gucharmap_chapter_get_string':
gucharmap-chapters.c:200: warning: control reaches end of non-void function
gucharmap-chapters.c: In function `gucharmap_chapter_set_string':
gucharmap-chapters.c:223: warning: implicit declaration of function `strcmp'

In the future please cvs add new files (it only makes local changes) and use
diff -uN from the root directory of the project, so that just a single diff is
needed.

This can wait, but it would also be nice if it remembered window size (but not
location).

Created attachment 35865 [details] [review]
Updated patch, fixes warnings, saves window size

I couldn't cvs add the schema file:

$ cvs add gucharmap.schemas.in
cvs [server aborted]: "add" requires write access to the repository

I'm using anoncvs. I don't know the diff format for adding files, so I guessed
at it and put the updated schema at the end.

You better file a bug against CVS for that.  Basically only cvc ci should nag,
not cvs add or cvs remove.

Created attachment 36274 [details] [review]
New version

Created attachment 36613 [details] [review]
New new version

*** Bug 167053 has been marked as a duplicate of this bug. ***

a me too: please include a key to remember if "view by script" or "view by
unicode block" is selected.

A recurrent request: please make gucharmap remember if "view by script" or "view
by unicode block" was selected and show the apropriate block at startup.

*** Bug 156922 has been marked as a duplicate of this bug. ***

*** Bug 311912 has been marked as a duplicate of this bug. ***

Any plans to apply this for GNOME 2.12?
If nobody has the time, I can apply a couple patches and make a release.

Behdad, Jason Allen and I exchanged some email, and after his last patch I sent
him something asking for yet further major revisions. That was like the 10th
time, and I think I finally frustrated him. I did some work on it myself after
the last email but left off at some point, and I suspect the state it’s in right
now on my hard disk would be more confusing than useful. What I last said to
Jason was that I wanted to organize gucharmap-settings.c like this:

static gchar *
get_default_chapter ()
{
 /* XXX: In the future, do something based on chapters mode and
locale or something. */
 return NULL;
}

/* [... other get_default_* functions] */

#if HAVE_GNOME

gchar *
gucharmap_settings_get_chapter ()
{
 gchar *chapter;

 if (ensure_initialised ())
   chapter = gconf_client_get_string (settings.gc, GCONF_PREFIX
"/chapter", NULL);

 if (chapter)
   return chapter;
 else
   return get_default_chapter ();
}

/* [... other gconf-using functions] */

#else /* HAVE_GNOME */

gchar *
gucharmap_settings_get_chapter ()
{
 return get_default_chapter ()
}

/* [... other non-gconf-using functions] */

#endif /* !HAVE_GNOME  */


I’m thinking now, though, that maybe it’s ok to get rid of non-gnome support. If
you want to do that it’s ok with me too.

*** Bug 324939 has been marked as a duplicate of this bug. ***

Behnam, can you finish this?

The non-GNOME stuff is mostly used on small devices. doesn't hurt to be there...

*** Bug 341693 has been marked as a duplicate of this bug. ***

Is the code in such a state that it can be released? Most users don't care that the code would be (slightly) imperfect. Future releases could improve on this, but in the meanwhile a lot of people are waiting for this.

*** Bug 349659 has been marked as a duplicate of this bug. ***

I agree with Matthijs. In this case really suits Linus's mantra "perfect is the enemy of good"*. This feature request is _two and half_ year old and no solution was perfect enough to make it to main branch. Any working solution should be accepted to fix it. It may be eventually rewrited if it's code isn't nice enough. But there's one huge difference againt now - it would _work_.

* http://os.newsforge.com/article.pl?sid=05/06/09/2128249

Behnam, ping.

It is also up to the point for me to start searching for more usable solution and abandon gucharmap, since current situation is so perfect.

My only and last attempt to salvage it though, a port of Jason's patch will be attached, so that it works against HEAD. Indeed it has some rough edges and breaks non-gnome case, but looks like that shouldn't be hard to fix.

Created attachment 82551 [details] [review]
A port of Jason's patch to HEAD

+AC_PATH_PROG(GCONFTOOL, gconftool-2, no)
+if test "$GCONFTOOL" != "no"; then
+    AM_GCONF_SOURCE_2
+fi

AM_GCONF_SOURCE_2 uses AM_CONDITIONAL and must therefore not be called conditionally. It's safe to just always call it; it doesn't abort if gconf isn't found.

*** Bug 439497 has been marked as a duplicate of this bug. ***

Sort of wasting time.

*** Bug 455115 has been marked as a duplicate of this bug. ***

Unfortunately I must agree with David Jaša... this is so embarrasing for GNOME... what a simple feature request and there are even patches... but we still have to stick with the situation from 2004. lol....

To all the naysayers, if you care so much about GNOME, get your hands dirty and finish they patch.

*** Bug 460616 has been marked as a duplicate of this bug. ***

I'm currently implementing the changes requested in comment 24. Is that what we are waiting for?

Created attachment 93354 [details] [review]
separated HAVE_GNOME from !HAVE_GNOME as in comment 24

I didn't add all the get_default_XXX functions because they didn't seem to make sense, e.g. the default font is already defined elsewhere. Also I'm new to Autotools so I don't really understand comment 34 and therefore didn't do anything about it. Also changed `gu_return_if_fail(expr)' to use `g_return_if_fail(expr)', since the former seemed to be causing compile issues. 

I tried testing this but something seems to be broken with my Jhbuild. I'll have a look into it on Saturday night. Any comments are welcome, Noah don't hesitate to email me as much as you like. :)

chpe, do you feel like reviewing the patch?  I've never wrote code for gnome-vfs before, you know ;).

Created attachment 93423 [details] [review]
remembered to use new default methods

Stupid mistake - I had forgotten to use get_default_* in the !HAVE_GNOME case.

(In reply to comment #43)
> chpe, do you feel like reviewing the patch?

If you review my patch in bug 454250 in exchange... :)

---

First, thanks for updating the patch!

+AC_PATH_PROG(GCONFTOOL, gconftool-2, no)
+if test "$GCONFTOOL" != "no"; then
+    AM_GCONF_SOURCE_2
+fi

Remove the |if...|/|fi| around AM_GCONF_SOURCE_2

+ * Copyright (c) 2005 Jason Allen

Don't forget to add yourself, too :)

+static gchar *
+get_default_chapter ()

Use |get_default_chapter (void)|, and same below on all the other occurrences of ().

+static gboolean
+gucharmap_settings_ensure_initialised ()

I'd prefer to have explicit initialisation (and also shutdown before quitting the programme). e.g. gucharmap_settings_init/shutdown, called after gnome_program_init/before unreffing the gnome program at the end of main.
And I'd avoid g_return_if_fail (ensure_initialised()) since either that ensure call never fails (in the HAVE_GNOME case, we always can get the gconfclient instance, so it doesn't fail) or it can fail, but in any case that's a _runtime_ problem, not a programming problem, and thus deserves a regular |if(...) return;| that cannot be compiled out like g_return_if_fail.

+  gchar *chapter;
+
+  if (gucharmap_settings_ensure_initialised())
+    chapter = gconf_client_get_string (settings.gc, GCONF_PREFIX"/chapter", NULL);
+
+  if (chapter) 

This is using |chapter| (potentially) uninitialised. Just declare it as |gchar *chapter = NULL;|.

+  gchar *mode;
+  
+  if (gucharmap_settings_ensure_initialised())
+  	mode = gconf_client_get_string (settings.gc, GCONF_PREFIX"/chapters_mode", NULL);
+
+  if (mode == NULL)

Same.

+  ChaptersMode ret;
[...]
+  if (g_ascii_strncasecmp (mode, "Script", 6) == 0)
+    ret = CHAPTERS_SCRIPT;
+  else if (g_ascii_strncasecmp (mode, "Block", 5) == 0)
+    ret = CHAPTERS_BLOCK;

Same problem.

+gunichar
+gucharmap_settings_get_last_char ()
+{
+  g_return_val_if_fail (gucharmap_settings_ensure_initialised(), 0);
+  return gconf_client_get_int (settings.gc, GCONF_PREFIX"/last_char", NULL);
+}
+
+void
+gucharmap_settings_set_last_char (gunichar wc)
+{
+  g_return_if_fail (gucharmap_settings_ensure_initialised());
+  gconf_client_set_int (settings.gc, GCONF_PREFIX"/last_char", wc, NULL);
+}

gunichar is unsigned.... either we need to cast, or preferably store the setting as string and thus convert the gunichar UTF-32 to/from UTF-8 for gconf.

 static void
+gucharmap_window_size_changed (GtkWidget *widget,
+                               GtkAllocation *allocation)
+{
+  gucharmap_settings_set_window_width (allocation->width);
+  gucharmap_settings_set_window_height (allocation->height);
+}
[...]

+  w = gucharmap_settings_get_window_width ();
+  h = gucharmap_settings_get_window_height ();
+  if (w > 0 && h > 0)
+    gtk_window_resize (GTK_WINDOW (window), w, h);

Here I think we should track the window-state-event too, and only store width/height if the window is neither maximised nor fullscreened. (There's a slight problem with that—see bug 449892—which makes that fail if we're starting up in fullscreen mode already, but that shouldn't prevent the normal case from working. You can just copy the code from http://svn.gnome.org/viewcvs/gnome-games/trunk/libgames-support/games-conf.c?revision=6478&view=markup , see the games_conf_add_window function there.)

+ * gucharmap_chapter_get_string:
+ * @chapters: a #GucharmapChapters
+ *
+ * Returns the name of the currently selected chapter
+ * The result must be freed with g_free()

Returns: a newly allocated string [etc.]

+      do
+        {
+	  gtk_tree_model_get(model, &iter, 0, &str, -1);
+	  match = strcmp (name, str);
+	  g_free(str);
+	  if (0 == match)
+	  {
+	    GtkTreePath *path = gtk_tree_model_get_path (chapters->tree_model, &iter);
+	    gtk_tree_view_set_cursor (GTK_TREE_VIEW (chapters->tree_view), path, NULL, FALSE);
+	    gtk_tree_path_free (path);
+	    return TRUE;
+          }

I think we should scroll to the selected path too. (gtk_tree_view_scroll_to_cell).


+void
+gucharmap_settings_set_last_char (gunichar wc)
[...]
+static gboolean
+gucharmap_active_char_save (gpointer last_char)
+{
+  gucharmap_settings_set_last_char (GPOINTER_TO_INT (last_char));
+  return FALSE;
+}
[...]
+  g_idle_add (gucharmap_active_char_save, GINT_TO_POINTER(wc));

gunichar is unsigned, so you need GPOINTER_TO_UINT and GUINT_TO_POINTER.

Not sure I see the point of doing this on idle instead of directly...

+  /* drawing area may not be exposed yet when restoring last char setting
+   */
+  if (chartable->drawing_area->window == NULL)
+    return;

Maybe use |if (!GTK_WIDGET_REALIZED (chartable))| ? Also this seems like a hack...

Filed Bug 469053 – g_unichar_to/from_string().

Fixed patch is attached.

(In reply to comment #45)
> 
> First, thanks for updating the patch!

My pleasure :)

> 
> +AC_PATH_PROG(GCONFTOOL, gconftool-2, no)
> +if test "$GCONFTOOL" != "no"; then
> +    AM_GCONF_SOURCE_2
> +fi
> 
> Remove the |if...|/|fi| around AM_GCONF_SOURCE_2

Done

> 
> + * Copyright (c) 2005 Jason Allen
> 
> Don't forget to add yourself, too :)

Nah... I haven't really done much work relative to Jason. :)

> 
> +static gchar *
> +get_default_chapter ()
> 
> Use |get_default_chapter (void)|, and same below on all the other occurrences
> of ().

Done

> 
> +static gboolean
> +gucharmap_settings_ensure_initialised ()
> 
> I'd prefer to have explicit initialisation (and also shutdown before quitting
> the programme). e.g. gucharmap_settings_init/shutdown, called after
> gnome_program_init/before unreffing the gnome program at the end of main.

Done

> And I'd avoid g_return_if_fail (ensure_initialised()) since either that ensure
> call never fails (in the HAVE_GNOME case, we always can get the gconfclient
> instance, so it doesn't fail) or it can fail, but in any case that's a
> _runtime_ problem, not a programming problem, and thus deserves a regular
> |if(...) return;| that cannot be compiled out like g_return_if_fail.

Partially done... I left in most of them just because it looks nicer, although I can change them all if necessary.

> 
> +  gchar *chapter;
> +
> +  if (gucharmap_settings_ensure_initialised())
> +    chapter = gconf_client_get_string (settings.gc, GCONF_PREFIX"/chapter",
> NULL);
> +
> +  if (chapter) 
> 
> This is using |chapter| (potentially) uninitialised. Just declare it as |gchar
> *chapter = NULL;|.

Done

> 
> +  gchar *mode;
> +  
> +  if (gucharmap_settings_ensure_initialised())
> +       mode = gconf_client_get_string (settings.gc,
> GCONF_PREFIX"/chapters_mode", NULL);
> +
> +  if (mode == NULL)
> 
> Same.

Done

> 
> +  ChaptersMode ret;
> [...]
> +  if (g_ascii_strncasecmp (mode, "Script", 6) == 0)
> +    ret = CHAPTERS_SCRIPT;
> +  else if (g_ascii_strncasecmp (mode, "Block", 5) == 0)
> +    ret = CHAPTERS_BLOCK;
> 
> Same problem.

Done

> 
> +gunichar
> +gucharmap_settings_get_last_char ()
> +{
> +  g_return_val_if_fail (gucharmap_settings_ensure_initialised(), 0);
> +  return gconf_client_get_int (settings.gc, GCONF_PREFIX"/last_char", NULL);
> +}
> +
> +void
> +gucharmap_settings_set_last_char (gunichar wc)
> +{
> +  g_return_if_fail (gucharmap_settings_ensure_initialised());
> +  gconf_client_set_int (settings.gc, GCONF_PREFIX"/last_char", wc, NULL);
> +}
> 
> gunichar is unsigned.... either we need to cast, or preferably store the
> setting as string and thus convert the gunichar UTF-32 to/from UTF-8 for gconf.

Done using code from bug 469053 :)

> 
>  static void
> +gucharmap_window_size_changed (GtkWidget *widget,
> +                               GtkAllocation *allocation)
> +{
> +  gucharmap_settings_set_window_width (allocation->width);
> +  gucharmap_settings_set_window_height (allocation->height);
> +}
> [...]
> 
> +  w = gucharmap_settings_get_window_width ();
> +  h = gucharmap_settings_get_window_height ();
> +  if (w > 0 && h > 0)
> +    gtk_window_resize (GTK_WINDOW (window), w, h);
> 
> Here I think we should track the window-state-event too, and only store
> width/height if the window is neither maximised nor fullscreened. (There's a
> slight problem with that—see bug 449892—which makes that fail if we're
> starting up in fullscreen mode already, but that shouldn't prevent the normal
> case from working. You can just copy the code from
> http://svn.gnome.org/viewcvs/gnome-games/trunk/libgames-support/games-conf.c?revision=6478&view=markup
> , see the games_conf_add_window function there.)

Done. A new key is added for maximized, but not fullscreened (I'm not sure whether that's necessary/desirable - anyway it's easy to add). Thanks for the code reference. :)

> 
> + * gucharmap_chapter_get_string:
> + * @chapters: a #GucharmapChapters
> + *
> + * Returns the name of the currently selected chapter
> + * The result must be freed with g_free()
> 
> Returns: a newly allocated string [etc.]

Done

> 
> +      do
> +        {
> +         gtk_tree_model_get(model, &iter, 0, &str, -1);
> +         match = strcmp (name, str);
> +         g_free(str);
> +         if (0 == match)
> +         {
> +           GtkTreePath *path = gtk_tree_model_get_path (chapters->tree_model,
> &iter);
> +           gtk_tree_view_set_cursor (GTK_TREE_VIEW (chapters->tree_view),
> path, NULL, FALSE);
> +           gtk_tree_path_free (path);
> +           return TRUE;
> +          }
> 
> I think we should scroll to the selected path too.
> (gtk_tree_view_scroll_to_cell).

Done

> 
> 
> +void
> +gucharmap_settings_set_last_char (gunichar wc)
> [...]
> +static gboolean
> +gucharmap_active_char_save (gpointer last_char)
> +{
> +  gucharmap_settings_set_last_char (GPOINTER_TO_INT (last_char));
> +  return FALSE;
> +}
> [...]
> +  g_idle_add (gucharmap_active_char_save, GINT_TO_POINTER(wc));
> 
> gunichar is unsigned, so you need GPOINTER_TO_UINT and GUINT_TO_POINTER.

Done

> 
> Not sure I see the point of doing this on idle instead of directly...

Possibly due to performance (e.g. if you are quickly moving across the table)...? I'm not sure, Jason wrote that.

> 
> +  /* drawing area may not be exposed yet when restoring last char setting
> +   */
> +  if (chartable->drawing_area->window == NULL)
> +    return;
> 
> Maybe use |if (!GTK_WIDGET_REALIZED (chartable))| ? Also this seems like a
> hack...
> 

Done. Again, I'm not sure because Jason wrote it.

Created attachment 94097 [details] [review]
updated patch

(In reply to comment #46)
> Filed Bug 469053 – g_unichar_to/from_string().

I was thinking more like using g_unichar_to_utf8 / g_utf8_to_ucs4, but your idea is good too :)

---

Just a few comments on the updated patch:

+  gucharmap_settings_initialize ();
   program = gnome_program_init ("gucharmap", VERSION, LIBGNOMEUI_MODULE,

I think you need to move that after gnome_program_init, since that initialised gconf and you use gconf.

Hmm... do we need to prepare for settings init failure? I think we should do either

if (!gnome_settings_initialize ())
  exit (EXIT_FAILURE);

or, if we want to allow settings failure (might be nice in the !HAVE_GNOME case once we implement that), we should change the g_return_if_fail (gucharmap_settings_initialized ()) checks to regular if (!...) return <DEFAULT>;.

(In reply to comment #49)
> (In reply to comment #46)
> > Filed Bug 469053 – g_unichar_to/from_string().
> 
> I was thinking more like using g_unichar_to_utf8 / g_utf8_to_ucs4, but your
> idea is good too :)
> 
> ---
> 
> Just a few comments on the updated patch:
> 
> +  gucharmap_settings_initialize ();
>    program = gnome_program_init ("gucharmap", VERSION, LIBGNOMEUI_MODULE,
> 
> I think you need to move that after gnome_program_init, since that initialised
> gconf and you use gconf.

Hmm.. before and after both work for me, but I'll trust you that after is better. :) I had put it before in order to fix a seg fault, but I think that was actually caused by a silly error elsewhere. It can be safely put outside of |#if HAVE_GNOME| (to around line 80), since it does nothing in the other case.

> 
> Hmm... do we need to prepare for settings init failure? I think we should do
> either
> 
> if (!gnome_settings_initialize ())
>   exit (EXIT_FAILURE);
> 
> or, if we want to allow settings failure (might be nice in the !HAVE_GNOME case
> once we implement that), we should change the g_return_if_fail
> (gucharmap_settings_initialized ()) checks to regular if (!...) return
> <DEFAULT>;.
> 

Good point. Well, the former has the advantage of being fail-fast, but exiting the entire program is a bit drastic. So I'd say that the latter is the better option, though I'm not sure whether some kind of error logging would be desirable.

On a related note, should there be an initialisation |static GucharmapSettings settings = { NULL };| or is it fine to leave the zero-initialisation implicit?

(In reply to comment #50)
> On a related note, should there be an initialisation |static GucharmapSettings
> settings = { NULL };| or is it fine to leave the zero-initialisation implicit?

I think it's ok to just leave it uninitialised.

The patch attached to bug 469131 removes --disable-gnome option altogether. Would this not be better called --enable-gconf?

Yep, make it autodetect whether gconf is available, but also have --enable-gconf and --disable-gconf.

Created attachment 94742 [details] [review]
more updated patch

Fixed issues from comment #49 and comment #53. :)

Created attachment 94916 [details] [review]
Fixed a problem with previous patch

Fixed a silly mistake from last patch. Here is the diff between the two patches:

=== modified file 'configure.ac'
--- configure.ac        2007-09-01 03:44:21 +0000
+++ configure.ac        2007-09-04 11:15:24 +0000
@@ -107,7 +107,7 @@
 then
     GCONFPKGS="gconf-2.0"
 fi
-AC_SUBST(GNOMEPKGS)
+AC_SUBST(GCONFPKGS)
 
 # checks for funcs 
 AC_CHECK_FUNCS([bind_textdomain_codeset])

Christian, is the patch ready to go on in your opinion? :P

I think it's ready, except for the missing implementation of gucharmap_settings_set_last_char : I'd not depend on an API addition (bug 349659) to glib, but just use this:

char utf8[7];
g_unichar_to_utf8(wc, utf8);
gconf_client_set_string(...)

and adapt gucharmap_settings_set_last_char accordingly. Shall I make that change, and commit?

Either UTF-8, or what I prefer, printf ("U+%04X").  Parse it using scanf and same format.

Actually the latest patch already includes that change, I had looked in the #ifndef GNOME section and just seen the dummy impl :)

Branched, and committed to trunk:

        * configure.ac: Bump version after branching.

        * Makefile.am:
        * gucharmap.pc.in:
        * gucharmap/Makefile.am:
        * gucharmap/gucharmap-block-chapters.c: (selection_changed),
        (gucharmap_block_chapters_settings_init),
        (gucharmap_block_chapters_init):
        * gucharmap/gucharmap-chapters.c: (gucharmap_chapter_get_string),
        (gucharmap_chapter_set_string):
        * gucharmap/gucharmap-chapters.h:
        * gucharmap/gucharmap-charmap.c: (gucharmap_active_char_save),
        (active_char_set), (gucharmap_charmap_new):
        * gucharmap/gucharmap-script-chapters.c: (selection_changed),
        (gucharmap_script_chapters_settings_init),
        (gucharmap_script_chapters_init):
        * gucharmap/gucharmap-table.c: (draw_chartable_from_scratch):
        * gucharmap/gucharmap-window.c: (snap_cols_pow2), (make_menu),
        (fontsel_changed), (gucharmap_window_state_changed),
        (gucharmap_window_size_changed), (gucharmap_window_init):
        * gucharmap/main.c: (main): Remember the settings. Bug #140414,
        patch by Jason Allen and Jared Moore.

I #if 0'd one part of the patch, the gucharmap_[sg]et_chapter{,s_mode} functions. The set_chapter function is setting the pref as a localised string, but IMO it should use a locale-neutral value. I'm going to file a follow-up bug for that though; this one is long enough :)