[PATCH] Change the look of dialogs created with `x-popup-dialog'

[PATCH] Change the look of dialogs created with `x-popup-dialog'

[Andrey Smirnov]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

Hello everybody,

I often close emacs with mouse by pressing 'close' buttons of its
window and therefore I often encounter questions presented in form of
dialogs produced by `x-popup-dialog' function. I know it is the matter
of one's taste, but I think that appearance of aforementioned dialogs
could be improved by making them look more similar to those created by
other Gtk and GNOME applications(I used GEdit as a template). The
patch in attachment is my attempt to implements such changes. To
summarize it does the following:

  - Adds, depending on the dialog type, appropriate icon to the dialog,
    one of GTK_STOCK_DIALOG_INFO or GTK_STOCK_DIALOG_QUESTION.
  - Removes resize button from the resulting dialog window.
  - For dialogs with more than two buttons, radio-buttons instead of
    regular buttons are created.
  - Makes the question to be rendered in bold font.

The difference between the results of `x-popup-dialog' can be seen on
the image in the attachment.

I guess, there are probably many improvements that can be added both
to the dialog's appearance and to other code of the patch, so at this
point I would like you to ask to comment on the patch.

Andrey Smirnov

[-- Attachment #2: 0001-Change-the-look-of-dialogs-created-with-x-popup-dial.patch --]
[-- Type: text/x-diff, Size: 13027 bytes --]

From 53a4634fc4ce2e72278c8d8df65148f89bd4cf0a Mon Sep 17 00:00:00 2001
From: Andrey Smirnov <andrew.smirnov@gmail.com>
Date: Sat, 10 Dec 2011 18:13:42 -0800
Subject: [PATCH 1/2] Change the look of dialogs created with
 `x-popup-dialog'(Gtk)

New code creates dialogs that looks(hopefully) more similar to dialogs
created by other Gtk applications such as GEdit. Created dialog no
longer has more than two buttons and multiple choices situation is
handled with radio-buttons.
---
 src/gtkutil.c |  255 +++++++++++++++++++++++++++++++++++++++++---------------
 src/xmenu.c   |    2 -
 2 files changed, 186 insertions(+), 71 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index bc71685..03228a8 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -1398,36 +1398,38 @@ xg_set_frame_icon (FRAME_PTR f, Pixmap icon_pixmap, Pixmap icon_mask)
 /* Return the dialog title to use for a dialog of type KEY.
    This is the encoding used by lwlib.  We use the same for GTK.  */
 
-static const char *
-get_dialog_title (char key)
+static GtkWidget *
+get_dialog_icon (char key)
 {
-  const char *title = "";
+  GtkWidget *image = NULL;
 
   switch (key) {
   case 'E': case 'e':
-    title = "Error";
+    image = gtk_image_new_from_stock (GTK_STOCK_DIALOG_ERROR,
+                                      GTK_ICON_SIZE_DIALOG);
     break;
 
   case 'I': case 'i':
-    title = "Information";
+    image = gtk_image_new_from_stock (GTK_STOCK_DIALOG_INFO,
+                                      GTK_ICON_SIZE_DIALOG);
     break;
 
   case 'L': case 'l':
-    title = "Prompt";
     break;
 
   case 'P': case 'p':
-    title = "Prompt";
     break;
 
   case 'Q': case 'q':
-    title = "Question";
+    image = gtk_image_new_from_stock (GTK_STOCK_DIALOG_QUESTION,
+                                      GTK_ICON_SIZE_DIALOG);
     break;
   }
 
-  return title;
+  return image;
 }
 
+
 /* Callback for dialogs that get WM_DELETE_WINDOW.  We pop down
    the dialog, but return TRUE so the event does not propagate further
    in GTK.  This prevents GTK from destroying the dialog widget automatically
@@ -1458,99 +1460,214 @@ create_dialog (widget_value *wv,
                GCallback select_cb,
                GCallback deactivate_cb)
 {
-  const char *title = get_dialog_title (wv->name[0]);
   int total_buttons = wv->name[1] - '0';
-  int right_buttons = wv->name[4] - '0';
-  int left_buttons;
-  int button_nr = 0;
-  int button_spacing = 10;
+
+  /*
+     Depending on the `total_buttons' value the dialog created can be
+    of two types:
+
+    1. when total_buttons > 2, then resulting dialog has following
+    structure:
+
+    +------- content_area -------------------------+
+    | +-------- hbox ----------------------------+ |
+    | | +-------+ +-- vbox --------------------+ | |
+    | | |       | | <message>                  | | |
+    | | | image | | +------ choices_box -----+ | | |
+    | | |       | | |  o first item          | | | |
+    | | |       | | |  o second item         | | | |
+    | | |       | | |  o third item          | | | |
+    | | |       | | |  ...                   | | | |
+    | | |       | | +------------------------+ | | |
+    | | |       | | +------ bbox ------------+ | | |
+    | | |       | | |     +----+  +--------+ | | | |
+    | | |       | | |     | OK |  | Cancel | | | | |
+    | | |       | | |     +----+  +--------+ | | | |
+    | | |       | | +------------------------+ | | |
+    | | +-------+ +----------------------------+ | |
+    | +------------------------------------------+ |
+    +----------------------------------------------+
+
+    2. when total_buttons <= 2, its structure is this:
+
+    +------------- content_area ------------------------------+
+    |  +--------- hbox -------------------------------------+ |
+    |  | +---------+ +------ vbox ------------------------+ | |
+    |  | |         | | <message>                          | | |
+    |  | | image   | | +----------- choices_box --------+ | | |
+    |  | |         | | | +------------+ +-------------+ | | | |
+    |  | |         | | | | first item | | second item | | | | |
+    |  | |         | | | +------------+ +-------------+ | | | |
+    |  | |         | | +--------------------------------+ | | |
+    |  | +---------+ +------------------------------------+ | |
+    |  +----------------------------------------------------+ |
+    +---------------------------------------------------------+
+
+   */
+
   GtkWidget *wdialog = gtk_dialog_new ();
-  GtkDialog *wd = GTK_DIALOG (wdialog);
-  GtkBox *cur_box = GTK_BOX (gtk_dialog_get_action_area (wd));
+  GtkWidget *content_area = gtk_dialog_get_action_area (GTK_DIALOG (wdialog));
   widget_value *item;
-  GtkWidget *whbox_down;
 
-  /* If the number of buttons is greater than 4, make two rows of buttons
-     instead.  This looks better.  */
-  int make_two_rows = total_buttons > 4;
+  GtkWidget *hbox, *vbox, *choices_box = NULL, *w = NULL;
+  gboolean first_item_p = TRUE;
 
-  if (right_buttons == 0) right_buttons = total_buttons/2;
-  left_buttons = total_buttons - right_buttons;
+  g_object_set (wdialog,
+                "destroy-with-parent" , TRUE,
+                "modal"               , TRUE,
+                "title"               , "",
+                "name"                , "emacs-dialog",
+                "resizable"           , FALSE,
+                "skip-taskbar-hint"   , TRUE,
+                NULL);
 
-  gtk_window_set_title (GTK_WINDOW (wdialog), title);
-  gtk_widget_set_name (wdialog, "emacs-dialog");
+  hbox = gtk_hbox_new (FALSE, 12);
+  gtk_container_set_border_width (GTK_CONTAINER (hbox), 5);
 
+  vbox = gtk_vbox_new (FALSE, 12);
 
-  if (make_two_rows)
-    {
-      GtkWidget *wvbox = gtk_vbox_new (TRUE, button_spacing);
-      GtkWidget *whbox_up = gtk_hbox_new (FALSE, 0);
-      whbox_down = gtk_hbox_new (FALSE, 0);
+  {
+    GtkWidget *image = get_dialog_icon (wv->name[0]);
+    gtk_misc_set_alignment (GTK_MISC (image), 0.5, 0.0);
 
-      gtk_box_pack_start (cur_box, wvbox, FALSE, FALSE, 0);
-      gtk_box_pack_start (GTK_BOX (wvbox), whbox_up, FALSE, FALSE, 0);
-      gtk_box_pack_start (GTK_BOX (wvbox), whbox_down, FALSE, FALSE, 0);
+    gtk_box_pack_start (GTK_BOX (hbox), image, FALSE, FALSE, 0);
+  }
 
-      cur_box = GTK_BOX (whbox_up);
-    }
+  gtk_box_pack_start (GTK_BOX (hbox), vbox, FALSE, FALSE, 0);
+  gtk_container_add (GTK_CONTAINER (content_area), hbox);
 
-  g_signal_connect (G_OBJECT (wdialog), "delete-event",
-                    G_CALLBACK (dialog_delete_callback), 0);
-
-  if (deactivate_cb)
-    {
-      g_signal_connect (G_OBJECT (wdialog), "close", deactivate_cb, 0);
-      g_signal_connect (G_OBJECT (wdialog), "response", deactivate_cb, 0);
-    }
 
   for (item = wv->contents; item; item = item->next)
     {
       char *utf8_label = get_utf8_string (item->value);
-      GtkWidget *w;
-      GtkRequisition req;
 
+      /* Question */
       if (item->name && strcmp (item->name, "message") == 0)
         {
-          GtkBox *wvbox = GTK_BOX (gtk_dialog_get_content_area (wd));
-          /* This is the text part of the dialog.  */
-          w = gtk_label_new (utf8_label);
-          gtk_box_pack_start (wvbox, gtk_label_new (""), FALSE, FALSE, 0);
-          gtk_box_pack_start (wvbox, w, TRUE, TRUE, 0);
-          gtk_misc_set_alignment (GTK_MISC (w), 0.1, 0.5);
+          GtkRequisition req;
+
+          w = gtk_label_new (NULL);
+          g_object_set (w,
+                        "wrap"       , TRUE,
+                        "use-markup" , TRUE,
+                        "selectable" , TRUE,
+                        "xalign"     , 0.0,
+                        "yalign"     , 0.5,
+                        NULL);
+          {
+            gchar *message = g_strconcat
+              ("<span weight=\"bold\" size=\"larger\">",
+               utf8_label, "</span>", NULL);
+
+            gtk_label_set_markup (GTK_LABEL (w), message);
+            g_free (message);
+          }
+
+          gtk_box_pack_start (GTK_BOX (vbox), w, TRUE, TRUE, 0);
 
           /* Try to make dialog look better.  Must realize first so
              the widget can calculate the size it needs.  */
           gtk_widget_realize (w);
           gtk_widget_get_preferred_size (w, NULL, &req);
-          gtk_box_set_spacing (wvbox, req.height);
-	  if (item->value && strlen (item->value) > 0)
-            button_spacing = 2*req.width/strlen (item->value);
+          gtk_box_set_spacing (GTK_BOX (vbox), req.height);
+
+          w = NULL;
         }
+      /* Choices (radiobuttons) */
+      else if (total_buttons > 2)
+        {
+          if (!choices_box)
+            choices_box = gtk_vbox_new (TRUE, 5);
+
+          w = gtk_radio_button_new_with_label_from_widget (GTK_RADIO_BUTTON (w),
+                                               utf8_label);
+         if (!item->enabled)
+            gtk_widget_set_sensitive (w, FALSE);
+
+         if (select_cb)
+           {
+             g_signal_connect (G_OBJECT (w), "clicked",
+                               G_CALLBACK (select_cb), item->call_data);
+
+             /*
+                Since the first item of radiobutton group is selected
+                by default we have to call select_cb to set
+                `menu_item_selection' to appropriate value.
+             */
+             if (first_item_p)
+               {
+                 first_item_p = FALSE;
+                 ((void (*) (GtkWidget *, gpointer)) select_cb)
+                   (w, item->call_data);
+               }
+           }
+
+         gtk_box_pack_start (GTK_BOX (choices_box), w, TRUE, TRUE, 0);
+        }
+      /* Choices (buttons) */
       else
         {
-          /* This is one button to add to the dialog.  */
+          if (!choices_box)
+            {
+              choices_box = gtk_hbutton_box_new ();
+              gtk_button_box_set_layout (GTK_BUTTON_BOX (choices_box),
+                                         GTK_BUTTONBOX_END);
+              gtk_box_set_spacing (GTK_BOX (choices_box), 10);
+            }
+
           w = gtk_button_new_with_label (utf8_label);
-          if (! item->enabled)
-            gtk_widget_set_sensitive (w, FALSE);
+
           if (select_cb)
             g_signal_connect (G_OBJECT (w), "clicked",
                               select_cb, item->call_data);
+          if (deactivate_cb)
+            g_signal_connect (G_OBJECT (w), "clicked", deactivate_cb, NULL);
 
-          gtk_box_pack_start (cur_box, w, TRUE, TRUE, button_spacing);
-          if (++button_nr == left_buttons)
-            {
-              if (make_two_rows)
-                cur_box = GTK_BOX (whbox_down);
-              else
-                gtk_box_pack_start (cur_box,
-                                    gtk_label_new (""),
-                                    TRUE, TRUE,
-                                    button_spacing);
-            }
+
+          gtk_container_add (GTK_CONTAINER (choices_box), w);
         }
+      if (utf8_label)
+        g_free (utf8_label);
+    }
+
+  if (total_buttons > 2)
+    {
+      GtkWidget *bbox = gtk_hbutton_box_new ();
+      GtkWidget *ok = gtk_button_new_from_stock (GTK_STOCK_OK);
+      GtkWidget *cancel = gtk_button_new_from_stock (GTK_STOCK_CANCEL);
+      GtkWidget *frame  = gtk_frame_new ("Possible actions/answers");
+
+      gtk_container_add (GTK_CONTAINER (bbox), ok);
+      gtk_container_add (GTK_CONTAINER (bbox), cancel);
+
+      gtk_button_box_set_layout (GTK_BUTTON_BOX (bbox), GTK_BUTTONBOX_END);
+      gtk_box_set_spacing (GTK_BOX (bbox), 10);
+
+
+      gtk_container_add (GTK_CONTAINER (frame), choices_box);
+      gtk_box_pack_start (GTK_BOX (vbox), frame, TRUE, TRUE, 0);
+
+      gtk_box_pack_start (GTK_BOX (vbox), bbox, TRUE, TRUE, 0);
 
-     if (utf8_label)
-       g_free (utf8_label);
+      if (deactivate_cb)
+        {
+          g_signal_connect (G_OBJECT (ok), "clicked", deactivate_cb, 0);
+          g_signal_connect (G_OBJECT (cancel), "clicked", select_cb, 0);
+          g_signal_connect (G_OBJECT (cancel), "clicked", deactivate_cb, 0);
+        }
+    }
+  else
+    {
+      gtk_box_pack_start (GTK_BOX (vbox), choices_box, TRUE, TRUE, 0);
+    }
+
+  g_signal_connect (G_OBJECT (wdialog), "delete-event",
+                    G_CALLBACK (dialog_delete_callback), 0);
+
+  if (deactivate_cb)
+    {
+      g_signal_connect (G_OBJECT (wdialog), "close", deactivate_cb, 0);
+      g_signal_connect (G_OBJECT (wdialog), "response", deactivate_cb, 0);
     }
 
   return wdialog;
diff --git a/src/xmenu.c b/src/xmenu.c
index 4b7bbfd..547b538 100644
--- a/src/xmenu.c
+++ b/src/xmenu.c
@@ -1904,8 +1904,6 @@ dialog_selection_callback (GtkWidget *widget, gpointer client_data)
      as long as pointers have enough bits to hold small integers.  */
   if ((intptr_t) client_data != -1)
     menu_item_selection = (Lisp_Object *) client_data;
-
-  popup_activated_flag = 0;
 }
 
 /* Pop up the dialog for frame F defined by FIRST_WV and loop until the
-- 
1.7.5.4


[-- Attachment #3: before-after.png --]
[-- Type: image/png, Size: 86879 bytes --]

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Lennart Borgman]

On Sun, Dec 11, 2011 at 16:01, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Hello everybody,
>
> I often close emacs with mouse by pressing 'close' buttons of its
> window and therefore I often encounter questions presented in form of
> dialogs produced by `x-popup-dialog' function. I know it is the matter
> of one's taste, but I think that appearance of aforementioned dialogs
> could be improved by making them look more similar to those created by
> other Gtk and GNOME applications(I used GEdit as a template). The
> patch in attachment is my attempt to implements such changes. To
> summarize it does the following:

I have previously sent similar patches for w32.

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Stefan Monnier]

> I have previously sent similar patches for w32.

Doesn't ring a bell, but my memory is overloaded by ever-growing list of
"bugs and patches I should look into".


        Stefan

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Andrey Smirnov]

On Mon, Dec 12, 2011 at 9:49 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> I have previously sent similar patches for w32.
>
> Doesn't ring a bell, but my memory is overloaded by ever-growing list of
> "bugs and patches I should look into".

I tried to search for it but didn't find anything. Lennart, could you
please give a link to your patches? I think it would be nice to have
some semblance of consistency in the look of the dialogs.

Andrey Smirnov

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Lennart Borgman]

On Tue, Dec 13, 2011 at 03:47, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Mon, Dec 12, 2011 at 9:49 AM, Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
>>> I have previously sent similar patches for w32.
>>
>> Doesn't ring a bell, but my memory is overloaded by ever-growing list of
>> "bugs and patches I should look into".
>
> I tried to search for it but didn't find anything. Lennart, could you
> please give a link to your patches? I think it would be nice to have
> some semblance of consistency in the look of the dialogs.

Hm, it was long ago. I will try to see if I can find them. I might
have lost them on the way and maybe I just told about them here.

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Stefan Monnier]

> I often close Emacs with mouse by pressing 'close' buttons of its
> window and therefore I often encounter questions presented in form of
> dialogs produced by `x-popup-dialog' function. I know it is the matter
> of one's taste, but I think that appearance of aforementioned dialogs
> could be improved by making them look more similar to those created by
> other Gtk and GNOME applications(I used GEdit as a template). The

I don't have time to look at the code now, and am not very familiar with
this part of the code anyway, but at least the intention and the sample
images sound good.

Of course, since we're in feature freeze we can't install it before the
trunk re-opens for development (hopefully "early" 2012), so there's time
to fine-tune it and get the paperwork ready,


        Stefan

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Jan Djärv]

Hello.

The change looks good in principle, the current dialog when closing Emacs with unsaved changes contains way too many buttons.  How does that look on your new implementation?  Should the radiobuttons be grouped into two columns when the exceed some number, say 4 or 6 for example?

The code has issues however:

You have removed the title, that is no good.  Several window managers show the title in icon bars and other places, so keep the title even if you add an icon.

The use of default bold indicates screaming to me.  It should not be default.  A configure option is OK.
Ditto font size larger.  Not sure if this is a good default.

The use of markup opens up the possibility of having markup in the text for the dialog.  Also, some characters will not display right, as they will be interpretered as masrkup.  You need to escape the text with g_markup_escape in glib.

Radio buttons should not have any callbacks.  When OK is pressed you should check what radiobutton is selected and then call the callback.  Just selecting a radio button should not execute any code if there are OK and Cancel buttons present.  Also the select callback pops down the dialog.  This is not something a radiobutton should do.


+          g_signal_connect (G_OBJECT (ok), "clicked", deactivate_cb, 0);
+          g_signal_connect (G_OBJECT (cancel), "clicked", select_cb, 0);
+          g_signal_connect (G_OBJECT (cancel), "clicked", deactivate_cb, 0);

Why select_cb on the cancel button?  It is not right.  You should make a new function that ok is bound to that extracts the selected radiobutton and calls select_cb.


-  popup_activated_flag = 0;

Do not remove this line.  It is needed.  Why are you removing it?

	Jan D.

11 dec 2011 kl. 16:01 skrev Andrey Smirnov:

> Hello everybody,
> 
> I often close emacs with mouse by pressing 'close' buttons of its
> window and therefore I often encounter questions presented in form of
> dialogs produced by `x-popup-dialog' function. I know it is the matter
> of one's taste, but I think that appearance of aforementioned dialogs
> could be improved by making them look more similar to those created by
> other Gtk and GNOME applications(I used GEdit as a template). The
> patch in attachment is my attempt to implements such changes. To
> summarize it does the following:
> 
>  - Adds, depending on the dialog type, appropriate icon to the dialog,
>    one of GTK_STOCK_DIALOG_INFO or GTK_STOCK_DIALOG_QUESTION.
>  - Removes resize button from the resulting dialog window.
>  - For dialogs with more than two buttons, radio-buttons instead of
>    regular buttons are created.
>  - Makes the question to be rendered in bold font.
> 
> The difference between the results of `x-popup-dialog' can be seen on
> the image in the attachment.
> 
> I guess, there are probably many improvements that can be added both
> to the dialog's appearance and to other code of the patch, so at this
> point I would like you to ask to comment on the patch.
> 
> Andrey Smirnov
> <0001-Change-the-look-of-dialogs-created-with-x-popup-dial.patch><before-after.png>

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Andrey Smirnov]

On Mon, Dec 12, 2011 at 11:07 PM, Jan Djärv <jan.h.d@swipnet.se> wrote:
> Hello.
>
> The change looks good in principle, the current dialog when closing
> Emacs with unsaved changes contains way too many buttons.  How does
> that look on your new implementation?

I, attached before/after screenshot to my initial e-mail, did you see
it? Or do you want to see the results of different code sample?

> Should the radiobuttons be grouped into two columns when the exceed
> some number, say 4 or 6 for example?

I don't know, since the upper limit on the amount of buttons is 9, and
9 radio-buttons in the same row looks fine to my taste, in my opinion
they shouldn't be, but then again that's the matter of personal taste.

>
> The code has issues however:
>
> You have removed the title, that is no good.  Several window
> managers show the title in icon bars and other places, so keep the
> title even if you add an icon.

Well, the title is pretty much useless, since it doesn't provide any
information, and I did set the "skip-taskbar-hint" to TRUE, so it
shouldn't appear in the task bar. What icon bars are we talking about?
I used GEdit as some sort of reference implementation of how dialogs of
GNOME editors should look like, and it's dialogs don't have the title.
I don't mind returning it back, it would be  a trivial change after
all, it's just I'm not sure that I see the point.

>
> The use of default bold indicates screaming to me.

I thought that function was reserved for all caps :)

> It should not be default. A configure option is OK. Ditto font
> size larger. Not sure if this is a good default.

Well, again I tried to emulate GEdit, whose dialogs I find to be
easier on the eyes. And don't you think it would be too minor detail
to be worth a dedicated configure option. Shouldn't we just some sort
of a consensus and just use that settings?

>
> The use of markup opens up the possibility of having markup in the
> text for the dialog. Also, some characters will not display right,
> as they will be interpretered as masrkup. You need to escape the
> text with g_markup_escape in glib.

Didn't think of that one. Thanks, will fix that.

>
> Radio buttons should not have any callbacks. When OK is pressed you
> should check what radiobutton is selected and then call the
> callback. Just selecting a radio button should not execute any code
> if there are OK and Cancel buttons present. Also the select
> callback pops down the dialog. This is not something a radiobutton
> should do.

Well, I guess there is more than one way to skin a cat, I'll rewrite
that portion of the code.

>
>
> +          g_signal_connect (G_OBJECT (ok), "clicked", deactivate_cb, 0);
> +          g_signal_connect (G_OBJECT (cancel), "clicked", select_cb, 0);
> +          g_signal_connect (G_OBJECT (cancel), "clicked", deactivate_cb, 0);
>
> Why select_cb on the cancel button?

Because I need to set `menu_item_selection' to 0 otherwise if user
fiddles with radiobuttons and then presses cancel button for the code
in xmenu.c it would be indistinguishable from OK button being pressed.

>
> -  popup_activated_flag = 0;
>
> Do not remove this line. It is needed. Why are you removing it?

It is removed because otherwise one wouldn't be able to change
selected radio button more than once, since the first call to
`select_cb' would close the whole dialog.

At least for Gtk this code is redundant anyway, since the same
functionality could, and IMHO should, be achieved by chaining
`select_cb' and `deactivate_cb' for particular signal. Not sure if all
the other toolkits allow you to do such a thing.

Andrey Smirnov

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Jan Djärv]

[-- Attachment #1: Type: text/plain, Size: 3297 bytes --]

Hello.

14 dec 2011 kl. 06:29 skrev Andrey Smirnov:

> On Mon, Dec 12, 2011 at 11:07 PM, Jan Djärv <jan.h.d@swipnet.se> wrote:
>> Hello.
>> 
>> The change looks good in principle, the current dialog when closing
>> Emacs with unsaved changes contains way too many buttons.  How does
>> that look on your new implementation?
> 
> I, attached before/after screenshot to my initial e-mail, did you see
> it? Or do you want to see the results of different code sample?

I checked, 6 looks quite Ok.

>> 
>> The code has issues however:
>> 
>> You have removed the title, that is no good.  Several window
>> managers show the title in icon bars and other places, so keep the
>> title even if you add an icon.
> 
> Well, the title is pretty much useless, since it doesn't provide any
> information, and I did set the "skip-taskbar-hint" to TRUE, so it
> shouldn't appear in the task bar. What icon bars are we talking about?
> I used GEdit as some sort of reference implementation of how dialogs of
> GNOME editors should look like, and it's dialogs don't have the title.
> I don't mind returning it back, it would be  a trivial change after
> all, it's just I'm not sure that I see the point.

If you Alt-tab in icewm for example, it shows the titles, see attached picture.


> 
>> 
>> The use of default bold indicates screaming to me.
> 
> I thought that function was reserved for all caps :)
> 

A lot of shouting going on...

>> It should not be default. A configure option is OK. Ditto font
>> size larger. Not sure if this is a good default.
> 
> Well, again I tried to emulate GEdit, whose dialogs I find to be
> easier on the eyes. And don't you think it would be too minor detail
> to be worth a dedicated configure option. Shouldn't we just some sort
> of a consensus and just use that settings?

Perhaps, but then we maybe should change all ports?

> 
> 
>> 
>> Radio buttons should not have any callbacks. When OK is pressed you
>> should check what radiobutton is selected and then call the
>> callback. Just selecting a radio button should not execute any code
>> if there are OK and Cancel buttons present. Also the select
>> callback pops down the dialog. This is not something a radiobutton
>> should do.
> 
> Well, I guess there is more than one way to skin a cat, I'll rewrite
> that portion of the code.
> 
>> 
>> 
>> +          g_signal_connect (G_OBJECT (ok), "clicked", deactivate_cb, 0);
>> +          g_signal_connect (G_OBJECT (cancel), "clicked", select_cb, 0);
>> +          g_signal_connect (G_OBJECT (cancel), "clicked", deactivate_cb, 0);
>> 
>> Why select_cb on the cancel button?
> 
> Because I need to set `menu_item_selection' to 0 otherwise if user
> fiddles with radiobuttons and then presses cancel button for the code
> in xmenu.c it would be indistinguishable from OK button being pressed.
> 
>> 
>> -  popup_activated_flag = 0;
>> 
>> Do not remove this line. It is needed. Why are you removing it?
> 
> It is removed because otherwise one wouldn't be able to change
> selected radio button more than once, since the first call to
> `select_cb' would close the whole dialog.

These two issues will go away if you defer calling callbacks until OK is pressed.

	Jan D.



[-- Attachment #2.1: Type: text/html, Size: 5465 bytes --]

[-- Attachment #2.2: dialog.png --]
[-- Type: image/png, Size: 66903 bytes --]

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Andrey Smirnov]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Wed, Dec 14, 2011 at 12:41 PM, Jan Djärv <jan.h.d@swipnet.se> wrote:
> If you Alt-tab in icewm for example, it shows the titles, see
> attached picture.

Now, I see your point. On the other hand in Unity window title doesn't
play that important role in window-switching. Can we compromise on
exposing run-time variable something along the lines of
`gtk-popup-dialogs-show-titles' and then the appearance could be
controlled from user's .emacs?

>
> The use of default bold indicates screaming to me.
>
>
> I thought that function was reserved for all caps :)
>
>
> A lot of shouting going on...

All right, in for a penny in for a pound, how about
`gtk-popup-dialogs-message-format' to control that aspect?

>
> Perhaps, but then we maybe should change all ports?

Do you mean change how dialogs look in all toolkits?
Yes, I think it would be even a better idea, I'll look into it.
Hopefully Lennart would be able to find his patches for w32, and then
they could be used.

> These two issues will go away if you defer calling callbacks until
> OK is pressed.

It still won't change the fact that original
`dialog_selection_callback' duplicates the functionality of
`popup_deactivate_callback'.


I attached reworked version of the patch, it still lacks comments with
description for new functions, but I'll add it as soon as the code
stabilizes.

Andrey Smirnov

[-- Attachment #2: 0001-Change-the-look-of-dialogs-created-with-x-popup-dial.patch --]
[-- Type: text/x-diff, Size: 16346 bytes --]

From 99ccca62b6a2402cba69114b9b86e534511222fd Mon Sep 17 00:00:00 2001
From: Andrey Smirnov <andrew.smirnov@gmail.com>
Date: Sat, 10 Dec 2011 18:13:42 -0800
Subject: [PATCH 1/2] Change the look of dialogs created with
 `x-popup-dialog'(Gtk)

New code creates dialogs that looks(hopefully) more similar to dialogs
created by other Gtk applications such as GEdit. Created dialog no
longer has more than two buttons and multiple choices situation is
handled with radio-buttons.
---
 src/gtkutil.c |  368 +++++++++++++++++++++++++++++++++++++++++++--------------
 src/gtkutil.h |    2 +
 src/xmenu.c   |    2 -
 3 files changed, 279 insertions(+), 93 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index bc71685..7114e4f 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -1398,36 +1398,38 @@ xg_set_frame_icon (FRAME_PTR f, Pixmap icon_pixmap, Pixmap icon_mask)
 /* Return the dialog title to use for a dialog of type KEY.
    This is the encoding used by lwlib.  We use the same for GTK.  */
 
