The custom format "---" causes the separator menu item to be activated instead of the "Custom" menu item. Reproduce by selecting custom date format in the properties dialog, entering the string "---" for the date format, clicking close. The plugin in displays "---" for the date. Display the properties dialog again. The date format menu displays "---" instead of "Custom...". I have an updated patch in the works for bug 4104 that addresses this problem.
Created attachment 1656 bug fix; the test case is now a single dash ("-") This patch fixes this bug. The essential test case is to configure a custom format with a single dash ("-"). I removed the RCS tag "$Id$" because it was expanded when I did an RCS checkin. They are in the other source files too. Braces are not required around array initializers that are structs. IMO, they would not improve readability in this case. http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Arrays-of-Structures Patch is against trunk.
I insist that the braces are a good thing: cc1: warnings being treated as errors datetime-dialog.c:62: warning: missing braces around initializer datetime-dialog.c:62: warning: (near initialization for ‘dt_combobox_date[0]’) datetime-dialog.c:79: warning: missing braces around initializer datetime-dialog.c:79: warning: (near initialization for ‘dt_combobox_time[0]’) make[2]: *** [libdatetime_la-datetime-dialog.lo] Error 1 make[2]: Leaving directory `/home/ongardie/xfce/xfce4-datetime-plugin/panel-plugin' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/ongardie/xfce/xfce4-datetime-plugin' make: *** [all] Error 2 ongardie@lappy:~/xfce/xfce4-datetime-plugin$
In response to Comment #1: > This patch fixes this bug. Agreed. > I removed the RCS tag "$Id$" because it was expanded when I did an RCS checkin. > They are in the other source files too. They seem to be there in many xfce modules. What do you suggest I do? I was pretty close to checking this in, until I reread this line: gtk_combo_box_set_active(GTK_COMBO_BOX(date_combobox), DT_COMBOBOX_DATE_COUNT-1); I think it largely defeats the purpose of classifying the items into different types if there's an arbitrary requirement that the custom type must be at the end. I'm still not sure of how I'd *like* this to work.
Created attachment 1657 add braces per compiler warning; use "gchar" instead of char Resubmitting with braces in array initialization. This also replaces "char" with "gchar". (In reply to comment #2) > I insist that the braces are a good thing: Thanks for insisting. :-) To get those warnings, I needed to enable: CFLAGS="$CFLAGS -Wall -Werror" in configure.in.in. Is there a better way?
(In reply to comment #3) > > I removed the RCS tag "$Id$" because it was expanded when I did an RCS checkin. > > They are in the other source files too. > They seem to be there in many xfce modules. What do you suggest I do? They are inconsistently used: [stephent@cedar libxfcegui4]$ pwd /home/stephent/src/xfce/libxfcegui4-4.4.2/libxfcegui4 [stephent@cedar libxfcegui4]$ fgrep -l '$Id' *.[ch] | wc -l 22 [stephent@cedar libxfcegui4]$ fgrep -L '$Id' *.[ch] | wc -l 75 I believe they could be removed. > I was pretty close to checking this in, until I reread this line: > gtk_combo_box_set_active(GTK_COMBO_BOX(date_combobox), > DT_COMBOBOX_DATE_COUNT-1); > I think it largely defeats the purpose of classifying the items into different > types if there's an arbitrary requirement that the custom type must be at the > end. Good catch! I was aware of that and agree that it is theoretically problematic. In reality, the code has to work and be easy to maintain, so I am not really troubled. I settled this by asking myself: "How many more 'Custom' menu items does datetime need to support?". > I'm still not sure of how I'd *like* this to work. The "type" member could be expanded to a "flags" member that has three bits for the types and one bit for whether the item is "Custom" or not. Something like: typedef enum { DT_COMBOBOX_ITEM_IS_FORMAT = (1 << 0), /* format string that is replaced with an example date or time */ DT_COMBOBOX_ITEM_IS_TEXT = (1 << 1), /* literal item text, possibly with translation */ DT_COMBOBOX_ITEM_IS_SEPARATOR = (1 << 2), /* inactive separator */ DT_COMBOBOX_ITEM_IS_CUSTOM = (1 << 3), /* item enables custom format entry */ } dt_combobox_item_flags; The "Custom" item would then be initialized with: { N_("Custom..."), DT_COMBOBOX_ITEM_IS_TEXT | DT_COMBOBOX_ITEM_IS_CUSTOM } What do you think?
Created attachment 1659 rename menu item types; find custom item dynamically This renames the menu item types to better describe their function. The custom menu item is found dynamically, rather than being fixed in the last position. In principle, the custom menu item could go anywhere now.
(In reply to comment #4) > To get those warnings, I needed to enable: > CFLAGS="$CFLAGS -Wall -Werror" > in configure.in.in. > > Is there a better way? Rerun the configure script with --enable-debug. To get the last line executed for the configure script run `head config.log'. --enable-debug=full will fail to build on warnings.
Committed in r4919 and 4921. I took advantage of the ability to add more separators in r4920 :) Thanks for the patches, Steve, and for being so patient. I think I'll do a release in the next couple days. I'll probably call it v1.0 to make myself feel special.
(In reply to comment #7) ... > Rerun the configure script with --enable-debug. To get the last line executed > for the configure script run `head config.log'. --enable-debug=full will fail > to build on warnings. Thanks Mike. Your tip works fine. $ ./configure --prefix=/usr --enable-debug=full ... $ grep '^CFLAGS' Makefile CFLAGS = -g -O2 -g3 -Werror -Wall -DXFCE_DISABLE_DEPRECATED
(In reply to comment #8) > Committed in r4919 and 4921. I took advantage of the ability to add more > separators in r4920 :) > > Thanks for the patches, Steve, and for being so patient. I think I'll do a > release in the next couple days. I'll probably call it v1.0 to make myself feel > special. Thanks! The additional separators and new formats look great. A 1.0 release sounds fine, but IMO, it should include a fix for Bug 4097.