Created attachment 84781 [details] [review]
Path to add gtk_calendar_set_date function

Here is a patch to current svn to add gtk_clendat_set_date function (http://live.gnome.org/GtkLove)

Created attachment 84783 [details] [review]
The previous patch was not correct. This one will compile

Fixed patch

Could you rework your patch ? Look at what will happen if values < -1 are passed to the function...

Created attachment 85010 [details] [review]
New patch. Fixed problem with value lesser than -1

Oops...

Should be good now.

Created attachment 85011 [details] [review]
fixed g_return_if_fail

This should be good this time

For me this enhancement is really too far from complete to know how to implement it as is...

Some corner cases:
* How should we handle an invalid date ? (2007/02/31 for example ?). We may return a boolean to tell if setting the date succeded, because the date may be invalid...
* The calendar should emit a signal to update appropriately the day, month and year as gtk_calendar_select_month and gtk_calendar_set_date do, to update the control to the new date.

Guillaume, for your last patch, you're still checking for the bad ranges:
+  g_return_if_fail ((month < 12) || (month < -1));
should be
+  g_return_if_fail ((month >= -1) && (month <= 12));
Please check g_return_if_fail documentation, you patch has more error of this kind.

Moreover, could someone comment on this, so that Guillaume knows what he exactly needs to implement ? Could someone of gtk knowledge also comment to tell if this enhancement is really justified, or is this bug just something we want to get rid of in bugzilla ? I'm especially talking to the people who set the http://live.gnome.org/GtkLove page.

Thanks.

Well, you need to do a lot more than setting the new values for the date: notify the property, invalidate the area, emit the signal...
As setting a new month/year already invalidates the whole widget it probably makes sense to write a new function that does the following:

* check the input
* set the new month and year
* calendar_compute_days
* set the day
* invalidate the widget
* notify the properties (between a couple of freeze/thaw notify)
* emit month_changed and day_selected signals

Check the code of both gtk_calendar_select_month and gtk_calendar_select_day to see how to do everything you need.

I'm not a GTK+ maintainer and only added the bug to my list because it's easy enough for a newbie to fix. Whatever is the GTK+ maintainers opinion about this feature is beyond my control, but I agree it should be resolved as WONTFIX if they think it's not needed.

Also it's not a bad idea to CC people if you want to get comments from them ;)

Thanks for your interest Guillaume!

Created attachment 85270 [details] [review]
Fixed patch

Ok, here is a improved version of the patch.

* The date is checked using g_date_valid_dmy.
* Signal are emitted.
* Property are setted.
* Widget is mark for redraw.

I would like to request a GDate interface to this widget too. (Get/Set)

Guillaume, using the range [0,11] for months is innintuitive. See the values defined in the enum GDateMonth in Glib.

I also think you can greatly simplify your code, replacing at the beginning the problematic "-1" value, and checking directly after that the date we want to set is valid. Here's a simple example for the month parameter, just extend it for day and year, it should be easy.
 
if (month == -1)
  month = calendar->month;

[...]

g_return_val_if_fail (g_date_valid_dmy(day, month, year), FALSE);

if (month != calendar->month)
{
  /* update and notify */
}

If the 0 - 11 range is replaced, probably every program using it needs to be rewritten, yes? It would be nice to have a today function in the calendar too (like its Win32 counterpart).

Stephan: Other enhancements should be on a separate bug. For now, we'd just like to have this one closed. When Guillaume will have a nice patch ready, I'm sure the GTK team will be happy to apply it :-).

Appart from this, I'm pretty sure the [1-12] range for months, and the [1-31] range for days will never change ;-p

Guillaume ? Have you some spare time to hack on this again ?

Created attachment 86259 [details] [review]
Updated patch

Updated patch.

* I have clean the code a bit. 
* Month are now in range 1-12.

Great, that's much better :-)

It seems I had overlooked the fact that GtkCalendar uses in its internal implementation 0-based values (which doesn't sound very logical for me, as it's not consistent with the GDateMonth type in Glib).

I'm really sorry about this, I mislead you. Stefan was right in comment 12, but I didn't understand what he meant. As gtk_calendar_get_date returns months in the [0;11] range, we have to do the same, even if I think it's plain dumb (days use the [1;31] range, why did they not follow this logic for the months ?). But otherwise, one wouldn't be able to call gtk_calendar_set_date with values retrieved from gtk_calendar_get_date.

