Hello.14 dec 2011 kl. 06:29 skrev Andrey Smirnov: On Mon, Dec 12, 2011 at 11:07 PM, Jan Djärv < address@hidden> 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.
|