Created attachment 143494 [details]
fixed button pixmap for the toolbar

Created attachment 143495 [details]
fixed radiobutton pixmap for the toolbar

I also propose to implement radio buttons in the same fashion, but in this case the value should be an integer. For each radiobutton you would then specify the linked cell and the integer value for the button. The radio button would be checked if the value in the cell matches the value given in the properties dialog.

This is pretty straightforward and doesn't require any kind of explicit grouping. The grouping would be implicit through the linked cell. (All radiobuttons linked to the same cell would belong to the same group.)

If I have the time I'll work this out, but I won't be sad if someone beats me to it. ;-)

Why would you want a second Label item in the toolbar? Note that Frame and Label Items are the same thing!

Sorry, I said frame when I meant rectangle. The rectangle (and oval items) are identical to the label item.

Well, leave it out then. I found it confusing however, that I have to insert a rectange when I want a text label, and the extra button in the object toolbar doesn't hurt IMHO.

And, btw, the ellipse doesn't show any text, even though the contents property is there. The rectangle works, though.

FWIW, I've cleaned up my patch, it's now split up in various, properly git-formatted patches which are based on the latest HEAD.

Unfortunately, bugzilla doesn't let me attach anything to this bug report any more, instead it complains: "The file you are trying to attach is empty, does not exist, or you don't have permission to read it." Well, the files do exist, are not empty, have proper permissions and they're not that big either. Packaging them up as a tarball gives the same error message. What's wrong with bugzilla, can anyone help?

For the time being, if anyone wants to have my latest patches, please drop me an email and I'll mail them to you.

Created attachment 143551 [details]
Revised and split-up button+radiobutton patch.

Ok, the attachment quirks seem to have been fixed, so here goes again. A patch to make radio buttons work, as I proposed earlier, is now included as well. I tested these extensively, and they work fine for me.

