From kde-core-devel  Wed Jan 03 14:40:53 2007
From: Simon Hausmann <hausmann () kde ! org>
Date: Wed, 03 Jan 2007 14:40:53 +0000
To: kde-core-devel
Subject: Re: The future of Sonnet
Message-Id: <200701031540.57741.hausmann () kde ! org>
X-MARC-Message: https://marc.info/?l=kde-core-devel&m=116783527223561
MIME-Version: 1
Content-Type: multipart/mixed; boundary="--nextPart1467471.gFuo7ClK4v"

--nextPart1467471.gFuo7ClK4v
Content-Type: text/plain;
  charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

On Tuesday 02 January 2007 05:13, Jacob R Rideout wrote:
> > Are the classes inside a namespace? Names like KAbstractFilter and
> > GuessLanguage are a bit too generic otherwise and the latter might
> > clash with symbols from something else.
>
> I've been considering this myself. KSpell2 used a namespace for all
> its classes. Most other classes in KDE, however, seem just to use
> distinct names or a prefix in the name. KOffice prefixes its class
> with ko. I'd prefer not to use a namespace, but I've yet decide on
> naming scheme and have thus solicited outside advice. I think
> prefixing internal (although still accessible) classes with Sonnet and
> those meant to be used by application developers with
> KUniqueAndDescriptiveName might be best.
>
> > Are the API docs online somewhere?
>
> All development right now is taking place in branches/work so they
> aren't auto-generated. I just created a few for the classes I've
> documented so far.
>
> http://www.jacobrideout.net/sonnet/classKAbstractFilter.html
> http://www.jacobrideout.net/sonnet/classKTextBreaks.html
> http://www.jacobrideout.net/sonnet/classKUnicodeData.html

I just had a look at it and the implementation and I suggest not to make th=
ose=20
classes public. As pointed out previously their names are generic. But in=20
addition I suggest a default policy for new code in libraries to make=20
everthing private by default and export/publicize API once it it is useful=
=20
for applications.

Applications using a spell checker are unlikely to be interested in TR32=20
specific character categories. And by having it in the public API it means=
=20
you can't change it anymore. You can't even move it into a different librar=
y,=20
because that is not binary compatible on Windows.

I suggest to make only SonnetSpeller public API, after another review. And=
=20
maybe a few helper functions for bringing up and controlling a the standard=
=20
spell checker dialog. But I wouldn't make the actual dialog class public.=20
Remember, if it's public it's very hard to replace with another dialog.

On the TR23 implementation: I think string lookups for every character to g=
et=20
the tr23 category are going to be dreadfully slow. And performance is=20
certainly important for a spell checker :).

I'm specifically think of this code:

       // start rule based checking

        //WB5
        if ( ( catagory =3D=3D data->categoryCode("ALetter") )
                &&
                ( catagory2 =3D=3D data->categoryCode("ALetter") )
           )
        {
            //kDebug(750) << "WB5";
            bk=3Dfalse;
        }

        [ ... more of these ... ]

An enum would be so much faster instead and keep the code still very readab=
le.

Even though I suggest private API for the AbstractFilter class here's one=20
suggestion for the Type enum:

If you look at an enum in the class header it is often easily understandabl=
e=20
what the individual values mean. But if you look at the caller code it is=20
suddenly not apparent anymore. For example:

	filter->setType(AbstractFilter::Sentence);

If you look at this piece of code half a year later I promise you'll have n=
o=20
idea anymore what exactly this means. It would be much more readable with a=
=20
longer name, including a verb. In Qt we very often repeat the name of the=20
enum in the individual values to solve this very problem. Here's an=20
alternative suggestion:

enum FilterMode {
    FilterSentences,
    FilterWords,
    FilterGraphemes
};

Or perhaps filtering is the wrong word here since in a way you don't filter=
=20
but you itemize the text. The current API suggests that at least.

Hope this helps a bit :)

Simon

--nextPart1467471.gFuo7ClK4v
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQBFm8B5WXvMThJCpvIRAg90AJ40i44DHmXof8TSdQi0uemhmI4sugCfa+Tp
yMeZkF/0RtWyFGeeL9PEp/o=
=1hou
-----END PGP SIGNATURE-----

--nextPart1467471.gFuo7ClK4v--