-static const char *
-get_dialog_title (char key)
+static GtkWidget *
+get_dialog_icon (char key)
 {
-  const char *title = "";
+  GtkWidget *image = NULL;
 
   switch (key) {
   case 'E': case 'e':
-    title = "Error";
+    image = gtk_image_new_from_stock (GTK_STOCK_DIALOG_ERROR,
+                                      GTK_ICON_SIZE_DIALOG);
     break;
 
   case 'I': case 'i':
-    title = "Information";
+    image = gtk_image_new_from_stock (GTK_STOCK_DIALOG_INFO,
+                                      GTK_ICON_SIZE_DIALOG);
     break;
 
   case 'L': case 'l':
-    title = "Prompt";
     break;
 
   case 'P': case 'p':
-    title = "Prompt";
     break;
 
   case 'Q': case 'q':
-    title = "Question";
+    image = gtk_image_new_from_stock (GTK_STOCK_DIALOG_QUESTION,
+                                      GTK_ICON_SIZE_DIALOG);
     break;
   }
 
-  return title;
+  return image;
 }
 
+
 /* Callback for dialogs that get WM_DELETE_WINDOW.  We pop down
    the dialog, but return TRUE so the event does not propagate further
    in GTK.  This prevents GTK from destroying the dialog widget automatically
@@ -1446,6 +1448,149 @@ dialog_delete_callback (GtkWidget *w, GdkEvent *event, gpointer user_data)
   return TRUE;
 }
 
+struct xg_popup_dialog_callback_data
+{
+  void (*select_cb) (GtkWidget *, gpointer);
+  void (*deactivate_cb) (GtkWidget *, gpointer);
+  gboolean multichoice_p;
+  GSList *radio_buttons;
+};
+
+static void
+popup_dialog_response_cb (GtkDialog *dialog,
+                          gint response_id,
+                          gpointer user_data)
+{
+  struct xg_popup_dialog_callback_data *data = user_data;
+
+  switch (response_id)
+    {
+    case GTK_RESPONSE_ACCEPT:
+      {
+        GSList *radio_button;
+        for (radio_button = data->radio_buttons;
+             radio_button != NULL;
+             radio_button = radio_button->next)
+          if (gtk_toggle_button_get_active
+              (GTK_TOGGLE_BUTTON (radio_button->data)))
+            {
+              gpointer val_data = g_object_get_data
+                (G_OBJECT (radio_button->data), XG_RADIO_BUTTON_DATA);
+
+              data->select_cb (GTK_WIDGET (dialog), val_data);
+              break;
+            }
+      }
+      break;
+    case GTK_RESPONSE_DELETE_EVENT:
+      data->deactivate_cb (GTK_WIDGET (dialog), NULL);
+      return;
+      break;
+    case GTK_RESPONSE_REJECT:
+      break;
+    default:
+      if (!data->multichoice_p)
+        {
+          GObject *object = G_OBJECT (gtk_dialog_get_widget_for_response
+                                      (GTK_DIALOG (dialog), response_id));
+          gpointer val_data = g_object_get_data (object, XG_RADIO_BUTTON_DATA);
+
+          data->select_cb (GTK_WIDGET (dialog), val_data);
+        }
+      break;
+    }
+
+  data->deactivate_cb (GTK_WIDGET (dialog), NULL);
+  g_free (user_data);
+}
+
+
+
+static inline GtkWidget *
+popup_dialog_create_simple_or_multichoice (gboolean multichoice_p)
+{
+  GtkWidget *dialog;
+  if (multichoice_p)
+    dialog = gtk_dialog_new_with_buttons ("",
+                                          NULL,
+                                          GTK_DIALOG_MODAL |
+                                          GTK_DIALOG_DESTROY_WITH_PARENT,
+                                          GTK_STOCK_OK,
+                                          GTK_RESPONSE_ACCEPT,
+                                          GTK_STOCK_CANCEL,
+                                          GTK_RESPONSE_REJECT,
+                                          NULL);
+  else
+    {
+      dialog = gtk_dialog_new ();
+      g_object_set (dialog,
+                    "destroy-with-parent" , TRUE,
+                    "modal"               , TRUE,
+                    "title"               , "",
+                    NULL);
+    }
+  g_object_set (dialog,
+                "name"                , "emacs-dialog",
+                "resizable"           , FALSE,
+                "skip-taskbar-hint"   , TRUE,
+                NULL);
+
+  return dialog;
+}
+
+static inline void
+popup_dialog_create_and_pack_message (GtkBox *vbox, char *utf8_label)
+{
+  GtkRequisition req;
+  gchar *message, *escaped_text;
+
+  GtkWidget *w = gtk_label_new (NULL);
+  g_object_set (w,
+                "wrap"       , TRUE,
+                "use-markup" , TRUE,
+                "selectable" , TRUE,
+                "xalign"     , 0.0,
+                "yalign"     , 0.5,
+                NULL);
+
+  escaped_text = g_markup_escape_text (utf8_label, -1);
+  message = g_strconcat ("<span weight=\"bold\" size=\"larger\">",
+                         escaped_text, "</span>", NULL);
+
+  gtk_label_set_markup (GTK_LABEL (w), message);
+
+  g_free (message);
+  g_free (escaped_text);
+
+  gtk_box_pack_start (GTK_BOX (vbox), w, TRUE, TRUE, 0);
+
+  /* Try to make dialog look better.  Must realize first so
+     the widget can calculate the size it needs.  */
+  gtk_widget_realize (w);
+  gtk_widget_get_preferred_size (w, NULL, &req);
+  gtk_box_set_spacing (GTK_BOX (vbox), req.height);
+}
+
+static inline GtkWidget *
+popup_dialog_add_radio_button (GtkBox *box,
+                               GtkWidget *previous_radio_button,
+                               char *utf8_label,
+                               gboolean enabled)
+{
+
+  GtkWidget *w = gtk_radio_button_new_with_label_from_widget
+    (GTK_RADIO_BUTTON (previous_radio_button),
+     utf8_label);
+
+  if (!enabled)
+    gtk_widget_set_sensitive (w, FALSE);
+
+  gtk_box_pack_start (GTK_BOX (box), w, TRUE, TRUE, 0);
+
+  return w;
+}
+
+
 /* Create a popup dialog window.  See also xg_create_widget below.
    WV is a widget_value describing the dialog.
    SELECT_CB is the callback to use when a button has been pressed.
@@ -1458,102 +1603,143 @@ create_dialog (widget_value *wv,
                GCallback select_cb,
                GCallback deactivate_cb)
 {
-  const char *title = get_dialog_title (wv->name[0]);
-  int total_buttons = wv->name[1] - '0';
-  int right_buttons = wv->name[4] - '0';
-  int left_buttons;
-  int button_nr = 0;
-  int button_spacing = 10;
-  GtkWidget *wdialog = gtk_dialog_new ();
-  GtkDialog *wd = GTK_DIALOG (wdialog);
-  GtkBox *cur_box = GTK_BOX (gtk_dialog_get_action_area (wd));
-  widget_value *item;
-  GtkWidget *whbox_down;
+  int total_buttons      = wv->name[1] - '0';
+  gboolean multichoice_p = total_buttons > 2;
+
+  /*
+     Depending on the `total_buttons' value the dialog created can be
+    of two types:
+
+    1. when multichoice_p is TRUE, then resulting dialog has following
+    structure:
+
+    +------- content_area -------------------------+
+    | +-------- hbox ----------------------------+ |
+    | | +-------+ +-- vbox --------------------+ | |
+    | | |       | | <message>                  | | |
+    | | | image | | +------ choices_box -----+ | | |
+    | | |       | | |  o first item          | | | |
+    | | |       | | |  o second item         | | | |
+    | | |       | | |  o third item          | | | |
+    | | |       | | |  ...                   | | | |
+    | | |       | | +------------------------+ | | |
+    | | +-------+ +----------------------------+ | |
+    | +------------------------------------------+ |
+    +----------------------------------------------+
+    +------- action_area --------------------------+
+    |                     +------+  +--------+     |
+    |                     |  OK  |  | Cancel |     |
+    |                     +------+  +--------+     |
+    +----------------------------------------------+
+
+
+    2. when multichoice_p is FALSE, its structure is this:
+
+    +------------- content_area ------------------------------+
+    |  +--------- hbox -------------------------------------+ |
+    |  | +---------+ +------ vbox ------------------------+ | |
+    |  | |         | | <message>                          | | |
+    |  | | image   | |                                    | | |
+    |  | |         | |                                    | | |
+    |  | +---------+ +------------------------------------+ | |
+    |  +----------------------------------------------------+ |
+    +---------------------------------------------------------+
+    +------- action_area -------------------------------------+
+    |                             +---------+  +--------+     |
+    |                             |  Item 1 |  | Item 2 |     |
+    |                             +---------+  +--------+     |
+    +---------------------------------------------------------+
+
+   */
+
+
+  GtkWidget *dialog = popup_dialog_create_simple_or_multichoice (multichoice_p);
+  GtkWidget *content_area, *hbox, *vbox, *frame, *choices_box;
+  GtkWidget *w = NULL;
+
+
+  content_area = gtk_dialog_get_content_area (GTK_DIALOG (dialog));
+
+  hbox = gtk_hbox_new (FALSE, 12);
+  gtk_container_set_border_width (GTK_CONTAINER (hbox), 5);
+
+  vbox = gtk_vbox_new (FALSE, 12);
 
