Sonnet: don't impose the default client
ClosedPublic

Authored by rjvbb on Mar 9 2018, 3:56 PM.

Details

Summary

Sonnet has a hidden default client concept (hidden because the standard configuration dialog doesn't expose the setting).

In the current implementation this client doesn't behave as one would expect from a default, but rather as an imposed choice. As a result, spell checking is disabled in practice in languages not supported by defaultClient, even if other backends do offer support.

This change relaxes the implementation in Loader::createSpeller(). When called with an empty clientName argument if will now check if the defaultClient supports the requested language. This test will also catch cases where the configuration file (Sonnet.ini) sets defaultClient to an inexistent client (e.g. one that is no longer available).

With this change the term "default" is more appropriate for the resulting behaviour: defaultClient will be used unless it doesn't offer the required language support. If that is the case, Sonnet will fall back to the most reliable available client that does support the language in question.

Test Plan

Works as expected on Mac and Linux, with the shipped examples and with Kate.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.Mar 9 2018, 3:56 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 9 2018, 3:56 PM
rjvbb requested review of this revision.Mar 9 2018, 3:56 PM
rjvbb updated this revision to Diff 29244.Mar 11 2018, 2:50 PM

Add missing context (patch unchanged)

rjvbb set the repository for this revision to R246 Sonnet.
rjvbb edited subscribers, added: kde-frameworks-devel; removed: Frameworks.
rjvbb added a comment.Mar 24 2018, 2:53 PM

Any objections if I commit this by the end of the week?

dfaure requested changes to this revision.Mar 24 2018, 7:22 PM

I like the idea.

src/core/loader.cpp
103

a std::find_if would do the same in much fewer lines

This revision now requires changes to proceed.Mar 24 2018, 7:22 PM
rjvbb updated this revision to Diff 30617.Mar 26 2018, 11:32 AM

Modified as requested.
(Please double-check, it works AFAICT but it's the first time I use this construct.)

rjvbb set the repository for this revision to R246 Sonnet.Mar 26 2018, 11:33 AM
rjvbb marked an inline comment as done.
mwolff added a subscriber: mwolff.Mar 26 2018, 12:16 PM
mwolff added inline comments.
src/core/loader.cpp
103

nope, that's wrong. find_if returns an iterator - in this case a Client* probably that gets converted to bool?

cullmann added inline comments.Mar 26 2018, 12:17 PM
src/core/loader.cpp
103

Isn't there a == lClients.constEnd() behind that does that?

mwolff added inline comments.Mar 26 2018, 12:32 PM
src/core/loader.cpp
103

ah, then this should be simplified by using std::any_of instead

ah, then this should be simplified by using std::any_of instead

I was asked to use std::find_if, and (spicey detail) referred to the KDevelop code for examples how to use it :)

dfaure requested changes to this revision.Mar 26 2018, 7:06 PM

I suggested find_if because I didn't think about any_of, which is even better indeed :-)
If I suggest something and Milian suggests something else then Milian is right :-)

src/core/loader.cpp
114

can this ever happen now that you set it if it was empty? If it can happen (because defaultClient() can return empty) then the any_of block needs an if (!backend.isEmpty()) around it.

This revision now requires changes to proceed.Mar 26 2018, 7:06 PM
rjvbb marked 4 inline comments as done.Mar 26 2018, 7:56 PM

If I suggest something and Milian suggests something else then Milian is right :-)

Unconditionally? :)

src/core/loader.cpp
114

Yes, defaultClient() just returns whatever is in Sonnet.ini, which can be an empty string.
We just do nothing if that's the case (instead of doing a useless any_of followed by a qWarning as happens right now)?

Note that the extra if does have to be inside the initial if (backend.isEmpty()), and not after it.

rjvbb updated this revision to Diff 30657.Mar 26 2018, 7:58 PM
rjvbb marked an inline comment as done.

next iteration

rjvbb set the repository for this revision to R246 Sonnet.Mar 26 2018, 7:59 PM
rjvbb updated this revision to Diff 31594.Apr 7 2018, 3:15 PM

Unnecessary rebump, pardon, rebase.

What's holding this up?

rjvbb set the repository for this revision to R246 Sonnet.Apr 7 2018, 3:16 PM
dfaure accepted this revision.Apr 7 2018, 5:21 PM
This revision is now accepted and ready to land.Apr 7 2018, 5:21 PM
This revision was automatically updated to reflect the committed changes.