Re: Review Request 124147: Create Data Dir If it Does Not Exist
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124147/ --- (Updated July 7, 2015, 8:29 a.m.) Review request for KDE Frameworks. Changes --- Adding qWarnings and renaming the misnamed (now static) function. Repository: kio Description --- Prevents a cold start bug when the data dir is not created. Also, by storing the file name as a member of the KCookieServer class we avoid calculating it every time cookies are saved. Diffs (updated) - src/ioslaves/http/kcookiejar/kcookieserver.h f61c7d0e4da58b805565632cf3dd484445c21275 src/ioslaves/http/kcookiejar/kcookieserver.cpp ac585a0b04637c485647564d18a89a75d6c11d97 Diff: https://git.reviewboard.kde.org/r/124147/diff/ Testing --- Restarted kded5 with no file named kcookiejar inside ~/.local/share and with a regular file named kcookiejar in that location. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124147: Create Data Dir If it Does Not Exist
On July 2, 2015, 9:47 a.m., David Faure wrote: src/ioslaves/http/kcookiejar/kcookieserver.cpp, line 68 https://git.reviewboard.kde.org/r/124147/diff/2/?file=380949#file380949line68 This would be worth a qWarning too. And then I would still return a non-null string, so the rest of the code can (try to) keep going. No point in giving up because of a permission problem; I would warn and keep going with the filename, to give a chance to the user to fix the permission problem and not have to reboot. David Narváez wrote: The whole reason why I started working on this is because the cookie server was continuing without letting me know about the fact that it could not create this folder. Is there a way to notify the user with more than a qWarning() (after all this is running from kded5) like a popup dialog, notification or something? If this crashes there will at least be a drkonqi dialog to notify the user. Yeah, you can show a messagebox. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124147/#review81981 --- On July 7, 2015, 8:29 a.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124147/ --- (Updated July 7, 2015, 8:29 a.m.) Review request for KDE Frameworks. Repository: kio Description --- Prevents a cold start bug when the data dir is not created. Also, by storing the file name as a member of the KCookieServer class we avoid calculating it every time cookies are saved. Diffs - src/ioslaves/http/kcookiejar/kcookieserver.h f61c7d0e4da58b805565632cf3dd484445c21275 src/ioslaves/http/kcookiejar/kcookieserver.cpp ac585a0b04637c485647564d18a89a75d6c11d97 Diff: https://git.reviewboard.kde.org/r/124147/diff/ Testing --- Restarted kded5 with no file named kcookiejar inside ~/.local/share and with a regular file named kcookiejar in that location. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124147: Create Data Dir If it Does Not Exist
On July 2, 2015, 9:47 a.m., David Faure wrote: src/ioslaves/http/kcookiejar/kcookieserver.cpp, line 68 https://git.reviewboard.kde.org/r/124147/diff/2/?file=380949#file380949line68 This would be worth a qWarning too. And then I would still return a non-null string, so the rest of the code can (try to) keep going. No point in giving up because of a permission problem; I would warn and keep going with the filename, to give a chance to the user to fix the permission problem and not have to reboot. The whole reason why I started working on this is because the cookie server was continuing without letting me know about the fact that it could not create this folder. Is there a way to notify the user with more than a qWarning() (after all this is running from kded5) like a popup dialog, notification or something? If this crashes there will at least be a drkonqi dialog to notify the user. On July 2, 2015, 9:47 a.m., David Faure wrote: src/ioslaves/http/kcookiejar/kcookieserver.cpp, line 113 https://git.reviewboard.kde.org/r/124147/diff/2/?file=380949#file380949line113 So how about changing the static function to actually return the filename? Then it can also ensure it's not null. See above. On July 2, 2015, 9:47 a.m., David Faure wrote: src/ioslaves/http/kcookiejar/kcookieserver.cpp, line 118 https://git.reviewboard.kde.org/r/124147/diff/2/?file=380949#file380949line118 ROTFL, ok, this code could be removed by now (kfm was KDE1 - and that file was surely not where QStandardPaths is looking) :-) Will make a separate patch for this. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124147/#review81981 --- On June 22, 2015, 12:27 a.m., David Narváez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124147/ --- (Updated June 22, 2015, 12:27 a.m.) Review request for KDE Frameworks. Repository: kio Description --- Prevents a cold start bug when the data dir is not created. Also, by storing the file name as a member of the KCookieServer class we avoid calculating it every time cookies are saved. Diffs - src/ioslaves/http/kcookiejar/kcookieserver.h f61c7d0e4da58b805565632cf3dd484445c21275 src/ioslaves/http/kcookiejar/kcookieserver.cpp ac585a0b04637c485647564d18a89a75d6c11d97 Diff: https://git.reviewboard.kde.org/r/124147/diff/ Testing --- Restarted kded5 with no file named kcookiejar inside ~/.local/share and with a regular file named kcookiejar in that location. Thanks, David Narváez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 --- src/knotificationmanager.cpp (line 87) https://git.reviewboard.kde.org/r/124281/#comment56593 Don't you have a memory leak here? You have a list of pointers which you own here, but QList doesn't know that. When that list goes out of scope it doesn't delete the pointers as far as i know. The simplest way i can think of is to make plugins a class member and call qDeleteAll(m_plugins) in the class destructor which then deletes all objects. Or you could go for smart pointers which you store in the QList, that would also clean it up nicely when going out of scope. I'm looking at the KNotifications dependency graph here [1] and see that KWindowSystem is only required for KCrash. So err, can't that one go as well since you removed KService which required KCrash which then required KWindowSystem? I could be very wrong if KNotifications is using KWindowSystem somewhere, obviously. But the graph doesn't give me that impression. [1] http://api.kde.org/frameworks-api/frameworks5-apidocs/knotifications/html/knotifications-dependencies.html - Mark Gaiser On jul 7, 2015, 7 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated jul 7, 2015, 7 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/knotificationplugin.cpp 7315c17 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/#review82192
---
src/plugins/voikko/voikkoclient.cpp (line 28)
https://git.reviewboard.kde.org/r/124282/#comment56578
here you can just use the Q_FUNC_INFO macro to get the same information
(the function signature).
src/plugins/voikko/voikkoclient.cpp (line 30)
https://git.reviewboard.kde.org/r/124282/#comment56579
plural is dictionaries
src/plugins/voikko/voikkoclient.cpp (line 32)
https://git.reviewboard.kde.org/r/124282/#comment56580
I prefer to error out early («if (!dictionaries) return;»), less
indentation and state to remember.
src/plugins/voikko/voikkodict.h (line 64)
https://git.reviewboard.kde.org/r/124282/#comment56585
prefix all private members with m_
src/plugins/voikko/voikkodict.cpp (line 47)
https://git.reviewboard.kde.org/r/124282/#comment56581
ALL_UPPERCASE usually means defines (because they have slightly different
behavior from other variables).
src/plugins/voikko/voikkodict.cpp (line 97)
https://git.reviewboard.kde.org/r/124282/#comment56582
always use braces {}
src/plugins/voikko/voikkodict.cpp (line 108)
https://git.reviewboard.kde.org/r/124282/#comment56584
just return true here, and you don't need the else block.
src/plugins/voikko/voikkodict.cpp (line 125)
https://git.reviewboard.kde.org/r/124282/#comment56586
if (replacements.contains(word)) {
suggestions.append(word);
}
is much more readable
src/plugins/voikko/voikkodict.cpp (line 130)
https://git.reviewboard.kde.org/r/124282/#comment56588
can't you free the string again here? then you can do an early return if
wcharSuggestions is empty.
(I'd also prefer avoiding hungarian notiation -- typeVariablename.)
src/plugins/voikko/voikkodict.cpp (line 140)
https://git.reviewboard.kde.org/r/124282/#comment56587
isn't this the wrong delete? It is declared as a pointer, not an array.
src/plugins/voikko/voikkodict.cpp (line 172)
https://git.reviewboard.kde.org/r/124282/#comment56590
This is not very elegant.
A better solution might be to split this up into two functions
(fetchPersonal() that returns languageNode, and storePersonal() that takes a
languageNode()), maybe?
src/plugins/voikko/voikkodict.cpp (line 180)
https://git.reviewboard.kde.org/r/124282/#comment56589
error out early instead.
also, QIODevice::Truncate? That will delete everything already in the file.
src/plugins/voikko/voikkodict.cpp (line 236)
https://git.reviewboard.kde.org/r/124282/#comment56591
error out early
- Martin Tobias Holmedahl Sandsmark
On July 7, 2015, 2:44 p.m., Jesse Jaara wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated July 7, 2015, 2:44 p.m.)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 7, 2015, 5:10 p.m., Alex Richardson wrote: src/knotificationmanager.cpp, line 89 https://git.reviewboard.kde.org/r/124281/diff/2/?file=383580#file383580line89 Use `KPluginLoader::findPlugins` or `KPluginLoader::instantiatePlugins` and then do the qobject_cast to avoid that duplicate code Fwiw, the KPluginLoader docu says this: Therefore, if you do not need the plugin version feature, you can (and should) just use QPluginLoader instead. Looks like it could be updated. On July 7, 2015, 5:10 p.m., Alex Richardson wrote: src/notifybypopup.cpp, line 278 https://git.reviewboard.kde.org/r/124281/diff/2/?file=383581#file383581line278 nullptr? or Q_NULLPTR if that's not allowed. Would make it explicit that it's a pointer. Yeah, good point. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82179 --- On July 7, 2015, 4:42 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 4:42 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 9 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Changes --- KPluginLoader + Q_NULLPTR Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs (updated) - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/knotificationplugin.cpp 7315c17 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
On July 7, 2015, 7:07 p.m., Martin Tobias Holmedahl Sandsmark wrote:
src/plugins/voikko/voikkodict.cpp, line 125
https://git.reviewboard.kde.org/r/124282/diff/3/?file=383592#file383592line125
if (replacements.contains(word)) {
suggestions.append(word);
}
is much more readable
I didn't see apol's comment above, but considering that this isn't something
that's going to be called often and be performance criticial I much prefer your
original code (much more readable).
Even if my example was wrong (the new version was hard to grok). :-)
- Martin Tobias Holmedahl
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/#review82192
---
On July 7, 2015, 2:44 p.m., Jesse Jaara wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated July 7, 2015, 2:44 p.m.)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 7, 2015, 9:38 p.m., Mark Gaiser wrote: src/knotificationmanager.cpp, line 89 https://git.reviewboard.kde.org/r/124281/diff/3/?file=383605#file383605line89 Don't you have a memory leak here? You have a list of pointers which you own here, but QList doesn't know that. When that list goes out of scope it doesn't delete the pointers as far as i know. The simplest way i can think of is to make plugins a class member and call qDeleteAll(m_plugins) in the class destructor which then deletes all objects. Or you could go for smart pointers which you store in the QList, that would also clean it up nicely when going out of scope. Don't you have a memory leak here? No; the plugins wanted are deleted when KNotificationManager is deleted, the plugins not wanted are deleted on line 101. The QObject *s are owned by KNotificationManager, that's what the last arg to KPluginLoader::instantiatePlugins does. On July 7, 2015, 9:38 p.m., Martin Klapetek wrote: I'm looking at the KNotifications dependency graph here [1] and see that KWindowSystem is only required for KCrash. So err, can't that one go as well since you removed KService which required KCrash which then required KWindowSystem? I could be very wrong if KNotifications is using KWindowSystem somewhere, obviously. But the graph doesn't give me that impression. [1] http://api.kde.org/frameworks-api/frameworks5-apidocs/knotifications/html/knotifications-dependencies.html No; KWS is used much more than that, do a grep in the repo. Basically it's used for window activation/raising and related stuff, not _only_ because of KCrash. In fact, KCrash is only a dependency of KService and not used at all in KNotifications, KService is gone but KWindowSystem is still very much needed. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 --- On July 7, 2015, 9 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 9 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/knotificationplugin.cpp 7315c17 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82179 --- Looks good to me otherwise src/knotificationmanager.cpp (line 87) https://git.reviewboard.kde.org/r/124281/#comment56570 Use `KPluginLoader::findPlugins` or `KPluginLoader::instantiatePlugins` and then do the qobject_cast to avoid that duplicate code src/notifybypopup.cpp (line 274) https://git.reviewboard.kde.org/r/124281/#comment56571 nullptr? or Q_NULLPTR if that's not allowed. Would make it explicit that it's a pointer. - Alex Richardson On July 7, 2015, 3:42 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 3:42 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
On July 7, 2015, 9:07 p.m., Martin Tobias Holmedahl Sandsmark wrote:
src/plugins/voikko/voikkoclient.cpp, line 32
https://git.reviewboard.kde.org/r/124282/diff/3/?file=383588#file383588line32
I prefer to error out early («if (!dictionaries) return;»), less
indentation and state to remember.
That's not the policy for KDE Frameworks though:
https://techbase.kde.org/Policies/Kdelibs_Coding_Style
See that running astyle is part of the process of becoming a framework:
https://community.kde.org/Frameworks/CreationGuidelines
- Aleix
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/#review82192
---
On July 7, 2015, 4:44 p.m., Jesse Jaara wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated July 7, 2015, 4:44 p.m.)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124220: kwindowsystem: Add a plugin infrastructure for platform specific implementations
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124220/#review82168 --- Seems kwindowsystem master doesn't build now (against Qt 5.5 at least): ``` [ 55s] In file included from /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp:21:0: [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo_p.h:72:11: error: 'QScopedPointer' does not name a type [ 55s] const QScopedPointerPrivate d; [ 55s]^ [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp: In constructor 'KWindowInfoPrivate::KWindowInfoPrivate(WId, NET::Properties, NET::Properties2)': [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp:55:7: error: class 'KWindowInfoPrivate' does not have any field named 'd' [ 55s] : d(new Private(window, properties, properties2)) [ 55s]^ [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp: In member function 'WId KWindowInfoPrivate::win() const': [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp:65:12: error: 'd' was not declared in this scope [ 55s] return d-window; [ 55s] ^ ``` - Hrvoje Senjan On Srp. 7, 2015, 1:31 popodne, Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124220/ --- (Updated Srp. 7, 2015, 1:31 popodne) Review request for KDE Frameworks. Repository: kwindowsystem Description --- A plugin interface is added which allows a platform specific plugin to implement an interface. If the plugin does not implement the interface, it will fall back to the default implementation. Also if no plugin can be resolved it will fall back to the default implementation. This replaces the existing compile time and runtime selection. In order to make this work the KWindowInfoPrivate is changed from a templated approach to using pure virtuals just like the other private implementations in this library. As the platform specific parts are no longer compiled in we cannot just delegate the KWindowSystem::icon with NETWinInfo overload to the xcb implementation. In order to solve this problem the required method is added to the private interface with a default implementation which does not return anything. If we are not on platform xcb and KWindowSystem is compiled with X11 support the plugin for xcb is loaded and the call gets delegated to the xcb implementation. This allows e.g. KWin to still read icons for Xwayland clients. Diffs - src/CMakeLists.txt ff2ce392ecd7969eb94543528c7a670ea0fcd870 src/config-kwindowsystem.h.cmake fa0eec115870be27a17ec7b398e40f0c7506f11b src/kwindoweffects.cpp fd88e20e1728506f135bcd5ecda3c05754839717 src/kwindoweffects_dummy.cpp 3e24cecb5c7d25883c179b622abdb5ab06587c33 src/kwindoweffects_dummy_p.h PRE-CREATION src/kwindoweffects_p.h 7c740da952f279a2c5fe689daa5a06c131fa9c9d src/kwindowinfo.cpp f29828581cdaecb7613c3f62cff72aa1fc33c266 src/kwindowinfo_dummy_p.h PRE-CREATION src/kwindowinfo_p.h 6727dd1715a13e5bd7793275620c5fa682318f1c src/kwindowsystem.cpp 789132e1b4883dd54218d29af9710dedfe6218e1 src/kwindowsystem_dummy_p.h PRE-CREATION src/kwindowsystem_p.h 0b5f3e8aeb7b70234c61c59979abd840f349b154 src/kwindowsystemplugininterface.cpp PRE-CREATION src/kwindowsystemplugininterface_p.h PRE-CREATION src/platforms/wayland/CMakeLists.txt PRE-CREATION src/platforms/wayland/plugin.h PRE-CREATION src/platforms/wayland/plugin.cpp PRE-CREATION src/platforms/wayland/wayland.json PRE-CREATION src/platforms/xcb/CMakeLists.txt PRE-CREATION src/platforms/xcb/kwindoweffects.cpp src/platforms/xcb/kwindoweffects_x11.h PRE-CREATION src/platforms/xcb/kwindowinfo.cpp src/platforms/xcb/kwindowinfo_p_x11.h src/platforms/xcb/kwindowsystem.cpp src/platforms/xcb/kwindowsystem_p_x11.h src/platforms/xcb/plugin.h PRE-CREATION src/platforms/xcb/plugin.cpp PRE-CREATION src/platforms/xcb/xcb.json PRE-CREATION src/pluginwrapper.cpp PRE-CREATION src/pluginwrapper_p.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/124220/diff/ Testing --- * unit tests still pass (X11) * kwin_wayland still shows icons for Xwayland clients Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org
Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124212: kwindowsystem: Change source file layout
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124212/ --- (Updated July 7, 2015, 11:31 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 21dd5094204c4b467a5c7f8114e1249b4d42f6bc by Martin Gräßlin to branch master. Repository: kwindowsystem Description --- In the src directory we introduce a platforms directory with a specific * osx * wayland * windows * xcb sub directory. The platform specific source and header files are moved into those directories. This is a preparation step to move the platform specific behavior into plugins which get loaded at runtime. Such a change is required to support more platforms in future for which kwindowsystem cannot provide support directly. An example is proper Wayland support on the Plasma platform. It needs to provide its own implementationn which has to differ from the generic Wayland implementation. Diffs - autotests/CMakeLists.txt dda0af259b292ee5e526bb9166811fc1b376f388 src/CMakeLists.txt ff2ce392ecd7969eb94543528c7a670ea0fcd870 src/fixx11h.h src/kkeyserver_mac.h src/kkeyserver_mac.cpp src/kkeyserver_win.h src/kkeyserver_win.cpp src/kkeyserver_x11.h src/kkeyserver_x11.cpp src/kmanagerselection.h src/kselectionowner.h src/kselectionowner.cpp src/kselectionwatcher.h src/kselectionwatcher.cpp src/kwindoweffects_x11.cpp src/kwindowinfo_mac.cpp src/kwindowinfo_mac_p.h src/kwindowinfo_p_x11.h src/kwindowinfo_win.cpp src/kwindowinfo_x11.cpp src/kwindowsystem.cpp 789132e1b4883dd54218d29af9710dedfe6218e1 src/kwindowsystem_mac.cpp src/kwindowsystem_p_wayland.h src/kwindowsystem_p_x11.h src/kwindowsystem_wayland.cpp src/kwindowsystem_win.cpp src/kwindowsystem_x11.cpp src/kxerrorhandler.cpp src/kxerrorhandler_p.h src/kxmessages.h src/kxmessages.cpp src/kxutils.cpp src/kxutils_p.h src/netwm.h src/netwm.cpp src/netwm_p.h src/platforms/CMakeLists.txt PRE-CREATION src/platforms/osx/CMakeLists.txt PRE-CREATION src/platforms/wayland/CMakeLists.txt PRE-CREATION src/platforms/windows/CMakeLists.txt PRE-CREATION src/platforms/xcb/CMakeLists.txt PRE-CREATION Diff: https://git.reviewboard.kde.org/r/124212/diff/ Testing --- compiles, installs and autotest pass on platform Linux/xcb Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124220: kwindowsystem: Add a plugin infrastructure for platform specific implementations
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124220/ --- (Updated July 7, 2015, 11:31 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 569a72353ba8ca9eb9f1208b7cc4c5d28f9e by Martin Gräßlin to branch master. Repository: kwindowsystem Description --- A plugin interface is added which allows a platform specific plugin to implement an interface. If the plugin does not implement the interface, it will fall back to the default implementation. Also if no plugin can be resolved it will fall back to the default implementation. This replaces the existing compile time and runtime selection. In order to make this work the KWindowInfoPrivate is changed from a templated approach to using pure virtuals just like the other private implementations in this library. As the platform specific parts are no longer compiled in we cannot just delegate the KWindowSystem::icon with NETWinInfo overload to the xcb implementation. In order to solve this problem the required method is added to the private interface with a default implementation which does not return anything. If we are not on platform xcb and KWindowSystem is compiled with X11 support the plugin for xcb is loaded and the call gets delegated to the xcb implementation. This allows e.g. KWin to still read icons for Xwayland clients. Diffs - src/CMakeLists.txt ff2ce392ecd7969eb94543528c7a670ea0fcd870 src/config-kwindowsystem.h.cmake fa0eec115870be27a17ec7b398e40f0c7506f11b src/kwindoweffects.cpp fd88e20e1728506f135bcd5ecda3c05754839717 src/kwindoweffects_dummy.cpp 3e24cecb5c7d25883c179b622abdb5ab06587c33 src/kwindoweffects_dummy_p.h PRE-CREATION src/kwindoweffects_p.h 7c740da952f279a2c5fe689daa5a06c131fa9c9d src/kwindowinfo.cpp f29828581cdaecb7613c3f62cff72aa1fc33c266 src/kwindowinfo_dummy_p.h PRE-CREATION src/kwindowinfo_p.h 6727dd1715a13e5bd7793275620c5fa682318f1c src/kwindowsystem.cpp 789132e1b4883dd54218d29af9710dedfe6218e1 src/kwindowsystem_dummy_p.h PRE-CREATION src/kwindowsystem_p.h 0b5f3e8aeb7b70234c61c59979abd840f349b154 src/kwindowsystemplugininterface.cpp PRE-CREATION src/kwindowsystemplugininterface_p.h PRE-CREATION src/platforms/wayland/CMakeLists.txt PRE-CREATION src/platforms/wayland/plugin.h PRE-CREATION src/platforms/wayland/plugin.cpp PRE-CREATION src/platforms/wayland/wayland.json PRE-CREATION src/platforms/xcb/CMakeLists.txt PRE-CREATION src/platforms/xcb/kwindoweffects.cpp src/platforms/xcb/kwindoweffects_x11.h PRE-CREATION src/platforms/xcb/kwindowinfo.cpp src/platforms/xcb/kwindowinfo_p_x11.h src/platforms/xcb/kwindowsystem.cpp src/platforms/xcb/kwindowsystem_p_x11.h src/platforms/xcb/plugin.h PRE-CREATION src/platforms/xcb/plugin.cpp PRE-CREATION src/platforms/xcb/xcb.json PRE-CREATION src/pluginwrapper.cpp PRE-CREATION src/pluginwrapper_p.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/124220/diff/ Testing --- * unit tests still pass (X11) * kwin_wayland still shows icons for Xwayland clients Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124222: kidletime: Introduce plugin infrastructure for platform specific parts
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124222/ --- (Updated July 7, 2015, 11:34 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit ef79ebfccac039d2a497c3a4d6f8ee7a2f4304ee by Martin Gräßlin to branch master. Repository: kidletime Description --- Instead of having the platform specific implementations hard compiled in, they are split out into plugins which can get loaded at runtime depending on the platform we are in. Diffs - src/CMakeLists.txt 23e5e2914e3a8991d14b664d80ac0d4b60545b40 src/abstractsystempoller.h 00202d00b19fbbbd7443e07e411bdedecca230ee src/kidletime.cpp fc4ce77454db52d1bbaf9831c378f22548a237a7 src/logging.h PRE-CREATION src/logging.cpp PRE-CREATION src/macpoller.h aab6a3acc16d5fb2b9a3156a142125a15d7e610f src/macpoller.cpp src/org.freedesktop.ScreenSaver.xml src/plugins/CMakeLists.txt PRE-CREATION src/plugins/osx/CMakeLists.txt PRE-CREATION src/plugins/osx/osx.json PRE-CREATION src/plugins/windows/CMakeLists.txt PRE-CREATION src/plugins/windows/windows.json PRE-CREATION src/plugins/xscreensaver/CMakeLists.txt PRE-CREATION src/plugins/xscreensaver/xcb.json PRE-CREATION src/plugins/xsync/CMakeLists.txt PRE-CREATION src/plugins/xsync/fixx11h.h PRE-CREATION src/plugins/xsync/xcb.json PRE-CREATION src/widgetbasedpoller.h fac0a724d32fb79a3c9b7829cba3b17f03bfff32 src/windowspoller.h 502ed6dd6c6eaae1b35a14dce66812ebec30677e src/windowspoller.cpp src/xscreensaverbasedpoller.h 363ec521faa39e5b996c9e6767171f72005d11ca src/xscreensaverbasedpoller.cpp src/xsyncbasedpoller.h 8f67cbed38519e15cf5a96b5086da348ec8fabd5 src/xsyncbasedpoller.cpp Diff: https://git.reviewboard.kde.org/r/124222/diff/ Testing --- Tried the example app on X11: it correctly loads the xsync plugin. If I modify it to be not available it correctly loads the xscreensaver plugin. In both cases the interaction is correct. With --platform wayland no plugin gets loaded (as expected) and the functionality is pretty much broken. Osx and Windows are obviously neither compile nor runtime tested. But I adjusted the code to my best knowledge. Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124266: Introduce an env variable to overwrite the platform name
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124266/ --- (Updated July 7, 2015, 11:35 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit fd607506642195b1cfd58d92b5d720d888b711b1 by Martin Gräßlin to branch master. Bugs: 349911 https://bugs.kde.org/show_bug.cgi?id=349911 Repository: kglobalaccel Description --- With the env variable KGLOBALACCELD_PLATFORM the platform name can be specified, which is being used for searching for the plugin to load. This is required because some plugins are more specific than the supported platforms. If the environment variable is not specified, the QGuiApplication::platformName() is used as before. CCBUG: 349911 Diffs - src/runtime/globalshortcutsregistry.cpp 1340e0e39c1f412b6138fa80cdb93bf81c8dd593 Diff: https://git.reviewboard.kde.org/r/124266/diff/ Testing --- KWin adjusted to use this environment variable: Loaded plugin /opt/kf5/lib/x86_64-linux-gnu/plugins/org.kde.kglobalaccel5.platforms/KF5GlobalAccelPrivateKWin.so for platform org.kde.kwin Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124133: Add dedicated Version tab to KAboutApplicationDialog
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124133/ --- (Updated July 7, 2015, 11:26 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 9c47f37bd1faa39d130739e6a49003442bd3388b by Martin Gräßlin to branch master. Repository: kxmlgui Description --- This replaces the version information of the version and frameworks and moves it into a dedicated tab. In this tab the information is extended by the Qt version (which is equally relevant as e.g. the frameworks version) in both runtime and compile time. Also windowing system is added. This will become a useful information for KWin developers starting in Plasma 5.4 when users start to test things out and we need to know whether the window they experience the problem with is running on wayland or xwayland. Diffs - src/kaboutapplicationdialog.cpp 5eeea7711aa4f95a9cd4191d68ad330ef795caea Diff: https://git.reviewboard.kde.org/r/124133/diff/ Testing --- File Attachments New wayland, new X11, old X11 https://git.reviewboard.kde.org/media/uploaded/files/2015/06/20/b76ba6ae-b01c-4c6a-9248-29d39c652d83__snapshot_J11265.png Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82175 --- I like it! - Martin Gräßlin On July 7, 2015, 2:52 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 2:52 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated heinä 7, 2015, 2 ip)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Changes
---
Repelaced few missed NULL's with Q_NULLPTR.
Removed end-of-line whitespace from FindVOIKKO.cmake
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs (updated)
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/#review82174
---
src/plugins/voikko/voikkoclient.h (line 37)
https://git.reviewboard.kde.org/r/124282/#comment56563
it's override, no need to specify it's virtual.
src/plugins/voikko/voikkodict.cpp (line 71)
https://git.reviewboard.kde.org/r/124282/#comment56567
This line does nothing.
src/plugins/voikko/voikkodict.cpp (line 125)
https://git.reviewboard.kde.org/r/124282/#comment56564
use constFind, now you're looking up the word twice.
src/plugins/voikko/voikkodict.cpp (line 261)
https://git.reviewboard.kde.org/r/124282/#comment56566
qCDebug(SONNET_VOIKKO) Loaded words.size() replacements from
the user dictionary.;
Same above.
I don't really know much about Voikko or even Sonnet, so just reviewing the
code.
- Aleix Pol Gonzalez
On July 7, 2015, 4 p.m., Jesse Jaara wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated July 7, 2015, 4 p.m.)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 124282: Implement Voikko based spellchecker for Sonnet
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61cf8de95301516fc80e7fed378215b447c
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124220: kwindowsystem: Add a plugin infrastructure for platform specific implementations
On July 7, 2015, 2:16 p.m., Hrvoje Senjan wrote: Seems kwindowsystem master doesn't build now (against Qt 5.5 at least): ``` [ 55s] In file included from /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp:21:0: [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo_p.h:72:11: error: 'QScopedPointer' does not name a type [ 55s] const QScopedPointerPrivate d; [ 55s]^ [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp: In constructor 'KWindowInfoPrivate::KWindowInfoPrivate(WId, NET::Properties, NET::Properties2)': [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp:55:7: error: class 'KWindowInfoPrivate' does not have any field named 'd' [ 55s] : d(new Private(window, properties, properties2)) [ 55s]^ [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp: In member function 'WId KWindowInfoPrivate::win() const': [ 55s] /home/abuild/rpmbuild/BUILD/kwindowsystem-5.12.0git.20150707T133122~569a723/src/kwindowinfo.cpp:65:12: error: 'd' was not declared in this scope [ 55s] return d-window; [ 55s] ^ ``` Should be fixed now. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124220/#review82168 --- On July 7, 2015, 1:31 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124220/ --- (Updated July 7, 2015, 1:31 p.m.) Review request for KDE Frameworks. Repository: kwindowsystem Description --- A plugin interface is added which allows a platform specific plugin to implement an interface. If the plugin does not implement the interface, it will fall back to the default implementation. Also if no plugin can be resolved it will fall back to the default implementation. This replaces the existing compile time and runtime selection. In order to make this work the KWindowInfoPrivate is changed from a templated approach to using pure virtuals just like the other private implementations in this library. As the platform specific parts are no longer compiled in we cannot just delegate the KWindowSystem::icon with NETWinInfo overload to the xcb implementation. In order to solve this problem the required method is added to the private interface with a default implementation which does not return anything. If we are not on platform xcb and KWindowSystem is compiled with X11 support the plugin for xcb is loaded and the call gets delegated to the xcb implementation. This allows e.g. KWin to still read icons for Xwayland clients. Diffs - src/CMakeLists.txt ff2ce392ecd7969eb94543528c7a670ea0fcd870 src/config-kwindowsystem.h.cmake fa0eec115870be27a17ec7b398e40f0c7506f11b src/kwindoweffects.cpp fd88e20e1728506f135bcd5ecda3c05754839717 src/kwindoweffects_dummy.cpp 3e24cecb5c7d25883c179b622abdb5ab06587c33 src/kwindoweffects_dummy_p.h PRE-CREATION src/kwindoweffects_p.h 7c740da952f279a2c5fe689daa5a06c131fa9c9d src/kwindowinfo.cpp f29828581cdaecb7613c3f62cff72aa1fc33c266 src/kwindowinfo_dummy_p.h PRE-CREATION src/kwindowinfo_p.h 6727dd1715a13e5bd7793275620c5fa682318f1c src/kwindowsystem.cpp 789132e1b4883dd54218d29af9710dedfe6218e1 src/kwindowsystem_dummy_p.h PRE-CREATION src/kwindowsystem_p.h 0b5f3e8aeb7b70234c61c59979abd840f349b154 src/kwindowsystemplugininterface.cpp PRE-CREATION src/kwindowsystemplugininterface_p.h PRE-CREATION src/platforms/wayland/CMakeLists.txt PRE-CREATION src/platforms/wayland/plugin.h PRE-CREATION src/platforms/wayland/plugin.cpp PRE-CREATION src/platforms/wayland/wayland.json PRE-CREATION src/platforms/xcb/CMakeLists.txt PRE-CREATION src/platforms/xcb/kwindoweffects.cpp src/platforms/xcb/kwindoweffects_x11.h PRE-CREATION src/platforms/xcb/kwindowinfo.cpp src/platforms/xcb/kwindowinfo_p_x11.h src/platforms/xcb/kwindowsystem.cpp src/platforms/xcb/kwindowsystem_p_x11.h src/platforms/xcb/plugin.h PRE-CREATION src/platforms/xcb/plugin.cpp PRE-CREATION src/platforms/xcb/xcb.json PRE-CREATION src/pluginwrapper.cpp PRE-CREATION src/pluginwrapper_p.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/124220/diff/ Testing --- * unit tests still pass (X11) * kwin_wayland still shows icons for Xwayland clients Thanks, Martin Gräßlin ___
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82172 --- src/knotificationmanager.cpp (line 87) https://git.reviewboard.kde.org/r/124281/#comment56560 Would it make any sense to remove this altogether? If nobody is using it, it sounds useless. Was it used for those Ubuntu-specific notifications, maybe? src/notifybypopup.cpp (line 561) https://git.reviewboard.kde.org/r/124281/#comment56561 Will that pass the DPI test? Have you tested with a different QT_DEVICE_PIXEL_RATIO? - Aleix Pol Gonzalez On July 7, 2015, 2:52 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 2:52 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated heinä 7, 2015, 2:44 ip)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Changes
---
* Don't mark methods virtual
* path = path.absolutePath(); does nothing, removed
* use path instead of QDir(directory) in the lambda's return statement
* Remove annoying qCDebug() clause, it's friend was already removode earlier
* Use constFind instead of contains to look for replacements
* Change Last NULL to Q_NULLPTR
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs (updated)
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 7, 2015, 3:56 p.m., Aleix Pol Gonzalez wrote: src/knotificationmanager.cpp, line 89 https://git.reviewboard.kde.org/r/124281/diff/1/?file=383552#file383552line89 Would it make any sense to remove this altogether? If nobody is using it, it sounds useless. Was it used for those Ubuntu-specific notifications, maybe? No, there were requests for custom plugins, but nobody was using it because the plugin class was not public until very recently. So yeah, this is wanted feature and I'd like to keep it in. On July 7, 2015, 3:56 p.m., Aleix Pol Gonzalez wrote: src/notifybypopup.cpp, line 565 https://git.reviewboard.kde.org/r/124281/diff/1/?file=383553#file383553line565 Will that pass the DPI test? Have you tested with a different QT_DEVICE_PIXEL_RATIO? Yeah it works fine if the app has the Qt::AA_UseHighDpiPixmaps attribute set (which they are supposed to be setting for hidpi support), then it scales correctly with any QT_DEVICE_PIXEL_RATIO. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82172 --- On July 7, 2015, 2:52 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 2:52 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 4:42 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Changes --- Set Qt::AA_UseHighDpiPixmaps attribute on the kpassivepopup test Apps are meant to set it and so should the tests Repository: knotifications Description --- This patch reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework. KService is used only for locating additional notification plugins and to my knowledge, there are none such plugins currently existing, at least not in all around KDE plus the class for the plugins wasn't exported until about two months ago, so this should be safe without a legacy support. KIconThemes is used only to get KIconLoader::Small icon pixmap for notifications using KPassivePopup. After some playing around it turns out that it puts the icon into the KPassivePopup title and makes it as big as the text. So I've made the icon size to be the same as the text height. So this keeps things visually the same + still DPI aware, though I believe the KPassivePopup should receive a complete visual overhaul anyway. Additionally, KCodecs dependency has again one single usage for decoding html entities to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs. Diffs (updated) - CMakeLists.txt 2d5437b metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/notifybypopup.cpp e377051 tests/kpassivepopuptest.h bc0dedc tests/kpassivepopuptest.cpp 2486fd8 Diff: https://git.reviewboard.kde.org/r/124281/diff/ Testing --- Everything still compiles + I added a test for KPassivePopup with an icon. Thanks, Martin Klapetek ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124282: Implement Voikko based spellchecker for Sonnet
On heinä 7, 2015, 2:11 ip, Aleix Pol Gonzalez wrote:
src/plugins/voikko/voikkodict.cpp, line 261
https://git.reviewboard.kde.org/r/124282/diff/1/?file=383564#file383564line261
qCDebug(SONNET_VOIKKO) Loaded words.size() replacements
from the user dictionary.;
Same above.
That's make the ting extreamly nasty if someone ever decides that they also want
the debugging and warning messages to be translated. That line mostlikely only
gets called once during the lifetime of the application, so performance wise it
isn't an issue.
- Jesse
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/#review82174
---
On heinä 7, 2015, 2:44 ip, Jesse Jaara wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124282/
---
(Updated heinä 7, 2015, 2:44 ip)
Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
Repository: sonnet
Description
---
# Implement Voikko based spellchecker for Sonnet
## Description
Implements a spell chekcing plugin based on libvoikko
http://voikko.puimula.org/.
Primarily for supporting highquality Finnishs spell checking, but HFST
trancuders
can be found several other languages too.
http://sourceforge.net/projects/hfst/files/resources/spell-transducers/
## List of commits (oldest 1st)
---
Define QLoggingCategory for for voikko speller plugin
* Declared SONNET_VOIKKO QLoggingCategory
--
Implement Voikko based spellchecker (dictionary)
* All Sonnet::SpellerPlugin functions are implemented.
* storeReplacement() and addToPersonal() use Json based storage.
* File location:
* UNIX OSX:
QStandardPaths::GenericDataLocation/Sonnet/Voikko-user-dictionary.json
* Windows = Vista:
QSP::GenericDataLocation/../Roaming/Sonnet/Voikko-user-dictionary.json
* XP: QSP::GenericDataLocation/../../Aplication
Data/Sonnet/Voikko-user-dictionary.json
* Format:
```JSON
{ languageId: {
PersonalWords: [
word
],
Replacements: [
{bad: eror,
good: error}
]
}
```
* Before use VoikkoDict based chekkers must be ensured to be with valid with
initFailed().
As so the ctor is protected and only accessible from friens class
VoikkoClient, which
does this check before returning the speller. Using an invalid speller will
result in
null-pointer exceptions.
--
Implement Sonnet::Client for Voikko speller
* Reliability set to 50.
Voikko is primarily only used for Finnish at the moment, although
the HFST transducer-backend has added support for other languages
of varying quality.
As for Finnish (99% of use cases) the results are top quality.
In any case the reliability should be higher than that of hunspell
and aspell to prevent them from kicking in for Finnish, as the
Finnish dictionarys for them are low-quality.
* Name is Voikko
--
Add in CMakeBits needed to compile Voikko speller.
* Added FindVOIKKO module
Diffs
-
cmake/FindVOIKKO.cmake PRE-CREATION
src/plugins/CMakeLists.txt 3d24d61
src/plugins/voikko/CMakeLists.txt PRE-CREATION
src/plugins/voikko/voikkoclient.h PRE-CREATION
src/plugins/voikko/voikkoclient.cpp PRE-CREATION
src/plugins/voikko/voikkodebug.h PRE-CREATION
src/plugins/voikko/voikkodebug.cpp PRE-CREATION
src/plugins/voikko/voikkodict.h PRE-CREATION
src/plugins/voikko/voikkodict.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/124282/diff/
Testing
---
Thanks,
Jesse Jaara
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
