D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Ahmad Samir]

ahmadsamir added a comment.


  In D18317#396483 , @loh.tar wrote:
  
  > Well, I'm not a Sonnet Guru, more a normal user.
  
  
  Me too; your views are appreciated all the same.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[loh tar]

loh.tar added a comment.


  Well, I'm not a Sonnet Guru, more a normal user. Sorry if it sounds so. I 
can't give a detailed point how to solve some particular issue.
  
  My comment is more about what I would expect or what I would try to achieve.
  
  I didn't investigate from where is Loader::createSpeller called and what 
happens after a nullptr is returned. Perhaps can there add some error handling 
to hint the user.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Ahmad Samir]

ahmadsamir added a comment.


  In D18317#395928 , @loh.tar wrote:
  
  > Just my thoughts:
  >
  > - I think there shouldn't be the (default) dictionary changed by some smart 
logic. Just hint the user that the setting is not applicable.
  
  
  The user will open the sonnet config dialog and see that the dictionary he 
wants is already selected (e.g. the dictionary files have just been renamed, 
but it's the same dictionary); that is confusing, as can be gleaned from the 
linked bug report.
  
  > - To set the dict to the system locale seems to me the less smartest trick. 
If everybody want such "auto-fix" should then the bad setting investigated and 
tried to find some similar setting, e.g. Bad "de_AT_ost" -> "de_AT" -> "de_DE". 
Why? Someone may have a locale of "de_DE" but a dict setting "en_US" for 
whatever reason. Besides I guess Loader::createSpeller is not only called when 
the default dict will loaded. IIRC has Sonnet some functionallity to guess a 
language and choose a fitting dict (May that help?)
  
  It's what sonnet settings default to initially:
  https://cgit.kde.org/sonnet.git/tree/src/core/settings.cpp#n291:
  
d->defaultLanguage = settings.value(QStringLiteral("defaultLanguage"),

QLocale::system().name()).toString();
  
  Language auto detection seems to cause some havoc: 
https://bugs.kde.org/show_bug.cgi?id=386611
  
  > - The Config-GUI should show some hint in case of trouble, 
src/ui/configui.ui
  
  [..]
  
  > - Don't overwrite permanently some bad setting with a new value. Perhaps 
has the user just set up a new system and only missed to install some package
  
  Same problem as above, the user opens the config dialog and the dictionary he 
wants is already selected
  
  [...]

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Ahmad Samir]

ahmadsamir added a comment.


  @loh.tar: I'll think that over, thanks for the pointers :)

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[loh tar]

loh.tar added a comment.


  Just my thoughts:
  
  - I think there shouldn't be the (default) dictionary changed by some smart 
logic. Just hint the user that the setting is not applicable.
  - To set the dict to the system locale seems to me the less smartest trick. 
If everybody want such "auto-fix" should then the bad setting investigated and 
tried to find some similar setting, e.g. Bad "de_AT_ost" -> "de_AT" -> "de_DE". 
Why? Someone may have a locale of "de_DE" but a dict setting "en_US" for 
whatever reason. Besides I guess Loader::createSpeller is not only called when 
the default dict will loaded. IIRC has Sonnet some functionallity to guess a 
language and choose a fitting dict (May that help?)
  - The Config-GUI should show some hint in case of trouble, src/ui/configui.ui
  - Don't overwrite permanently some bad setting with a new value. Perhaps has 
the user just set up a new system and only missed to install some package
  - Perhaps should the error message saved in a QString(List), retrievable 
later so it can be shown e.g by KTextEditor::Message
  - Perhaps should Loader::createSpeller return some "Error-Helper-Dictionary" 
instead of a nullptr, so that Sonnet::defaultLanguage may give something like 
"ERROR-de_AT_ost" instead of e.g "de_AT_ost".
  - Accordingly should Sonnet::preferredDictionaries filled with data like 
"ERROR-de_AT_ost"/"ERROR-Deutsch (Östereich something)"
  - See also Sonnet::DictionaryComboBox. There should then the error hints are 
be visible
  - Take a look at D18125 , why I think 
that may helpful. In the new button should then the hint possible to be shown 
"ERROR-foo".
  
  Not investigated if all of these is needed or possible.
  
  PS: Typo in SUMMARY "..of the the dictionary.." 2x the

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Ahmad Samir]

ahmadsamir added a comment.


  In D18317#395530 , @pino wrote:
  
  > In D18317#395513 , @pino wrote:
  >
  > > This makes a "core" library grow a dependency on widgets -- not really a 
good idea, considering there is the sonnetui library for that.
  >
  >
  > In addition to the above, there are also really bad consquences: take a 
console-only application running QCoreApplication as application class and 
using sonnetcore. Now, in case of the situation described, sonnetcore will try 
spawn a QMessageBox, which IIRC exits the application, as no GUI is available.
  
  
  OK. I didn't know the core/ui differentiation, I'll adjust the patch and see 
if I can come up with a better solution without having core lib depend on 
widgets.
  
  Thanks for the pointers.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Pino Toscano]

pino added a comment.


  In D18317#395513 , @pino wrote:
  
  > This makes a "core" library grow a dependency on widgets -- not really a 
good idea, considering there is the sonnetui library for that.
  
  
  In addition to the above, there are also really bad consquences: take a 
console-only application running QCoreApplication as application class and 
using sonnetcore. Now, in case of the situation described, sonnetcore will try 
spawn a QMessageBox, which IIRC exits the application, as no GUI is available.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Pino Toscano]

pino added a comment.


  This makes a "core" library grow a dependency on widgets -- not really a good 
idea, considering there is the sonnetui library for that.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Nathaniel Graham]

ngraham added a reviewer: loh.tar.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D18317

To: ahmadsamir, sandsmark, loh.tar
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18317: Don't fail if defaultLanguage dictionary can't be loaded

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added a reviewer: sandsmark.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  If the files of the the dictionary set in defaultLanguage= can't be
  loaded, instead of failing silently, try to load the locale language
  dictionary, and display a message informing the user of what's been done.
  
  BUG: 325541
  FIXED-IN: 5.55.0

TEST PLAN
  - Edit ~/.config/KDE/Sonnet.conf (or wherever your distro puts the conf file 
under the home dir, and set defaultLanguage= to something invalid/unavailable, 
say en_AB
  - Start e.g. kate and try to spell check something; note that doesn't work 
and this error log is spewed to the terminal: "sonnet.core: No language 
dictionaries for the language: "de_AT_ost""
  - Apply the diff and try again

REPOSITORY
  R246 Sonnet

BRANCH
  invalid-defaultLanguage

REVISION DETAIL
  https://phabricator.kde.org/D18317

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/loader.cpp
  src/core/sonnet-core.pro

To: ahmadsamir, sandsmark
Cc: kde-frameworks-devel, michaelh, ngraham, bruns