-  /* If the number of buttons is greater than 4, make two rows of buttons
-     instead.  This looks better.  */
-  int make_two_rows = total_buttons > 4;
-
-  if (right_buttons == 0) right_buttons = total_buttons/2;
-  left_buttons = total_buttons - right_buttons;
+  {
+    GtkWidget *image = get_dialog_icon (wv->name[0]);
+    gtk_misc_set_alignment (GTK_MISC (image), 0.5, 0.0);
 
-  gtk_window_set_title (GTK_WINDOW (wdialog), title);
-  gtk_widget_set_name (wdialog, "emacs-dialog");
+    gtk_box_pack_start (GTK_BOX (hbox), image, FALSE, FALSE, 0);
+  }
 
+  gtk_box_pack_start (GTK_BOX (hbox), vbox, FALSE, FALSE, 0);
+  gtk_container_add (GTK_CONTAINER (content_area), hbox);
 
-  if (make_two_rows)
+  if (multichoice_p)
     {
-      GtkWidget *wvbox = gtk_vbox_new (TRUE, button_spacing);
-      GtkWidget *whbox_up = gtk_hbox_new (FALSE, 0);
-      whbox_down = gtk_hbox_new (FALSE, 0);
+      frame  = gtk_frame_new ("Possible actions/answers");
+      choices_box = gtk_vbox_new (FALSE, 1);
+    }
 
