GNOME Bugzilla – Bug 339540
GtkCalender: displaying extra information for each day
Last modified: 2008-03-04 14:03:16 UTC
It would be nice if GtkCalendar could display extra information on each day - such as information about events.
Created attachment 64186 [details] [review] gtkcalendar_extra_display_func.patch Here is a half-finished patch that I might finish some day. I need to figure out how to calculate the layout extents without calling the callback function too often.
Created attachment 97756 [details] [review] [1/1] Finish the patch for bug 339540. gtk/gtkcalendar.c | 118 ++++++++++++++++++++++++++-- gtk/gtkcalendar.h | 11 +++ tests/testcalendar.c | 213 +++++++++++++++++++++++++++++++++++++------------- 3 files changed, 279 insertions(+), 63 deletions(-)
Created attachment 97757 [details] That's what it looks like now.
Mathias, please write a ChangeLog entry for this, stating what API it adds, and mentioning that it's used in the test. That makes it easier to review. moap's "moap changelog prepare" is great for this and it works with cvs, svn, git, etc.
I'd really like to see a new calendar widget, the current API is pretty broken. Just have a look at the Tasks source for several hacks just to make a calendar popup work.
Yeah, a new calendar widget might be nice. In the meantime, I need this one little feature for Glom.
Yeah, the calendar is not exactly the nicest widget we have. And I'm a little bit dubious about the general usefulness of this. Also, the example doesn't really show how this looks when used in practise. Can you show how this looks if only some of the days have details, and different details of various sizes. How does the bolding of the current day interact with the markup in the detail, etc.
Created attachment 100999 [details] [review] Display extra information for each day in GtkCalendar (In reply to comment #7) > Can you show how this looks if only some of the days have details, and > different details of various sizes. How does the bolding of the current day > interact with the markup in the detail, etc. Ok, checking this revealed some issues. This update introduces new "detail-width-chars" and "detail-height-rows" properties for fixating the detail area's size. Without fixation GtkCalender calls gtk_widget_queue_resize, instead of gtk_widget_queue_draw now.
Created attachment 101002 [details] Demonstration of the patch
Haven't looked at the patch at all yet, but visually, I have to say that I think those separators are too dominant. Can we make those lines thinner, and/or the gap between them wider ?
Created attachment 101096 [details] [review] [PATCH] Add infrastructure for GtkCalendar details. (#339540) * gtk/gtkcalendar.c, gtk/gtkcalendar.h: Add "detail-width-chars" and "detail-height-rows" properties, and gtk_calendar_set_detail_func function. --- ChangeLog | 8 +++ gtk/gtkcalendar.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++-- gtk/gtkcalendar.h | 20 ++++++++ 3 files changed, 155 insertions(+), 6 deletions(-)
Created attachment 101097 [details] [review] [PATCH] Avoid some compiler warnings and remove obsolete code. (#339540) * gtk/gtkcalendar.c: Change week and year variable in calendar_paint_week_numbers from gint to guint. Remove obsolete "#if 0" block from calendar_paint_day: The feature in question is handled few lines above. Cast data returned by gtk_selection_data_get_text() to (gchar*) in gtk_calendar_drag_data_received. --- ChangeLog | 10 ++++++++++ gtk/gtkcalendar.c | 16 +++++++--------- 2 files changed, 17 insertions(+), 9 deletions(-)
Created attachment 101098 [details] [review] [PATCH] Consider in size-request and show calender details. (#339540) * gtk/gtkcalendar.c: Add gtk_calendar_get_detail and is_color_attribute functions. Change gtk_calendar_size_request and calendar_paint_day to consider and show calender details. --- ChangeLog | 8 ++ gtk/gtkcalendar.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 188 insertions(+), 7 deletions(-)
Created attachment 101099 [details] [review] [PATCH] Without explicitly set "detail-width-chars" and "detail-height-rows" properties not only the widget has to be redrawn on certain conditions, but also its size must be recalculated. (#339540) * gtk/gtkcalendar.c: Add calendar_queue_refresh and call that function instead of gtk_widget_queue_draw. --- ChangeLog | 9 +++++++++ gtk/gtkcalendar.c | 33 ++++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 11 deletions(-)
Created attachment 101100 [details] [review] [PATCH] Implement GtkTooltip API for calendar details. (#339540) * gtk/gtkcalendar.c: Add gtk_calendar_query_tooltip and chain it up. --- ChangeLog | 6 ++++++ gtk/gtkcalendar.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 0 deletions(-)
Created attachment 101101 [details] [review] [PATCH] Restructure testcalendar for testing calendar details. (#339540) * tests/testcalendar.c: Move code arround to test calendar details. --- ChangeLog | 6 + tests/testcalendar.c | 549 ++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 421 insertions(+), 134 deletions(-)
Created attachment 101102 [details] [review] [PATCH] Try more decent appearance of calendar details separator. (#339540) * gtk/gtkcalendar.c: Use different colors for drawing the separator, and make it short by one pixel on each side. --- ChangeLog | 7 +++++++ gtk/gtkcalendar.c | 15 +++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)
So I've splitted the patch into smaller pieces for better review and with the last patch I've tried to give the details separator a more decent appearance.
Created attachment 101103 [details] Updated separator appearance with my custom theme
Created attachment 101104 [details] Updated separator appearance with Clearlooks
Created attachment 101105 [details] Updated separator appearance with Raleigh
Good stuff... Mathias, does it support hiding the extra information and show it as a tooltip (boldening entries with information)?
While we are adding feature requests, I always thought a way to add custom markers (ie little icon overlays) would be cute.
Created attachment 101144 [details] [review] [PATCH] Add GTK_CALENDAR_SHOW_DETAILS display flag, which chooses if details are shown within the widget, or jst as tooltip. * gtk/gtkcalendar.c, gtk/gtkcalendar.h: Add "show-details" property aka. GTK_CALENDAR_SHOW_DETAILS, and use it. * tests/testcalendar.c: Test GTK_CALENDAR_SHOW_DETAILS. Reduce padding in flags vbox. --- ChangeLog | 10 ++++++++ gtk/gtkcalendar.c | 61 ++++++++++++++++++++++++++++++++++++++----------- gtk/gtkcalendar.h | 3 +- tests/testcalendar.c | 11 +++++---- 4 files changed, 65 insertions(+), 20 deletions(-)
(In reply to comment #23) > While we are adding feature requests, I always thought a way to add custom > markers (ie little icon overlays) would be cute. > Considering this request is serious: What kind of API do you have in mind? Just provide a pixbuf for each day? Allow drawing to a cairo context for each day? Do you have some mockups somewhere?
(In reply to comment #25) > (In reply to comment #23) > > While we are adding feature requests, I always thought a way to add custom > > markers (ie little icon overlays) would be cute. > > > > Considering this request is serious: What kind of API do you have in mind? Just > provide a pixbuf for each day? Allow drawing to a cairo context for each day? > > Do you have some mockups somewhere? Something like Nautilus emblems I assume.
> Something like Nautilus emblems I assume. Unless there's somone who is actually likely to use this feature I don't think it's worth the effort and code.
(In reply to comment #27) > > Something like Nautilus emblems I assume. > > Unless there's somone who is actually likely to use this feature I don't think > it's worth the effort and code. > Nevertheless this topic is important for deciding, whether a simple callback is sufficient, of if some interface in the spirit of GtkCellRenderer should be used.
In the last patch, the doc comment says hide-details, while the actual property name is show-details.
Not reviewing the patches in detail yet, just some more comments on the general behaviour, based on the ogg you attached earlier: - It appears that the max-width/height are also min-width/height ? at least, I see all cells being sized to 8x3 even if there is only a single details with a single line... - If the text does not fit, I think you should get some visual indication about the hidden stuff, and probably the tooltip showing the full text. - I'd be interested to see how the font-based sizing plays with <size> tags in the markup... - How about copy and paste ? I know in-place selection is probably out of the question, but maybe a context menu is doable. If you consider that out-of-scope, it is probably fine to leave it out initially. Other than that, the separator looks much better now, thanks. On to reviewing the patches...
Comment on attachment 101096 [details] [review] [PATCH] Add infrastructure for GtkCalendar details. (#339540) >+ /* Size requistion for details provided by the hook. */ >+ gint8 detail_height_rows; >+ gint8 detail_width_chars; > }; I'd just make those straight ints. Saving a few bytes here doesn't really buy us anything. >+ /* Call the destroy function for the extra display callback: */ >+ if (priv->detail_func_destroy && priv->detail_func_user_data) >+ { >+ priv->detail_func_destroy (priv->detail_func_user_data); >+ priv->detail_func_user_data = NULL; >+ } You probably want to reset detail_func_destroy too, here ? >+void >+gtk_calendar_set_detail_width_chars (GtkCalendar *calendar, >+ gint chars) >+{ >+ g_return_if_fail (GTK_IS_CALENDAR (calendar)); >+ g_object_set (calendar, I_("detail-width-chars"), chars, NULL); >+} The usual pattern for setters in gtk is to do the actual setting in the setter, and call the setter from the generic property_set function. Any reason to do things differently here ? Also, we typically avoid the change notification if the value is unchanged, for int properties.
(In reply to comment #31) > (From update of attachment 101096 [details] [review] [edit]) > > >+ /* Size requistion for details provided by the hook. */ > >+ gint8 detail_height_rows; > >+ gint8 detail_width_chars; > > }; > > I'd just make those straight ints. Saving a few bytes here doesn't really buy > us anything. Just wanted to please Havoc here. Also prefer gint for alignment. Will change that. > >+ /* Call the destroy function for the extra display callback: */ > >+ if (priv->detail_func_destroy && priv->detail_func_user_data) > >+ { > >+ priv->detail_func_destroy (priv->detail_func_user_data); > >+ priv->detail_func_user_data = NULL; > >+ } > > You probably want to reset detail_func_destroy too, here ? Duh! > >+void > >+gtk_calendar_set_detail_width_chars (GtkCalendar *calendar, > >+ gint chars) > >+{ > >+ g_return_if_fail (GTK_IS_CALENDAR (calendar)); > >+ g_object_set (calendar, I_("detail-width-chars"), chars, NULL); > >+} > > The usual pattern for setters in gtk is to do the actual setting in the setter, > and call the setter from the generic property_set function. Any reason to do > things differently here ? > > Also, we typically avoid the change notification if the value is unchanged, for > int properties. I've adopted that pattern after talking with Tim Janik in June of this year (during the tooltip merge). He said calling g_object_set would be the way to go: (15:02:55) tbf: rambokid: regarding the tooltip-text patch: the properties are modified in _set_property and the accessor methods call g_object_set. shouldn't it be handled the other way arround: object is modified in the accessors and _set_properties calls the accessors for efficience and code style? ... (15:05:12) rambokid: tbf: no, i think this way the code is written much better. a) because the function accessors could possibly be left out, and b) because g_obejct_set also automatically does the needed property notification. if you want that in an accessor you have to do it by hand using g_object_notify. so ideally, all code would be written like this. (15:07:28) tbf: rambokid: ok, was considering the lookup overhead in g_object_set - but guess with g_object_notify you'd also have it. so i am convinced. (15:07:47) rambokid: you're right
Ok, checking for change in the integer properties would make sense: to avoid the property lookup and the notification.
Author: hasselmm <hasselmm@7eb1c76a-c725-0410-a3b5-a37faf6256f8> Date: Wed Dec 19 22:57:01 2007 +0000 Avoid some compiler warnings and remove obsolete code. (#339540) * gtk/gtkcalendar.c: Change week and year variable in calendar_paint_week_numbers from gint to guint. Remove obsolete "#if 0" block from calendar_paint_day: The feature in question is handled few lines above. Cast data returned by gtk_selection_data_get_text() to (gchar*) in gtk_calendar_drag_data_received.
Created attachment 101288 [details] [review] [1/1] Add infrastructure for GtkCalendar details. (#339540) * gtk/gtkcalendar.c, gtk/gtkcalendar.h: Add "detail-width-chars" and "detail-height-rows" properties, and gtk_calendar_set_detail_func function. --- ChangeLog | 8 +++ gtk/gtkcalendar.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++- gtk/gtkcalendar.h | 20 +++++++ 3 files changed, 174 insertions(+), 3 deletions(-)
Patch updated as requested in comment #31.
(In reply to comment #30) > Not reviewing the patches in detail yet, just some more comments on the general > behaviour, based on the ogg you attached earlier: > > - It appears that the max-width/height are also min-width/height ? at least, I > see all cells being sized to 8x3 even if there is only a single details with > a > single line... Its really fixed size. That hack is needed to avoid wild size-requests jumping on switching months. Dropped the word "Maximum". > - If the text does not fit, I think you should get some visual indication about > the hidden stuff, and probably the tooltip showing the full text. The tooltip already contains the full text. Do you think a small arrow would work? Enabling ellipses and wrapping at the same time does not really work in this very size restricted case. > - I'd be interested to see how the font-based sizing plays with <size> tags in > the markup... The generated markup already contains size tags... > - How about copy and paste ? I know in-place selection is probably out of the > question, but maybe a context menu is doable. If you consider that > out-of-scope, it is probably fine to leave it out initially. Guess a matter of copy and paste - all information needed is there. But would prefer doing that separately.
At some point I add pango_layout_set_height()...
I have tried your patches now; I think this is a decent addition to the calendar, so we should go ahead and add it. I have some more comments on visuals and behaviour, and then some detailed comments on the patches. First the general comments: About the separators: Should these respect the GtkWidget::wide-separators and GtkWidget::separator-height style properties ? I guess they probably should. About the tooltips: Can we arrange for the tooltip to only show if it actually adds something ? I find tooltips that only duplicate the text annoying. Also, can we make the tooltip move to the current day while hovering over the calendar ? See testtooltips for an example of this on the treeview.
Comment on attachment 101288 [details] [review] [1/1] Add infrastructure for GtkCalendar details. (#339540) The new api here needs docs.
Comment on attachment 101098 [details] [review] [PATCH] Consider in size-request and show calender details. (#339540) - g_snprintf (buffer, sizeof (buffer), Q_("calendar:day:digits|%d"), day); + g_snprintf (buffer, sizeof buffer, Q_("calendar:day:digits|%d"), day); Unrelated change, plus I prefer functional notation for sizeof. + colors = pango_attr_list_filter (attrs, is_color_attribute, NULL); I see (lt-testcalendar:3339): Pango-CRITICAL **: pango_attr_list_filter: assertion `list != NULL' failed
Comment on attachment 101100 [details] [review] [PATCH] Implement GtkTooltip API for calendar details. (#339540) See the general comments on tooltip behaviour
Comment on attachment 101101 [details] [review] [PATCH] Restructure testcalendar for testing calendar details. (#339540) gchar *key = g_strdup_printf ("%04d-%02d-%02d", year, month + 1, day); Seems pretty pointless to add 1 to the month here. Far too many coding style fixes mixed into this patch, it would be good to commit those separately.
Comment on attachment 101144 [details] [review] [PATCH] Add GTK_CALENDAR_SHOW_DETAILS display flag, which chooses if details - P_("Details Details Height"), + P_("Maximum Details Height"), I believe an earlier patch threw out the "Maximum" ? +/** + * GtkCalendar:hide-details: The property is "show-details", not "hide-details". It appears the default value for the property disagrees with the default flags.
Created attachment 101678 [details] [review] Add infrastructure for GtkCalendar details (rev. 3) (In reply to comment #40) > (From update of attachment 101288 [details] [review] [edit]) > The new api here needs docs. > Done.
Created attachment 101680 [details] [review] Consider calender details in size-request and show them (rev. 2) (In reply to comment #41) > (From update of attachment 101098 [details] [review] [edit]) > - g_snprintf (buffer, sizeof (buffer), Q_("calendar:day:digits|%d"), day); > + g_snprintf (buffer, sizeof buffer, Q_("calendar:day:digits|%d"), day); Unrelated change dropped. > + colors = pango_attr_list_filter (attrs, is_color_attribute, NULL); Guarded by NULL check now.
Created attachment 101682 [details] [review] Implement GtkTooltip API for calendar details (rev. 2) (In reply to comment #42) > (From update of attachment 101100 [details] [review] [edit]) > See the general comments on tooltip behaviour > Showing them at proper position now, and only when needed.
Created attachment 101683 [details] [review] Add GTK_CALENDAR_SHOW_DETAILS display flag (rev. 2) (In reply to comment #44) > (From update of attachment 101144 [details] [review] [edit]) > - P_("Details Details > Height"), > + P_("Maximum Details > Height"), > > I believe an earlier patch threw out the "Maximum" ? Some artifact, well and the patch was outdated. It also failed to apply at other places. > +/** > + * GtkCalendar:hide-details: > > The property is "show-details", not "hide-details". > > It appears the default value for the property disagrees with the default flags. The joys of inverting the meaning of a flag... Corrected.
Comment on attachment 101680 [details] [review] Consider calender details in size-request and show them (rev. 2) I think the color stripping may be worth a small comment explaining that color markup would conflict with selection marking Any comment on the idea to use separator width style properties ?
Comment on attachment 101682 [details] [review] Implement GtkTooltip API for calendar details (rev. 2) Would still be nice to have some visual overflow indication, have you investigated that any further ?
Comment on attachment 101683 [details] [review] Add GTK_CALENDAR_SHOW_DETAILS display flag (rev. 2) When I tried the last set of patches, it seemed that tooltips in the !show-details case didn't work anymore ?
2007-12-27 Mathias Hasselmann <mathias@openismus.com> Add infrastructure for GtkCalendar details. (#339540) * gtk/gtkcalendar.c, gtk/gtkcalendar.h, gtk/gtk.symbols: Add "detail-width-chars" and "detail-height-rows" properties, and gtk_calendar_set_detail_func function. 2007-12-27 Mathias Hasselmann <mathias@openismus.com> * reference/gtk/gtk-sections.txt: Add new GtkCalendar symbols.
Created attachment 101700 [details] [review] Consider details for size-request and expose-event (rev. 3) (In reply to comment #49) > (From update of attachment 101680 [details] [review] [edit]) > I think the color stripping may be worth a small comment explaining that color > markup would conflict with selection marking Agreed.
Created attachment 101702 [details] [review] Work in progress: Consider style properties for separator (In reply to comment #49) > Any comment on the idea to use separator width style properties ? > Here some work in progress. Also removes the max_detail_height field from GtkCalendarPrivate and corrects color selection for the details text: Currently details for 2007-11-27 are rendered as selected, on 2007-12-27. Open question: Can gtk_paint_box be told, to use foreground instead of background color of GTK_STATE_SELECTED state, or is that fine-tuning a theme author's task? Would like to apply that patch on top of the others (instead of merging it into attachment 101700 [details] [review]) to prevent regressions.
(In reply to comment #50) > (From update of attachment 101682 [details] [review] [edit]) > Would still be nice to have some visual overflow indication, have you > investigated that any further ? > Should be possible to render ellipses by doing something like this (pseudo-code): ellipses = new PangoGlyphString("...", last_line.last_glyph.attributes); ellipses.get_extends (null, &ebox); do { last_line.get_extends (null, &lbox); if (lbox.width + ebox.width <= day_rect.width) break; last_line.glyphs.remove_last (); } while (last_line.glyphs.length > 0); last_line.draw (cr, x, y, ...); ellipses.draw (cr, x + lbox.width, y, ...); But maybe two separate bugs should be opened, instead of this ad-hoc solution: 1) implement pango_layout_set_height, as suggested by Behdad 2) use pango_layout_set_height in GtkCalendar
Created attachment 101711 [details] [review] Add GTK_CALENDAR_SHOW_DETAILS display flag (rev. 3) (In reply to comment #51) > (From update of attachment 101683 [details] [review] [edit]) > When I tried the last set of patches, it seemed that tooltips in the > !show-details case didn't work anymore ? > Duh, fixed: mathias@pergamaunz:~/Software/gnome/gtk+$1? interdiff /tmp/y/0007-Add-GTK_CALENDAR_SHOW_DETAILS-display-flag-which-ch.patch /tmp/z/0006-Add-GTK_CALENDAR_SHOW_DETAILS-display-flag-which-ch.patch diff -u b/gtk/gtkcalendar.c b/gtk/gtkcalendar.c --- b/gtk/gtkcalendar.c +++ b/gtk/gtkcalendar.c @@ -1700,7 +1700,8 @@ col = calendar_column_from_x (calendar, x - x0); row = calendar_row_from_y (calendar, y - y0); - if (priv->detail_overflow[row] & (1 << col)) + if (0 != (priv->detail_overflow[row] & (1 << col)) || + 0 == (calendar->display_flags & GTK_CALENDAR_SHOW_DETAILS)) { detail = gtk_calendar_get_detail (calendar, row, col); calendar_day_rectangle (calendar, row, col, &day_rect);
> But maybe two separate bugs should be opened, instead of this ad-hoc solution: > > 1) implement pango_layout_set_height, as suggested by Behdad > 2) use pango_layout_set_height in GtkCalendar Yeah, fine with me. Lets take this to separate bugs. > Would like to apply that patch on top of the others (instead of merging it into > attachment 101700 [details] [review] [edit]) to prevent regressions. And this too.
Comment on attachment 101702 [details] [review] Work in progress: Consider style properties for separator The wide separators seem to work reasonably well, but the color thing doesn't. The separator vanishes for the selected day, since it is drawn in the same color as the selection.
2007-12-28 Mathias Hasselmann <mathias@openismus.com> Add GTK_CALENDAR_SHOW_DETAILS display flag, which chooses if details are shown within the widget, or jst as tooltip. * gtk/gtkcalendar.c, gtk/gtkcalendar.h: Add "show-details" property aka. GTK_CALENDAR_SHOW_DETAILS, and use it. * tests/testcalendar.c: Test GTK_CALENDAR_SHOW_DETAILS. Reduce padding in flags vbox. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> Try more decent appearance of calendar details separator. (#339540) * gtk/gtkcalendar.c: Use different colors for drawing the separator, and make it short by one pixel on each side. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> Apply trivial code-style changes from attachement 101101. (#339540) * tests/testcalendar.c: Some updates to current GTK+ code-style. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> Restructure testcalendar for testing calendar details. (#339540) * tests/testcalendar.c: Push code arround for testing calendar details. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> Implement GtkTooltip API for calendar details. (#339540) * gtk/gtkcalendar.c: Add gtk_calendar_query_tooltip and chain it up. Remember detail overflows in calendar_paint_day to show the tooltip only when neccessary. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> Without setting "detail-width-chars" and "detail-height-rows" properties not only the widget has to be redrawn on certain conditions, but also its size must be recalculated. (#339540) * gtk/gtkcalendar.c: Add calendar_queue_refresh and call that function instead of gtk_widget_queue_draw. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> Consider details for size-request and expose-event. (#339540) * gtk/gtkcalendar.c: Add gtk_calendar_get_detail and is_color_attribute functions. Change gtk_calendar_size_request and calendar_paint_day to consider and show calender details. 2007-12-28 Mathias Hasselmann <mathias@openismus.com> * gtk/tmpl/gtkcalendar.sgml: Move documentation for GtkCalendarDisplayOptions to gtk/gtkcalendar.c.
.oO(The attachement list below really should allow sorting and filtering: Its hard to work with for larger patchsets)
Created attachment 101732 [details] [review] [1/2] Remove max_detail_height field from GtkCalendarPrivate. (#339540) * gtk/gtkcalendar.c: Drop max_detail_height field, and use a local variable instead in gtk_calendar_size_request. --- ChangeLog | 9 ++++++++- gtk/gtkcalendar.c | 13 ++++++------- 2 files changed, 14 insertions(+), 8 deletions(-)
Created attachment 101733 [details] [review] [2/2] Consider "wide-separators" and "separator-height" style properties: The separator is drawn using gtk_paint_box instead of cairo, when "wide-separators" is set. Also do not highlight details for previous or next month, if their day matches the selected day. (#339540) * gtk/gtkcalendar.c: Consider "wide-separators" and "separator-height" in gtk_calendar_size_request and calendar_paint_day * gtk/gtkstyle.c: Modify gtk_default_draw_box to use special colors for selected detail separators in GtkCalendar. --- ChangeLog | 12 ++++++ gtk/gtkcalendar.c | 100 +++++++++++++++++++++++++++++++++++++++------------- gtk/gtkstyle.c | 20 ++++++++--- 3 files changed, 102 insertions(+), 30 deletions(-)
(In reply to comment #58) > (From update of attachment 101702 [details] [review] [edit]) > The wide separators seem to work reasonably well, but the color thing doesn't. > The separator vanishes for the selected day, since it is drawn in the same > color as the selection. > I refered to that problem, when asking "Can gtk_paint_box be told, to use foreground instead of background color of GTK_STATE_SELECTED state, or is that fine-tuning a theme author's task?" in comment #54. Well, split the patch into two pieces: Attachement 101732 for dropping the max_detail_height field and and attachement 101733 for considering style properties. Added some special casing to gtk_default_draw_box() to get proper color for the separator.
Comment on attachment 101732 [details] [review] [1/2] Remove max_detail_height field from GtkCalendarPrivate. (#339540) - are shown within the widget, or jst as tooltip. + are shown within the widget, or jst as tooltip. (#339540) Wierd... you added the bug ref, but left the typo. Otherwise, the patch looks fine.
(In reply to comment #64) > (From update of attachment 101732 [details] [review] [edit]) > - are shown within the widget, or jst as tooltip. > + are shown within the widget, or jst as tooltip. (#339540) > > Wierd... > you added the bug ref, but left the typo. Pattern recognition, spell checking: Different regions of brain. ;-)
The wide separator patch seems to work ok now, but looking at the patch, the color selections are certainly not nice, code-wise. In particular, it looks bad that the wide-separator and !wide-separator code paths use different colors. Can that be right ?
(In reply to comment #66) > The wide separator patch seems to work ok now, but looking at the patch, the > color selections are certainly not nice, code-wise. > In particular, it looks bad that the wide-separator and !wide-separator code > paths use different colors. Can that be right ? > The joys of dealing with a restricted theming engine. Let's analyse which colors I use. The calendar widget uses four states for drawing days: 1) other month 2) current month, but not selected 3) current month and selected, but no focus 4) current month, selected and focus When directly using cairo, I use the most appropriate colors: 1) base[GTK_STATE_INSENSITIVE] 2) base[GTK_STATE_ACTIVE] 3) text[GTK_STATE_ACTIVE] 4) text[GTK_STATE_SELECTED] For state 1 and 2 I use base, instead of text as you wanted a more decent look. GTK_STATE_ACTIVE has to be used in state 2, as base[GTK_STATE_NORMAL] is equal to the day area's background color. In "wide-separator" I am restricted by the capabilities of gtk_paint_box due the specification of the "wide-separator" style property. So without tweaking the default style, I get this colors: 1) bg[GTK_STATE_INSENSITIVE] 2) bg[GTK_STATE_NORMAL] 3) bg[GTK_STATE_SELECTED] 4) bg[GTK_STATE_SELECTED] Since bg[GTK_STATE_SELECTED] also is used for painting the selected day's background, I patched the default style to recognize that situation, which gives the following colors: 1) bg[GTK_STATE_INSENSITIVE] 2) bg[GTK_STATE_NORMAL] 3) text[GTK_STATE_ACTIVE] 4) text[GTK_STATE_SELECTED] I used GTK_STATE_NORMAL for state 2, as this is the state required there: "GTK_STATE_NORMAL - State during normal operation" vs. "GTK_STATE_ACTIVE - State of a currently active widget, such as a depressed button." So what to do next? Should I also handle the states 1 and 2 in gtk_default_draw_box()?
Yeah, hmm. I think the only chance we have for reasonable contrast across themes is to always use the same color that is used for the day number.
The, I could change the code to always call gtk_paint_box(), and change gtk_default_draw_box() to mix colors when drawing the "detail-separator" detail. Something like: (2 * text[state] + base[state]) / 3 This should give reasonable contrast, and a soft look. Just wondering if this would be too much of a hack.
#define SELECTED_FG_COLOR(widget) (&(widget)->style->text[GTK_WIDGET_HAS_FOCUS (widget) ? GTK_STATE_SELECTED : GTK_STATE_ACTIVE]) #define MARKED_COLOR(widget) (&(widget)->style->text[GTK_WIDGET_STATE (widget)]) #define NORMAL_DAY_COLOR(widget) (&(widget)->style->text[GTK_WIDGET_STATE(widget)]) the text drawing uses these, no ? if (calendar->selected_day == day) text_color = SELECTED_FG_COLOR (widget); else if (calendar->marked_date[day-1]) text_color = MARKED_COLOR (widget); else text_color = NORMAL_DAY_COLOR (widget); Can't we just use those same colors for the separator ?
(In reply to comment #70) > (&(widget)->style->text[GTK_WIDGET_STATE(widget)]) > > Can't we just use those same colors for the separator ? Well, 'cause you rejected this color choices in comment #10: > Haven't looked at the patch at all yet, but visually, I have to say that I > think those separators are too dominant. Can we make those lines thinner, > and/or the gap between them wider ? Only why to make 1px lines "thinner" is not giving full tint.
Well, and even when using the macros, gtk_default_draw_box() still has to be patched, if you want consistent colors - as gtk_default_draw_box() uses base colors, not text colors for painting.
Created attachment 101958 [details] [review] Consider "wide-separators" and "separator-height" style properties This variant of the patch always calls gtk_paint_box() in GtkCalendar, and tries to mix reasonable colors in gtk_default_draw_box().
Created attachment 101959 [details] Appearance of last patch with custom theme. Compared to attachement 101103 this looks quite gray. :-(
Created attachment 102834 [details] [review] [PATCH] Change GtkCalendarDetailFunc to return newly allocated string. (#339540) * gtk/gtkcalendar.c: Release the memory returned by the detail_func. * gtk/gtkcalendar.h: Remove G_CONST_RETURN from GtkCalendarDetailFunc. * tests/testcalendar.c: Duplicate calendar details before returning. --- ChangeLog | 8 ++++++++ gtk/gtkcalendar.c | 12 ++++++++---- gtk/gtkcalendar.h | 13 +++++++------ tests/testcalendar.c | 4 ++-- 4 files changed, 25 insertions(+), 12 deletions(-)
Turned out, that GtkCalendarDetailFunc with G_CONST_RETURN is quite inconvenient to use in real application code. Ok to commit that change? Well, and regarding wide-separator support? Any new ideas? Shall I post to gtk-devel-list regarding that topic? Or gnome-themes? Or Planet GNOME?
Alternatively, you could allow to specify a free function together with the detail function. If you support NULL for the free function, you can support both conventions. Or would that be more confusing ?
(In reply to comment #77) > Alternatively, you could allow to specify a free function together with the > detail function. If you support NULL for the free function, you can support > both conventions. > > Or would that be more confusing ? Guess that would be confusing, specially, as gtk_calendar_set_detail_func already has a destroy notifier its data argument. Also this approach would require a const cast when the detail function takes the chance to return static strings. As you might suspect by the initial API design, I absolutely prefer avoiding needless strdup calls, but the only real solution I see for this kind of problems, is adding reference counting to GString, or introducing of a reference counted immutable string type: struct _GImmutableString { volatile guint ref_count; gsize len; gchar str[1]; }; GImmutableString* g_immutable_string_new (const gchar *str, gssize len) { GImmutableString *self; if (-1 == len) len = strlen (len); self = g_new (gchar, sizeof (GImmutableString) + len); self->ref_count = 1; self->len = len; memcpy (self->str, str, len); self->str[len] = '\0'; return self; } Please forgive the clumsy name, but someone decided to call glib's string-buffer GString, instead of GStringBuffer, GStringBuilder, ... ;-)
Sorry, I've not been following very closely. Why do you need to strdup again? Why not return the internal copy and ask it to not be modified, like pango_layout_get_text() does for example? As for string theory, I found this very interesting: http://weblogs.mozillazine.org/roc/archives/2008/01/string_theory.html Many improvements there can be implemented in GString already.
pango_layout_get_text() returns something that is already stored in the PangoLayout. You couldn't return a const if the return text had to be created/calculated each time. This calendar day details callback can be expected to be created dynamically. There will usually be no "internal copy" in the application. There's no reason to assume that people are more likely to want to directly show part of a static array of data. An alternative free callback would work, but I don't see the need for the complexity. It's premature optimization - saving the memory copying isn't worthwhile here.
(In reply to comment #76) > Turned out, that GtkCalendarDetailFunc with G_CONST_RETURN is quite > inconvenient to use in real application code. also in bindings for garbage collected languages: returning a const string is usually a pain to implement.
So, is it OK to commit that change to make it return const gchar*? I'd like to get this bug off my personal TODO list.
You mean to make it _not_ return const gchar*, right ? No big objection from me. Please go ahead.
2008-01-16 Mathias Hasselmann <mathias@openismus.com> Change GtkCalendarDetailFunc to return newly allocated string. (#339540) * gtk/gtkcalendar.c: Release the memory returned by the detail_func. * gtk/gtkcalendar.h: Remove G_CONST_RETURN from GtkCalendarDetailFunc. * tests/testcalendar.c: Duplicate calendar details before returning.
I think we forgot to close this bug.