Just a few problems to solve and we're there:

1. The documentation header:
replace "if you don't to change" by either "if you don't change" or "if you don't want to change" (you copy/pasted the error on the 3 input parameters)

2. I would have prefered to send the signals only if the value has *really* changed, for performance reasons. Calling stuff when not necessary is always a bad habit.

3. g_return_val_if_fail (GTK_IS_CALENDAR (calendar), FALSE) should be the first line of the function. It's a good habit to check the object before doing anything else.

For point 2 and your notification code, I'm not sure, so if a GTK developer passes by, or someone with GTK skills, please review the rest of the patch.

I think I'm going to open a bug against GtkCalendar, because it duplicates things implemented in glib, but hasn't the same month range, which is counter intuitive.

Oh, there's also a typo in the return value documentation:
"if an error happen" -> should be "happened"

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

See my other bug report. Probably the GtkCalendar thing needs to get into GDate style calls. Then make the other calls deprecated.

The problem is that the GtkCalendar elements are readable.
The GtkCalendar documentation specifies the following:

"month, year, and selected_day contain the currently visible month, year, and selected day respectively.
All of these fields should be considered read only, and everything in this struct should only be modified using the functions provided below.
Note:
Note that month is zero-based (i.e it allowed values are 0-11) while selected_day is one-based (i.e. allowed values are 1-31)."

That means we can't change the internal representation of the "month" attribute, without affecting people reading that value. Even worse: if we do so, their program has no way to know it. It will compile, but the values used will just be plain wrong. So this is a dead end. Maybe we need to do another calendar widget, using 90% of the current calendar, but which uses the behavior that glib has when dealing with dates.

Maybe adding a new month field to the structure and deprecate the old one would do the trick, but then we would break the ABI...

Hum, in fact there's a lot of unused fields in the GtkCalendar struct. So it would be possible to keep a double representation (two month fields, one 0-base, the other 1-based) using one of these. There's even a private data structure if we don't want to expose it. I just wonder why so many attributes are kept public ?

