Re: Error feedback on graph data selection entries

[Emmanuel Pacaud <emmanuel pacaud univ-poitiers fr>]

Le dimanche 16 décembre 2007 à 10:26 -0500, Jody Goldberg a écrit :
> The current behavior is broken, and this seems like a reasonable
> solution, but the api worries me.  The behaviour should be
> consistent across the various prop dialogs.  We're also too close to
> 1.8.0 to get this properly tested (not your fault).  Please update
> with the comments below, and we can try get it in for 1.8.1.

I don't know if we have other open reproductible crasher, but this issue
is almost one.

> On Sat, Dec 08, 2007 at 08:51:23PM +0100, Emmanuel Pacaud wrote:
> >  gnm_expr_entry_parse (GnmExprEntry *gee, GnmParsePos const *pp,
> >                   GnmParseError *perr, gboolean start_sel,
> > -                 GnmExprParseFlags flags)
> > +                 GnmExprParseFlags flags, gboolean update_entry)
> This is the only piece that gives me pause.
> Why do we need different behaviours for update_entry.

That's where I need your help. I've done the fix in a way it should not
impact other part of gnumeric, by having a parameter that keep the
current behaviour of gnm_expr_entry_parse. I'm a bit puzzled by this
behaviour, and don't understand why a parse function would change the
content of the widget entry. But I didn't remove the entry update
completely, because I don't know how the other part of gnumeric would be
affected by such a change. It seems gnm_expr_entry_parse has always
updated its entry after the parsing. 
 
> > +static void
> > +cb_graph_dim_entry_changed (GnmExprEntry *gee,
> > +                       GraphDimEditor *editor)
> > +{
> > +   if (!GTK_WIDGET_SENSITIVE (gee) || editor->dataset == NULL)
> > +           return;
> > +
> > +   get_texpr (editor, NULL);
> > +}
> > +
> > +static void
> > +cb_graph_dim_editor_update (GnmExprEntry *gee,
> > +                       G_GNUC_UNUSED gboolean user_requested,
> > +                       GraphDimEditor *editor)
> > +{
> > +   GnmExprTop const *texpr;
> > +
> > +   /* Ignore changes while we are insensitive. useful for displaying
> > +    * values, without storing then as Data.  Also ignore updates if the
> > +    * dataset has been cleared via the weakref handler  */
> > +   if (!GTK_WIDGET_SENSITIVE (gee) || editor->dataset == NULL)
> > +           return;
> > +
> > +   if (get_texpr (editor, &texpr)) {
> That repeated chunk seems like it belongs in get_texpr

I'll change that.

> > +static gboolean
> > +get_texpr (GraphDimEditor *editor, GnmExprTop const **new_texpr)
> >  {
> ...
> >             if (texpr == NULL) {
> > +                   if (editor->data_type == GOG_DATA_SCALAR) {
> > +                           if (new_texpr != NULL)
> >                                     texpr = gnm_expr_top_new_constant (
> >                                             value_new_string (gnm_expr_entry_get_text (gee)));
> Looks like something is missing here.  Don't we want to set is_valid
> TRUE if texpr != NULL ?

is_valid is already initialized to TRUE.

        Emmanuel.