Created attachment 6569 [details] [review]
address-entry respin

Created attachment 6570 [details] [review]
fix to previous

CVS around 20020131, apply snake-oil.patch then snake2

Any problems found with these ones?
I must say it plays much nicer with the selection (does not reset the
user's selection)

snake2 uses alias_start_pos == 0 and alias_end_pos = 0 to test when an
alias-expansion is not in progress. They are reset only on keystrokes,
not mouse button clicks. There may be a better test, or alternatively
alias_start_pos and alias_end_pos could be reset in some other event
handlers. I'll test out some ideas.

WorksForMe(tm) but didn't actually look at it carefuly

Try this:
 - start typing an address and get a tentative expansion;
 - without accepting it, click on part of the expansion text, and
select a character or two;
 - the whole string is highlighted, because
libbalsa_address_entry_draw still sees an alias and replaces the
selection limits with the alias limits;
 - hit `Del' to delete the selected text, and of course only the
really selected text goes, not the highlighted text.

This is definitely an `unbehave', which can be fixed by clearing
alias_start_pos and alias_end_pos in
libbalsa_address_entry_button_press, but a deeper issue is what Balsa
should do in this case:
 - treat the mouse button click as accepting the alias;
 - abandon the attempt to match, but leave the expansion text,
parenthesized, in the entry;
 - treat the click as rejecting the match and clear the expansion
text.

The current code takes the second route, but it too has a problem with
the whole expansion being highlighted briefly.

IMO we come closest to the current strategy if the test is based on
editable->selection_start_pos != editable->selection_end_pos. I'll
give that a try and post again.

OK: editable->has_selection seems to be the best test, but it has to
be reset when we see an ordinary character key-press.

One other problem: at some point, the cursor seems to have gone away,
so I had to cut'n'paste a few lines from gtkentry.c--ugh!

Incremental patch, snake-3.patch, follows. Incidentally, snake-oil is
`patch -p0', whereas snake-2 and snake-3 are `patch -p1'.

Created attachment 6579 [details] [review]
snake-3.patch: apply after snake-oil and snake-2

Playing around selecting address text, I noticed the following:

1. Address_entry sometimes loses touch with what's in the entry. If
you  select some text and delete it, the code looks for the addresses
that contain the start and end of the selected text. Two errors can
arise: it doesn't find such an address; it finds one but computes a
negative relative cursor position. I've seen both errors occur, in
both the original code and with the snake-oil patches, but I can't
give precise details to reproduce them.

2. Possibly related: 
 - go to an empty address_entry;
 - type ",x" (or any character that doesn't trigger a match);
 - entry contains ", x";
 - select it all;
 - type "x";
 - see the funny characters! ", xÅ"@(Å"@" in my case, but ymmv!

Yes I have seen a few bugs in the original code (nothing related to
Peter's Patch). I'll send a patch for them but I'm waiting first for
Peter's final patch to be more maintainer-friendly.
I hope my fixes will actually fix all the issues you mentionned.
Manu

One more comment: The cursor (the vertical bar) does not disappear on
focus-out. Can anybody reproduce it?

Yes, you're right--the first focus-out causes problems, doesn't it?

Perhaps I should try to figure out why the cursor isn't drawn in the
gtkentry code, instead of fudging one in the address_entry code!

1. I commited it as-is in order to make further hacking easier (all
patches above).
2. I have noticed there was a removed focus-out handler from
sendmsg-window.c.

Re 2: The original code had a focus_out_event method that had been
commented out because of gtk+ error messages, and attached a signal
handler to each instance instead. Snake-oil restores the method and
cuts out attaching the handler, and nobody has reported the errors
that bds saw. Presumably the gtk bug that caused them has been fixed
in the
interim.

OK... But then, I still wonder why the cursor does not get hidden on
focus-out.

Apparently, because libbalsa_address_entry_draw is called on focus-out
events, in which case it should hand over immediately to the parent
method instead of doing its highlighting trickery. The following
one-liner seems to fix it.

I still don't understand why the highlighting is a different color, 
and why we need to have our own cursor code. 

Created attachment 6595 [details] [review]
incremental patch

6595 is in. Thanks!

OK Pawel, could I begin to send patch in now that Peter's patch seems
more mature. I identified 2 or 3 bugs plus all code glitches I yet
noticed?
Manu

I finally posted a first patch to the list against current CVS. It
cleans up the code. Next one will also correct a few actual bugs.
Manu

The following patch, against cvs of Feb 7 with Manu's patch, makes
these changes in libbalsa/address-entry.c:
 - remove an unused variable in libbalsa_address_entry_class_init;
 - replaces GTK_WIDGET_CLASS(parent_class)->draw_focus with the cleaner
   gtk_widget_draw_focus in libbalsa_address_entry_draw;
 - fixes the typo in libbalsa_address_entry_button_press that loses
   pasted text;
 - tidies up a g_list_insert in libbalsa_keystroke_comma.

Created attachment 6632 [details] [review]
patch -p0 against current cvs

Oops--here's another small one that I forgot in the previous patch.
Replaces 2 calls to gtk_editable code with one to gtk_entry code. 

Minor, but it cuts out a lot of display changes: in the `To:' entry,
clearing the entry triggers a style change to the `bad address' style,
and then repopulating it reverses the change, so there are two style
changes for a single keystroke. With this change, they're both eliminated.

Created attachment 6633 [details] [review]
patch -p0 against current cvs

I have impression that pasting an address in does not work now. Can
this be confirmed?

Created attachment 6650 [details] [review]
patch -p0 against cvs--fixes pasting (I hope!)

Another round of changes to address-entry.c follows. The biggest
change is eliminating the inputData structure, and moving its active
member to the parent LibBalsaAddressEntry structure. That reduces the
level of indirection in all references to the emailData (addy) structures.

There's also a convenience function libbalsa_delete_link, which
combines g_list_remove_link and g_list_free_1.

Finally, some awkward code constructions are simplified.

Created attachment 6666 [details] [review]
patch (-p0 in top-level balsa directory) against current cvs

More address-entry-related changes. In current code, sendmsg-window
casts LibBalsaAddressEntry objects to GtkEntry or GtkEditable; it's
not clear that those will always be valid casts. This patch removes
all such casts (afaik), replacing them with calls to the
LibBalsaAddressEntry API. Thus all casts are internal to
libbalsa-address-entry.c, where they'll be easier to catch.

It also moves the definition of the LibBalsaAddressEntry structure to
libbalsa-address-entry.c, so that the members are private, leaving
just the typedef in libbalsa-address-entry.h.

Files:

libbalsa/libbalsa-address-entry.[hc]:
 - move definition of the LibBalsaAddressEntry structure;
 - add some wrappers to the API;
 - minor adjustment to cursor placement after delete-word.
src/sendmsg-window.c:
 - replace casts of LibBalsaAddressEntry pointers with wrappers.

Created attachment 6738 [details] [review]
patch (-p0) against current cvs (after Manu's patch)

Commited. FIXMEs have been addressed. Thanks!

balsa-1.3.5 and 2.0.0 released, closing this report for good