-      gtk_box_pack_start (cur_box, wvbox, FALSE, FALSE, 0);
-      gtk_box_pack_start (GTK_BOX (wvbox), whbox_up, FALSE, FALSE, 0);
-      gtk_box_pack_start (GTK_BOX (wvbox), whbox_down, FALSE, FALSE, 0);
+  {
+    widget_value *item;
+    gint button_number = 0;
+    for (item = wv->contents; item; item = item->next)
+      {
+        char *utf8_label = get_utf8_string (item->value);
 
-      cur_box = GTK_BOX (whbox_up);
+        if (g_strcmp0 (item->name, "message") == 0)
+          popup_dialog_create_and_pack_message (GTK_BOX (vbox), utf8_label);
+        else
+          {
+            if (!multichoice_p)
+              {
+                gtk_dialog_add_button (GTK_DIALOG (dialog),
+                                       utf8_label, button_number);
+                w = gtk_dialog_get_widget_for_response (GTK_DIALOG (dialog),
+                                                        button_number++);
+              }
+            else
+              w = popup_dialog_add_radio_button (GTK_BOX (choices_box), w,
+                                                 utf8_label,
+                                                 item->enabled);
+            g_object_set_data (G_OBJECT (w), XG_RADIO_BUTTON_DATA,
+                               item->call_data);
+          }
+
+        g_free (utf8_label);
+      }
+  }
+
+  if (multichoice_p)
+    {
+      gtk_container_add (GTK_CONTAINER (frame), choices_box);
+      gtk_box_pack_start (GTK_BOX (vbox), frame, TRUE, TRUE, 0);
     }
 