once a member of a struct is public, it must be kept that way (unless you go on and add an #ifdef G_DISABLE_DEPRECATED ... #endif around it, which would force people to compile deprecated stuff just to access that struct member).

GtkCalendar was written when GObject didn't have support for the private field, and it also didn't really change between GTK+ 1.x and 2.x.

I'd say that if we want to fix the calendar widget, either we wait for GTK+ 3.0, and avoid deprecation altogether, or someone writes a gtk-new-calendar widget with a sane API, using GLib's date and time API and possibly implementing the RFEs of the current GtkCalendar widget (style properties, better signals, etc.).

Created attachment 86445 [details] [review]
fixed documentation and month

New patch.

* Fixed error in the documentation
* Month in range 0-11

I haven't changed anything for signal. Not sure if I can set field while the the widget is freeze.

So, I agree that it doesn't make much sense to invest a lot of time to "properly" fix GtkCalendar at this point, but now that Guillaume has written a patch to implement the missing API I guess it won't hurt to add it. Could any maintainer give his opinion on this?

Something new about this ? I really feel bad when we ask people to come and help us, and finally patches end rotting...

I like this API addition! While you are at it, what about

void set_gdate(GDate* date) and
GDate* get_gdate(GDate* date)?

It is very ugly to convert between the day, month, year to GDate because of the zero and one based month.

I think it would be overkill to completely rewrite the GtkCalendar widget just to add these simple things though it would be a good thing to deprecate the public fields.

Of course some other API comes into my mind like adding multiple markers with different color to days...

sorry for taking this long to review the patches.

+gboolean
+gtk_calendar_set_date (GtkCalendar *calendar, 
+		       gint	    year,
+		       gint         month,
+		       gint	    day)

this function accepts the date in a slightly different format that gtk_calendar_get_date() which is supposed to mirror. this is broken on various levels. if we were to add a gtk_calendar_set_date() it should be possible to roundtrip between what gtk_calendar_get_date() returns and this function accepts. I understand the need for a sane API, as opposed to the current one, but it must not be an excuse for subtly breaking stuff.

then: why using the split format instead of using GDate? GDate already supports conversion and overflow, so GtkCalendar should have API to set and get a GDate, and gtk_calendar_get_date() should probably be deprecated:

  gboolean gtk_calendar_set_gdate (GtkCalendar *calendar, GDate *gdate);
  void     gtk_calendar_get_gdate (GtkCalendar *calendar, GDate *gdate);

+  g_signal_emit(calendar,
+		gtk_calendar_signals[DAY_SELECTED_SIGNAL],
+		0);
+  g_signal_emit(calendar,
+		gtk_calendar_signals[MONTH_CHANGED_SIGNAL],
+		0);

these signals should not be emitted in case the day and month did not change.

Created attachment 88966 [details] [review]
Updated patch

I'have changed the gtk_calendar_set_gdate function to use a GDate. Signals are now only emitted, if value has changed. I also add the gtk_calendar_get_gdate

(In reply to comment #27)

> I'have changed the gtk_calendar_set_gdate function to use a GDate. Signals are
> now only emitted, if value has changed. I also add the gtk_calendar_get_gdate

thanks, looks better now.

some minor nitpicks:

 /**
+ * gtk_calendar_set_gdate:
+ * @calendar: a #GtkCalendar
+ * @date: the date to set
+ * Return value: %TRUE if the date was set, %FALSE if an error happened.
+ * Set the selected date to a #GtkCalendar.
+ **/

you should leave a line between the arguments and the function description, the return value should be below the description, and you should add "Since: 2.12".

+  oldDay = calendar->selected_day;
+  oldMonth = calendar->month;
+  calendar->month = g_date_get_month(date) - 1;
+  calendar->year = g_date_get_year(date);

no camel case variables, and you should leave a space between the function name and the parenthesis.

+/**
+ * gtk_calendar_get_gdate:
+ * @calendar: a #GtkCalendar
+ * @date: location to store the date
+ * Obtains the selected date from a #GtkCalendar.
+ **/

same as above: add a Since line and leave a newline between the arguments and the description.

+  g_return_if_fail(GTK_IS_CALENDAR(calendar));
+  
+  if (!date)
+    date = g_date_new();

I'd add a "g_return_if_fail (date != NULL);" and not create a GDate ourselves.

Created attachment 89025 [details] [review]
Updated patch

As earlier commenters, I am not too fond of trying to fix GtkCalendar 
at this stage. And I don't think these additions are worth it unless
we do it properly...

- Why do we add a getter/setter pair but not a property ? That would
  of course require GDate to be registered as a boxed type.

- The setter should return void, not boolean. FALSE is only returned
  by the return_if_fail checks anyway.

- To follow established patterns, I would expect the getter to be
  GDate *gtk_calendar_get_date (GtkCalendar *calendar). That would
  require GtkCalendar to have a GDate representation for the selected
  date internally, which it can return here.

- Some leftover stylistic issues, like missing spaces between
  function name and (

(In reply to comment #30)

> - To follow established patterns, I would expect the getter to be
>   GDate *gtk_calendar_get_date (GtkCalendar *calendar).

what about using a GDate objects on the stack? the system log viewer uses GDate a lot and even though some are allocated, sometimes the GDate is just used within the scope of a function.

(In reply to comment #21)
> once a member of a struct is public, it must be kept that way (unless you go on
> and add an #ifdef G_DISABLE_DEPRECATED ... #endif around it, which would force
> people to compile deprecated stuff just to access that struct member).
> 
> GtkCalendar was written when GObject didn't have support for the private field,
> and it also didn't really change between GTK+ 1.x and 2.x.
> 
> I'd say that if we want to fix the calendar widget, either we wait for GTK+
> 3.0, and avoid deprecation altogether, or someone writes a gtk-new-calendar
> widget with a sane API, using GLib's date and time API and possibly
> implementing the RFEs of the current GtkCalendar widget (style properties,
> better signals, etc.).

Is there plans to rework GtkCalendar for GNOME 3.0 ? We should at least hide the implementation so it can evolve on the GNOME 3.0 era...

if someone is interesed to work on this, Maybe It's a good idea to use the new time API if It lands in time, see bug #50076.

we never got this done. nowadays it should use GDateTime, but that is a bit more complicated, because GDateTime is not stack-allocated and immutable. Not worth it after all these years.