[PATCH] sonnet cleanup patch 1

[Jordi]

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.

Konqueror should be using sonnet, but I the menu does not show up.

Other apps should be ported to sonnet and tested.

I'm sure I will discover more :P


> I am really interested in sonnet, I want to make sonnet usable for KDE
> > 4 and add some features for KDE 4.1
>
> Wicked! That's great to hear :)
>
> Some comments and observations about the patch:
>
> > Index: sonnet/plugins/aspell/kspell_aspelldict.cpp
> > ===================================================================
> > --- sonnet/plugins/aspell/kspell_aspelldict.cpp (revision 729074)
> > +++ sonnet/plugins/aspell/kspell_aspelldict.cpp (working copy)
> > @@ -42,6 +42,7 @@
> >      else
> >          m_speller = to_aspell_speller( possible_err );
> >
> > +    name="aspell";
> >  }
>
> Why do you need the name of the speller in the dict itself? It's held in
> the
> Client, so if for some reason you need it in the dict then it'd be better
> to
> simply change the Loader to propagate it to the speller that it created.


The loader will use the clients to create the dict. So the right place
should be the clients code in the plugins themselves. I'll change it in the
next patch.



> Index: kdeui/sonnet/dialog.cpp
> > ===================================================================
> > --- kdeui/sonnet/dialog.cpp     (revision 729263)
> > +++ kdeui/sonnet/dialog.cpp     (working copy)
> > @@ -72,8 +72,8 @@
> >      d->checker = checker;
> >
> >      initGui();
> > -    initConnections();
> > -    setMainWidget( d->wdg );
> > +//    initConnections();
> > +    setMainWidget( d->wdg); //new QLabel("trusmis",this)); //d->wdg );
> >  }
>
> Any reason why you disabled the connections? Without them it won't work.


Sorry, I was testing and left this commented out. Should not be commented



>  void TestDialog::check( const QString& buffer )
> >  {
> > -    Sonnet::Dialog *dlg = new Sonnet::Dialog(
> > +    dlg = new Sonnet::Dialog(
> >          new BackgroundChecker(this), 0);
> >      connect(dlg, SIGNAL(done(const QString&)),
> >              SLOT(doneChecking(const QString&)));
> >      dlg->setBuffer(buffer);
> > -    dlg->show();
> > +    dlg->exec();
> >  }
>
> What's the reason for this change?



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?


> Index: kdecore/sonnet/speller.cpp
> > ===================================================================
> > --- kdecore/sonnet/speller.cpp  (revision 729074)
> > +++ kdecore/sonnet/speller.cpp  (working copy)
> > @@ -49,18 +49,11 @@
> >
> >          dict = loader->createSpeller(language);
> >      }
> > -    bool isValid()
> > -    {
> > -        if (settings->modified()) {
> > -            recreateDict();
> > -            settings->setModified(false);
> > -        }
> > -        return dict;
> > -    }
>
> I don't think the isValid changes really give us anything. Furthermore now
> looking at it, recreating dictionary on changing of the default language
> doesn't make sense (it won't change anything because the language used by
> the
> current speller has already been set). We should likely just stop even
> calling recreateDict() in that case because I don't think that changing of
> the "default language" should ever change the language of the currently
> used
> speller.
> Same probably applies to the Client. In this case though we likely should
> have
> setClient() and client() methods that would act like setLanguage/language
> do.



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!

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.
Yes, setting the client is nice, I'll include it in the next patch.

Now each method check if a dict could be created for this client/language.
To avoid the check for every word, two solutions:
-Create a dummy plugin that just return nothing that will be used if not
dict is available, so we always have a valid dict.
-Return an error to the user and stop spellchecking

comments will be appreciated

> Index: kdecore/sonnet/loader.cpp
> > ===================================================================
> > --- kdecore/sonnet/loader.cpp   (revision 729074)
> > +++ kdecore/sonnet/loader.cpp   (working copy)
> > @@ -74,7 +74,7 @@
> >  {
> >      kDebug()<<"Removing loader : "<< this;
> >      d->plugins.clear();
> > -    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.


z
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20071026/1e38c111/attachment.htm>