-  g_signal_connect (G_OBJECT (wdialog), "delete-event",
+  g_signal_connect (G_OBJECT (dialog), "delete-event",
                     G_CALLBACK (dialog_delete_callback), 0);
 
   if (deactivate_cb)
-    {
-      g_signal_connect (G_OBJECT (wdialog), "close", deactivate_cb, 0);
-      g_signal_connect (G_OBJECT (wdialog), "response", deactivate_cb, 0);
-    }
+      g_signal_connect (G_OBJECT (dialog), "close", deactivate_cb, 0);
 
-  for (item = wv->contents; item; item = item->next)
+  if (select_cb)
     {
-      char *utf8_label = get_utf8_string (item->value);
-      GtkWidget *w;
-      GtkRequisition req;
+      struct xg_popup_dialog_callback_data *data = g_malloc (sizeof (*data));
 
-      if (item->name && strcmp (item->name, "message") == 0)
-        {
-          GtkBox *wvbox = GTK_BOX (gtk_dialog_get_content_area (wd));
-          /* This is the text part of the dialog.  */
-          w = gtk_label_new (utf8_label);
-          gtk_box_pack_start (wvbox, gtk_label_new (""), FALSE, FALSE, 0);
-          gtk_box_pack_start (wvbox, w, TRUE, TRUE, 0);
-          gtk_misc_set_alignment (GTK_MISC (w), 0.1, 0.5);
-
-          /* Try to make dialog look better.  Must realize first so
-             the widget can calculate the size it needs.  */
-          gtk_widget_realize (w);
-          gtk_widget_get_preferred_size (w, NULL, &req);
-          gtk_box_set_spacing (wvbox, req.height);
-	  if (item->value && strlen (item->value) > 0)
-            button_spacing = 2*req.width/strlen (item->value);
-        }
-      else
-        {
-          /* This is one button to add to the dialog.  */
-          w = gtk_button_new_with_label (utf8_label);
-          if (! item->enabled)
-            gtk_widget_set_sensitive (w, FALSE);
-          if (select_cb)
-            g_signal_connect (G_OBJECT (w), "clicked",
-                              select_cb, item->call_data);
-
-          gtk_box_pack_start (cur_box, w, TRUE, TRUE, button_spacing);
-          if (++button_nr == left_buttons)
-            {
-              if (make_two_rows)
-                cur_box = GTK_BOX (whbox_down);
-              else
-                gtk_box_pack_start (cur_box,
-                                    gtk_label_new (""),
-                                    TRUE, TRUE,
-                                    button_spacing);
-            }
-        }
+      data->select_cb = (void (*) (GtkWidget *, gpointer)) select_cb;
+      data->multichoice_p = multichoice_p;
+      data->radio_buttons = (multichoice_p) ?
+        gtk_radio_button_get_group (GTK_RADIO_BUTTON (w)) : NULL;
+
+      if (deactivate_cb)
+        data->deactivate_cb = (void (*) (GtkWidget *, gpointer)) deactivate_cb;
 
-     if (utf8_label)
-       g_free (utf8_label);
+      g_signal_connect (G_OBJECT (dialog), "response",
+                        G_CALLBACK (popup_dialog_response_cb), data);
     }
 
-  return wdialog;
+  return dialog;
 }
 
 struct xg_dialog_data
