Review Request: port Sonnet to QSettings

[Kevin Krammer]

On Thursday, 2012-12-27, Martin Tobias Holmedahl Sandsmark wrote:
> On Thu, Dec 27, 2012 at 12:18:21PM -0000, Kevin Krammer wrote:
> > Two levels are most likley the most common case because most systems do
> > not have any system administrator. Once you have admin customization you
> > have at least three (package, admin, user). I don't have any evidence
> > for nor against usage of Kiosk features in regard to spell checking, I
> > am just pointing out that the solution proposed here does have none of
> > those.
> 
> Well, the library in question here does spell checking, so that's what we
> should focus on. IMHO spell checking doesn't need any of the extra features
> offered by KConfig.

Indeed, it should be doing any form of config IO. It can't know whether any of 
its options need to be persisted, or are supplied hardcoded by the using code 
or offered to the user but volatile or offered the users and stored 
persistently or confgured by the system administrator or configured by the 
system vendor.

Due to the old code we can deduce that the option values were potentially 
stored persistently, allowing user, administrator and system vendor to 
provide, override and lock-down each of them.

But as you said yourself: it does spell checking, it does not have to care 
about settings persistence at all.

> > The reuse of the filename "sonnetrc" suggest to me that the intention is
> > to use the same file. A KConfig based application and a QSettings based
> > application will behave inconsitently if using the same file. Or is this
> > a per-application stored file?
> 
> Do I use sonnetrc anywhere? In that case I missed something.

You are right, sonnet does not.
Somehow the change request seems to have picked up an unrelated change in 
kdeui/widgets/ktextedit.cpp:71

> > Do we assume that KDE application developers who's programs are being
> > used in an "Enterprise" setup will fork the library and reimplement the
> > config with KConfig or do we want them to use the KF5 version? The later
> > will either require that the library does not handle config itself or at
> > least allows applications to intercept config access or provide config
> > that takes precendence over stored config.
> 
> If they care about that they can just load using whatever config system
> they want, and set the loaded options manually.

My point exactly!

> I also don't think this is a very likely scenario, tbh.

No idea, maybe they all just hard-code values. The old code's usage of KConfig 
and the config access in ktextedit.cpp suggests that some applications wanted 
to settings to stay configurable.

In either case nothing the spell checker needs to care about. It uses the 
values, it has no interest what so ever in how it got them in the first place.

> > IMHO the most sensible case is to let the application handle config I/O,
> > allowing it to store the config in a way that is most approriate for its
> > usage. If that is a KConfig INI file, a simple QSetting INI file or a
> > JSON file shouldn't really matter to an engine which only is interested
> > in the values.
> 
> I think the most sensible is to not let the application developers bother
> their pretty little heads with storing the config at all if they don't want
> to.

Of course, I had assumed that all settings had hard-coded default values.

I was referring to application developers who want their apps spell checking 
capabilities configurable. Those developers already bothered their pretty 
little heads with config storing and loading, again removing the need for the 
spell checker to bother.

> With what I have proposed here they can reimplement the config storage if
> they want to, but by default It Just Works™.

My problem with understanding this is: why does the spell checker need the 
capability to secretly access persistent config?
Removing its internal config access will still have it "Just Works" by default 
(again assuming that those variables are at least initialized properly).

Lets look at Sonnet::Speller (judging by its export macro a public class).
It has two methods, save and restore, that have no indication what so ever on 
where they would save to or restore from. Hyperspace probably.

It does not, however, have any way to retrieve the Sonnet::Settings object on 
which's content it supposeldy operates.

Or when I look at Sonnet::ConfigWidget, supposedly at widget providing 
consistent user interface for Sonnet options. Again some ominous save() method 
without any way to tell it where and how to save and again not way to retrieve 
the Sonnet::Settings object this is supposedly working on.

So I looked at Sonnet::Loader which seemed to be referred to a lot regarding 
settings. Finally, a way to access the settings! Weird thing is that it is 
declared in a header with _p.h suffix, usually indicating that this is not 
public API. Even weirder, the same seems to be true for Sonnet::Settings!

The more I read, the less it makes sense. Settings that cannot be read or 
written.  Mysterious layer violations in KTextEdit. Strange indeed.

Cheers,
Kevin
-- 
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121227/5e5f548d/attachment.sig>