[PATCH] sonnet cleanup patch 1

[Zack Rusin]

On Thursday 25 October 2007 10:35:11 pm Jordi wrote:
> On 10/26/07, Zack Rusin <zack at kde.org> wrote:
> > On Thursday 25 October 2007 12:51:51 pm Jordi wrote:
> > > Today 12:51:51 pm
> > >
> > > Some cleanup of sonnet.
> > > Much more to be done before KDE 4, right now it is failing for me in
> > > some ways :/
> >
> > Can you let me know what ways?
>
> The code as the current HEAD can not run test_dialog
> That's why  I was playing with it and left the connections commented out
> (didn't mean to go to the patch)
> The dialog didn't appear if I call it with show();
> Calling exec () will appear but crash due a problem with libenchant.

It works for me. Can you send me the backtrace privately?

> Konqueror should be using sonnet, but I the menu does not show up.
>
> Other apps should be ported to sonnet and tested.

Those are not problems with Sonnet itself but with applications. I absolutely 
agree that we need to sit down and get all of them working though :)

> The loader will use the clients to create the dict. So the right place
> should be the clients code in the plugins themselves.

Well, no not really. This way every Client has to do:
dict->setName(name());
whereas the loader could have just one:
dict->setName(client->name()); 
So instead of duplicating the code in all the client plugins you could just 
put in the loader.

> The difference should be that exec makes the dialog modal by default, while
> show() will not IIRC.
> See the  comment above about problems found.
> Making the dialog modal will be an usability problem?

Nah, it's a testing application, usability there isn't really a big concern. 
It's just that it's effectively a no-op change so I was wondering whether 
there were any technical reasons for it. If there's not lets just keep it the 
way it was :)

> See that I delete the isValid() method. Previously the dict was recreated
> with every change and that includes adding a new word to the dictionary!

No, that's not the way it worked. The way you seem to want to do it is this:
speller.setConfigX();
//dictionary recreated
speller.setConfigY();
//dictionary recreated
speller.setConfigZ();
//dictionary recreated
dictionary.isMisspelled();// dictionary recreated three times

The way I wanted it to work is:
speller.setConfigX();
speller.setConfigY();
speller.setConfigZ();
dictionary.isMisspelled();//check if the config changed, it did - recreate it

I think the current code semantics are correct. We don't want to recreate the 
dictionary when it's being set, it's a pretty expensive operation - we want 
to wait until the user is done setting it and recreate it when we need it.

Also it's the settings object that should make sure that it's modified flag 
isn't set when language/client has been to its current value. Actually it 
does it for language so the code that you added to it in the speller is 
duplicating what's in the settings object.

> You are right, any change to the current spelling language should be made
> by setLanguage so recreateDict() is not needed in setDefaultLanguage().  I
> guess that can be needed if you are spellcheking and in the middle of that
> you go to settings (old Kcontrol) and change the default spelling language.

Nah, I did that in KSpell2 and I don't think it was a good idea. Changing 
default global spelling language doesn't necessary mean the email I'm 
currently composing is in the default language that just changed - that 
setting is application specific.

> Now each method check if a dict could be created for this client/language.
> To avoid the check for every word, two solutions:

That's ok "if (bool)" is hardly going to be a bottleneck :) I think we should 
leave it, at least for now.

> > > -    delete d->settings; d->settings = 0;
> > > +    delete d->settings;
> > >      delete d;
> > >  }
> >
> > Any particular reason for this change?
>
> we are deleting d, so we are deleting the pointer to settings, we don't
> need to set it to 0.

That's not really the reason for it :) Personally I like it as a general 
guideline for projects with multiple people working on it. Now if someone is 
going to add cleanup() method that doesn't require a d pointer and hence put 
it after deleting the pointer and another person is going to come and add 
some code manipulating d pointer in the cleanup() method there they'll get an 
immediate crash. Things like that tend to get rather useful in big projects 
so lets leave it.

z