diff --git a/src/gtkutil.h b/src/gtkutil.h
index 7cc2d21..9c55fa6 100644
--- a/src/gtkutil.h
+++ b/src/gtkutil.h
@@ -38,6 +38,8 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 /* Key for data that menu items hold.  */
 #define XG_ITEM_DATA "emacs_menuitem"
 
+#define XG_RADIO_BUTTON_DATA "emacs_radio_button"
+
 /* This is a list node in a generic list implementation.  */
 typedef struct xg_list_node_
 {
diff --git a/src/xmenu.c b/src/xmenu.c
index 4b7bbfd..547b538 100644
--- a/src/xmenu.c
+++ b/src/xmenu.c
@@ -1904,8 +1904,6 @@ dialog_selection_callback (GtkWidget *widget, gpointer client_data)
      as long as pointers have enough bits to hold small integers.  */
   if ((intptr_t) client_data != -1)
     menu_item_selection = (Lisp_Object *) client_data;
-
-  popup_activated_flag = 0;
 }
 
 /* Pop up the dialog for frame F defined by FIRST_WV and loop until the
-- 
1.7.5.4

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Jan D.]

Andrey Smirnov skrev 2011-12-15 05:56:
> On Wed, Dec 14, 2011 at 12:41 PM, Jan Djärv<jan.h.d@swipnet.se>  wrote:
>> If you Alt-tab in icewm for example, it shows the titles, see
>> attached picture.
>
> Now, I see your point. On the other hand in Unity window title doesn't
> play that important role in window-switching. Can we compromise on
> exposing run-time variable something along the lines of
> `gtk-popup-dialogs-show-titles' and then the appearance could be
> controlled from user's .emacs?
>

I think we should keep the same title as we had before.  Those that need 
it really need it, and those that don't won't notice it.

> All right, in for a penny in for a pound, how about
> `gtk-popup-dialogs-message-format' to control that aspect?

I'm not opposed to using larger and bold, but I think we need some sort 
of poll, or a descision from the maintainers.

>
>>
>> Perhaps, but then we maybe should change all ports?
>
> Do you mean change how dialogs look in all toolkits?

Actually I was just talking about larger and bold here.

>
> It still won't change the fact that original
> `dialog_selection_callback' duplicates the functionality of
> `popup_deactivate_callback'.

Yes, but that is something we won't want to change, as some toolkits 
can't chain callbacks as Gtk+ can.  And it forces changes for other 
toolkits as well, for a small gain in clarity.

>
>
> I attached reworked version of the patch, it still lacks comments with
> description for new functions, but I'll add it as soon as the code
> stabilizes.
>

You are still not resetting popup_activated_flag.

+struct xg_popup_dialog_callback_data
+{
+  void (*select_cb) (GtkWidget *, gpointer);
+  void (*deactivate_cb) (GtkWidget *, gpointer);
+  gboolean multichoice_p;
+  GSList *radio_buttons;
+};
+

You could use GCallback here and cast functions with GCALLBACK().  It is 
more Gtk-ish.  Otherwise, make a typedef like:
   typedef void (*Xg_CBfunction) (GtkWidget *, gpointer);
and use Xg_CBfunction.


+      struct xg_popup_dialog_callback_data *data = g_malloc (sizeof 
(*data));

This is never free:d.

	Jan D.

Re: [PATCH] Change the look of dialogs created with `x-popup-dialog'

[Andrey Smirnov]

On Fri, Dec 16, 2011 at 5:42 AM, Jan D. <jan.h.d@swipnet.se> wrote:
> Andrey Smirnov skrev 2011-12-15 05:56:
>
>> On Wed, Dec 14, 2011 at 12:41 PM, Jan Djärv<jan.h.d@swipnet.se>  wrote:
>>>
>>> If you Alt-tab in icewm for example, it shows the titles, see
>>> attached picture.
>>
>>
>> Now, I see your point. On the other hand in Unity window title doesn't
>> play that important role in window-switching. Can we compromise on
>> exposing run-time variable something along the lines of
>> `gtk-popup-dialogs-show-titles' and then the appearance could be
>> controlled from user's .emacs?
>>
>
> I think we should keep the same title as we had before.  Those that need it
> really need it, and those that don't won't notice it.

Not true, I don't need it but notice how, IMHO, ugly it is. :)
But, of course, if you insist, I'll add it.

>> All right, in for a penny in for a pound, how about
>> `gtk-popup-dialogs-message-format' to control that aspect?
>
>
> I'm not opposed to using larger and bold, but I think we need some sort of
> poll, or a descision from the maintainers.
>
>
>>
>>>
>>> Perhaps, but then we maybe should change all ports?
>>
>>
>> Do you mean change how dialogs look in all toolkits?
>
>
> Actually I was just talking about larger and bold here.
>

Oh, OK, but I'm still looking into other toolkit's code.

>>
>> It still won't change the fact that original
>> `dialog_selection_callback' duplicates the functionality of
>> `popup_deactivate_callback'.
>
>
> Yes, but that is something we won't want to change, as some toolkits can't
> chain callbacks as Gtk+ can.  And it forces changes for other toolkits as
> well, for a small gain in clarity.

There are two versions of `dialog_selection_callback' already:
Gtk-specific and `dialog_selection_callback' for any other toolkit.
How do my changes to Gtk-specific version force change on any other
toolkits?

And in this version of the patch even the chaining isn't needed
anymore.

>>
>> I attached reworked version of the patch, it still lacks comments with
>> description for new functions, but I'll add it as soon as the code
>> stabilizes.
>>
>
> You are still not resetting popup_activated_flag.

I know, see my comment above.

>
> +struct xg_popup_dialog_callback_data
> +{
> +  void (*select_cb) (GtkWidget *, gpointer);
> +  void (*deactivate_cb) (GtkWidget *, gpointer);
> +  gboolean multichoice_p;
> +  GSList *radio_buttons;
> +};
> +
>
> You could use GCallback here and cast functions with GCALLBACK().  It is
> more Gtk-ish.

And then I would still, have to cast them to `void (*) (GtkWidget *,
gpointer)' in order to actually call them in `popup_dialog_response_cb'.

> Otherwise, make a typedef like:
>  typedef void (*Xg_CBfunction) (GtkWidget *, gpointer);
> and use Xg_CBfunction.

OK, since I already defined the structure whose definition isn't used
more than twice, I think I'll stick with this option.

>
> +      struct xg_popup_dialog_callback_data *data = g_malloc (sizeof
> (*data));
>
> This is never free:d.

Am I missing something and the last line in
`popup_dialog_response_cb':

g_free (user_data);

is never reached?

Although it was true for a piece of code(case
GTK_RESPONSE_DELETE_EVENT) I forgot to correct. Already fixed that.

Andrey Smirnov