For convenience, I packaged these up in a tarball. They are to be applied, in the indicated order, against HEAD (as of Sun Sep 20 21:28:17 UTC 2009, http://git.gnome.org/cgit/gnumeric/commit/?id=18abcb2546f5b584aaf1cbf78e16d51fcdbb0d7d). It would be nice if these could go into 1.9.14.

I forgot to mention that the radiobuttons are actually a bit more general than what I proposed, the radiobutton values (i.e., what you enter into the 'Value' field of the radio button properties) can be either booleans, numbers or strings in the version that I implemented now. A string value looking like a boolean or number can be escaped with a leading single quote in the 'Value' field. As I proposed, grouping is done automatically, based on the linked cells.

Where is so-radiobutton.glade?

We definitely do not want duplicated items in the object toolbar. If it is confusing that one needs to select the rectangle button to include a label, tehn perhaps that button's look should change. In the long erm in fact the label, rectangle and oval should all be one button since they should only differ by formatting.

Created attachment 143649 [details]
Missing so-radiobutton.glade

Sorry about the missing glade file, here it is.

> We definitely do not want duplicated items in the object toolbar.

I understand. Maybe just use a different icon that shows text in a rectangle , that should make it clear.

What is pushing an non-radio button supposed to do (without being able to attach it ti scripts?)

Well, the cell linked to the button goes to TRUE when the button is pushed, and back to FALSE when the button is released again. So you can use it to trigger a function defined in a script (Perl, Python or Pure), if you define the script function to take appropriate action when the linked value changes to TRUE (or FALSE, depending on when you want to execute the action). The action might be to just recompute some value, or modify some other cells (Python and Pure only, Perl doesn't provide the necessary functions for that).

E.g., assume that A1 is the linked cell of the button. Then you might have a formula '=calendar(A1)' in some other cell which computes a calendar and inserts it into cells B3:Y35, say. In Pure that might look as follows (note that in Pure land Gnumeric booleans are marshalled to integers, 1 for TRUE and 0 for FALSE):

calendar 1 = set_range "B3:Y35" (my_fancy_calendar (year today));
calendar 0 = (); // do nothing

my_fancy_calendar would be a Pure function to compute a nicely formatted calendar as a Pure matrix of strings, for instance. In the above example, the action would be executed when pushing the button. If you want it to be executed when releasing the button, you just swap '0' and '1' in the above definition, that's all.

(I've actually programmed this little example in Pure. Let me know if you want to have the script and sample Gnumeric spreadsheet.)

So I'd say that Gnumeric is perfectably scriptable already. ;-) It's just a matter of providing a more comprehensive API in the existing scripting plugins. (The Pure plugin only allows you to modify cell values right now, but in the future I will provide more elaborate facilities.)

Please submit regular patches that can be applied with patch. Your patches seem to assume that we are using git-am. It is unlikely that we in fact commit a patch exactly as provided.

Also, you patches do not contain any Changelog information so they definitely cannot be committed as submitted.

Committed slightly modified versions of the first two patches.

If you should redo your patches please note for example that

<property name="invisible_char" translatable="yes">*</property>

should probably be

<property name="invisible_char">*</property>

since translaters can't properly translate "*", especially if it might appear in several contexts.

Similarly we should not mark the empty string for translation.

> <property name="invisible_char" translatable="yes">*</property>

Well, that stuff is in my glade files because these were based on so-checkbox.glade, so you'll have to fix that one as well. In fact, if you grep through the .glade files in the dialog directory, you'll find many such bogus 'translatable' attributes.

I'll redo the patches in the way you recommended and add ChangeLog entries asap, but this may take a few days as I'm currently busy with other things.

I just tried my GIT-formatted patches and they work fine with patch, you have to apply them in the main source directory, though. E.g.:

cd gnumeric
patch -p1 < 0003-Add-button-widget-properties.patch

I get no rejects here, not even any fuziness warnings from patch. So I don't see what the problem is. If you can make the patches work for you then it'll be less work for me, I'll just have to provide you with the ChangeLog entries, will do that later today. Ok?

I had done the equivalent of patch 1 by hand and run into difficulties with patch 2. I never tried patches 3 and onwards (I assumed that the difficulties of patch 2 would continue.)

I just tried patch 0003... and got only hunk #5 failing so if you could do the Changelog(s) entries that would be great!

The best way to apply git-produced patches is with "git apply foo.patch".

Right now I don't see any "Properties" menu entry for buttons.  Is that
expected?

Morten: The properties part is in patches 3 and onwards. I am still reviewing those.

Patches 3 and 4 are in. So only the radiobutton patch is still pending.

Tonight is a bit too late, I spent the entire day adding GtkGLExt support to the Pure plugin so that it can render OpenGL in a frame widget. I'll prepare the ChangeLog entries for the button patches tomorrow, sorry for the delay.

The only ChangeLog entries still needed are for patch 0005.

After applying patch 5 (and hopefully not messing anything up since it did not apply cleanly and neededhuman intervention), I have some problems:

I create two radio buttons.
I configure the first one to link to A2 and set its value to 1
I configure the second one to link to A2 and set its value to 2

Now if I click on a button the value changes correctly to 1 or 2 resp.
But if I type a 3 into A2 I would expect both radio buttons to become deselected. They do not!

I also saw  a bunch of 
** (gnumeric:5031): CRITICAL **: gnm_expr_top_ref: assertion `IS_GNM_EXPR_TOP (texpr)' failed
but can't replicate them.

okay, I get a bunch of those criticals as I create a radio button.

Created attachment 143876 [details]
Suggested ChangeLog entries for the radiobuttons patch

> Now if I click on a button the value changes correctly to 1 or 2 resp.
> But if I type a 3 into A2 I would expect both radio buttons to become
> deselected. They do not!

Well, that's just how radio buttons work; exactly one button in a group *must* always be activated. Consequently, you can't explicitly deactivate a radio button, you can only activate it (in which case all the other buttons in the same group are deactivated automatically). So there's not much I can do about that. One might pop up an error dialog in such a situation, but that would be very annoying.

(NB: Actually, there *is* one horrible kludge to deactivate all buttons in a group, which I found by accident: If you move the active button into a new group, then the remaining buttons of its old group all remain inactive. At least that's the case with the Gtk version I'm running here. But I really consider this a Gtk bug. In fact, in the sheet_widget_radio_button_set_group callback you can see that I work around this misbehaviour by first selecting another button in the same group before moving the active button to a new group.)

Created attachment 143882 [details] [review]
assertion `IS_GNM_EXPR_TOP (texpr)' failed

> okay, I get a bunch of those criticals as I create a radio button.

Fix attached.

Andreas, I just noticed another minor glitch: In the radiobuttons dialog, it tries to do a selection on both entries. Only the first of these should be selected. The second call to gtk_editable_select_region should thus be removed. Here's the patch; it would be nice if you could just add this when committing the radiobutton stuff:

diff --git a/src/sheet-object-widget.c b/src/sheet-object-widget.c
index ea72551..7fc4c34 100644
--- a/src/sheet-object-widget.c
+++ b/src/sheet-object-widget.c
@@ -2941,7 +2941,6 @@ sheet_widget_radio_button_user_config (SheetObject *so, SheetControl *sc)
 	gtk_editable_select_region (GTK_EDITABLE(state->label), 0, -1);
 	state->value = glade_xml_get_widget (state->gui, "value_entry");
 	gtk_entry_set_text (GTK_ENTRY (state->value), swrb->value);
-	gtk_editable_select_region (GTK_EDITABLE(state->value), 0, -1);
 	gnumeric_editable_enters (GTK_WINDOW (state->dialog),
 				  GTK_WIDGET (state->expression));
 	gnumeric_editable_enters (GTK_WINDOW (state->dialog),

On Sheet1 create 2 radio buttons, link one to $A$1 with value 1 and the other to Sheet1!$A$1 with value 2. Now both are linked to the same cell but neither works.

Yes, the cell reference texprs need to be normalized somehow, before looking them up in the group hash table. Not sure what's the best way to do that yet, will have a closer look tomorrow.

Clicking an unconfigured radiobutton that is not active yields:

(gnumeric:10246): Gtk-CRITICAL **: gtk_button_clicked: assertion `GTK_IS_BUTTON (button)' failed

I also see on exiting:

Leaking expression at 0x9280e74: $A$1.

** (gnumeric:10252): WARNING **: Leaked 1 nodes from expression pool for big nodes.

Are you sure that you are releasing all expressions?

Created attachment 143966 [details] [review]
Fix for radio buttons on different sheets.

Fixed the reported issues with radio button links to other sheets.

This probably won't apply cleanly to HEAD as in the meantime I've also done some other changes to my copy of sheet-object-widget.c so the line numbers are a bit out of sync. But basically it's just updated versions of the dep_hash and dep_equal functions, so it should be easy to apply this manually. For convenience, here's the new ChangeLog entry as plain text:

2009-09-25  Albert Graef  <Dr.Graef@t-online.de>

	* src/sheet-object-widget.c (dep_hash, dep_equal): Use explicit
	sheet references when hashing and comparing dependents.


And the modified versions of dep_hash and dep_equal:

#include "expr-impl.h"

/* Keep track of GnmDepenmdent -> radio button group mapping. We have to go to
   some lengths here because the cell reference expressions in the dependents
   are not forced to have explicit sheet references. Thus, when hashing or
   comparing the dependents, we check for cell references with a NULL sheet in
   which case we assume the sheet of the dependent instead. */

static GHashTable *groups = NULL;

static guint
dep_cellref_hash(GnmDependent const *k)
{
	const GnmCellRef *ref = &k->texpr->expr->cellref.ref;
	GnmCellRef r;
	if (!ref->sheet) {
		r = *ref;
		r.sheet = k->sheet;
		ref = &r;
	}
	return gnm_cellref_hash(ref);
}

static guint
dep_hash(GnmDependent const *k)
{
	if (!k || !k->texpr)
		return 0;
	else if (GNM_EXPR_GET_OPER (k->texpr->expr) == GNM_EXPR_OP_CELLREF)
		return dep_cellref_hash(k);
	else
		/* shouldn't happen */
		return gnm_expr_top_hash(k->texpr);
}

static gint
dep_cellref_equal(GnmDependent const *k1, GnmDependent const *k2)
{
	const GnmCellRef *ref1 = &k1->texpr->expr->cellref.ref;
	const GnmCellRef *ref2 = &k2->texpr->expr->cellref.ref;
	GnmCellRef r1, r2;
	if (!ref1->sheet) {
		r1 = *ref1;
		r1.sheet = k1->sheet;
		ref1 = &r1;
	}
	if (!ref2->sheet) {
		r2 = *ref2;
		r2.sheet = k2->sheet;
		ref2 = &r2;
	}
	return gnm_cellref_equal(ref1, ref2);
}

static gint
dep_equal(GnmDependent const *k1, GnmDependent const *k2)
{
	if (k1 && k2)
		if (!k1->texpr || !k2->texpr)
			return k1->texpr == k2->texpr;
		else if (GNM_EXPR_GET_OPER (k1->texpr->expr) == GNM_EXPR_OP_CELLREF &&
			 GNM_EXPR_GET_OPER (k2->texpr->expr) == GNM_EXPR_OP_CELLREF)
			return dep_cellref_equal(k1, k2);
		else
			/* shouldn't happen */
			return gnm_expr_top_equal(k1->texpr, k2->texpr);
	else
		return k1==k2;
}

> Are you sure that you are releasing all expressions?

Well, my code does keep track of all radiobutton group changes and updates the hash table accordingly. But there seem to be some corner cases where some hash table entries remain at program exit, and thus the texpr references held by these entries are not released. I'm not sure why that happens. One might forcibly destroy the dependent->group hash table at exit time to cure this if it's a real problem, but I don't know how to do that. (Is there a way I can register a "clean up my sh*t at exit" routine with Gnumeric?)

> Clicking an unconfigured radiobutton that is not active yields:

> (gnumeric:10246): Gtk-CRITICAL **: gtk_button_clicked: assertion `GTK_IS_BUTTON
> (button)' failed

I cannot reproduce this here. Can you please provide a test case?

Created attachment 143969 [details] [review]
Fix for radio buttons on different sheets, revised.

Ok, here is the same patch for the dependent hashing functions again, now based on my previous copy of sheet-object-widget.c, so it should apply cleanly on top of https://bugzilla.gnome.org/attachment.cgi?id=143882 ("assertion `IS_GNM_EXPR_TOP (texpr)' failed", see comment #34).

We don't really want to "cleanup my..." at exit but making sure that we never leave any behind when radio buttons are destroyed or reconfigured.

> We don't really want to "cleanup my..." at exit but making sure that we never
> leave any behind when radio buttons are destroyed or reconfigured.

In an ideal world, yes. But this is hard if not impossible to do in the current implementation, as the dependent keys in the hash table are shared by all the buttons in the same group, so unfortunately I can't just deal with this in a local fashion in the finalize callback. Anything else that I can think of would require much more bookkeeping and be quite a bit of additional work.

So I'd rather take care of the common cases where leaks occur, where this is possible in a local fashion, and clean up the rest when the last radiobutton is finalized. That's not perfect, but IMHO a workable solution, and you won't get any annoying warnings about leaked expression nodes any more. Does that sound like a deal?

I have already implemented this, will post a patch in due course.

Created attachment 144048 [details] [review]
Plug leaks on expression nodes.

Plug leaks of radio buttons on expression nodes, as described in previous comment.

patches comitted

Thanks a lot. Andreas, I just noticed there are still two issues with the object toolbar:

- The checkbox tool is in the wrong position. I think that it should be moved between the new button and radiobutton tools.

- You changed the icon of the rectangle tool, as I recommended, but after seeing it now I think that the icon looks odd there among the other graphic tools. I think you should change it back to the rectangle icon after all.

Regarding the rectangle tool icon we just really need a better icon: it has to show a proper rectangle that happens to contain text.