Re: Dia find & replace?

[Hans Breuer <hans breuer org>]

At 11.10.2008 04:12, Johann Tienhaara wrote:
> Hi Dia folks.
>
> I sent this patch for "find & replace" to Hans a few days ago.  I know he's a very busy guy, 
> but I am a little worried that I may have been spam-filtered!  :)
Not only quite busy but recently also cursed by the worst email provider
imaginable. Still recovering from a five day(!) email outage [1]

> --- On Tue, 10/7/08, Johann Tienhaara <jtienhaara yahoo com> wrote:
> > Hi Hans.
> >
> > I downloaded Dia trunk to add "find &
> > replace" to Dia, only to discover, to my delight, that
> > you had already added the feature quite some time ago! 
> > That'll teach me to stop using my ancient distro-bundled
> > version...  I'm going to live on the bleeding edge from
> > now on.  :)
Welcome at the bleeding edge ;)

> > I made a few changes to find-and-replace.c so that --
> > optionally -- any and all text properties can be found &
> > replaced (rather than just the "name" and
> > "text" properties of an object).
> >
Thanks for the patch, that feature is certainly useful.

> > This gives me the rest of what I was looking for -- the
> > ability to replace text in UML attribute types and operation
> > parameter names and what-not.
> >
> > I would definitely appreciate your feedback.  
First thing is keeping consisteny, at least on the single file level. You
have introduce some new programming style, which make the patch unnecessary
hard to read:
 - extra spaces around the conditions
 - comparing to NULL (a == NULL) rather than the usual not NULL check (!a)
 - superfluous braces for single operation following conditions
 - extra line for the brace before the else

Also it produces just noise in the patch, where there is no change in
behaviour.

> > Especially
> > since some of the data structures had me scratching my head
> > a bit (like the list-of-property-lists "records"
> > inside sarray and darray properties -- why a list-of-lists
> > of Properties? Why not just a list of Properties?).

This on is simple:
For example the UML class has methods (list).
A method is a _record_ with name, return value and parameters (list).
A parameter again consists of a record with a name, a (default-)value, a
type etc.

> > I suspect I've made a few hacks 
> > that should be re-done with cleaner/safer/better solutions.
> >
> > I also am not yet sure whether there's a gauntlet of
> > automated and/or manual tests that you run these sorts of
> > changes through...?
Something like this got added in the current development phase in the
tests/* directory. Patchs to increase the coverage are certainly
appreciated. But at the moment one would need to jump through some extra 
loops to test something from app/

> > I have not yet touched the i18n/l10n stuff either.  There
> > is currently a rather cumbersome message for the "all
> > properties" checkbox.  I'd rather have a
> > terse-yet-still-meaningful message before I figure out how
> > to hack the message catalogs.
No need to hack any message catalogs. By simply marking a translateable 
string with _() your are usually done with internationalization. 
Localization is than done on the message catalog by the translation teams.

> > At the advice of the woefully out of date FAQ :)
> > (http://www.gnome.org/projects/dia/faq.html#MakingPatches) I
> > decided to send the patch in its current state directly to
> > you, since the text is about 10K (and it's pretty hard
> > to read in an email, to boot).
Best place for patches not to be forgotten is bugzilla, but sometimes 
private or preferably public work, too.

Beside the formatting issue mentioned above the patch looked basically ok. 
I've doen the reformatting and some minor changes and just commited it.

The thing missing is a good name for the two new options.

Thanks,
        Hans

[1]
http://www.thomascrampton.com/internet/netidentity-email-outage-19-hours-and-counting/

-------- Hans "at" Breuer "dot" Org -----------
Tell me what you need, and I'll tell you how to
get along without it.                -- Dilbert