Re: Review Request 121084: Rename libmolletnetwork to avoid conflict with KDE4
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121084/#review70581 --- Ship it! Looks sane to me, also solves a real-world problem. Thanks! - Sebastian Kügler On Nov. 9, 2014, 4:10 p.m., Armin K. wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121084/ --- (Updated Nov. 9, 2014, 4:10 p.m.) Review request for KDE Runtime, KDE Frameworks and Plasma. Repository: kio-extras Description --- KDE-Runtime already provides libmolletnetwork, so lets rename this one to include 5 as a suffix since many apps outside of KDE still depend on KDE-Runtime. Diffs - network/ioslave/CMakeLists.txt 06a964d network/kded/CMakeLists.txt 3be676e network/network/CMakeLists.txt c0fb43e Diff: https://git.reviewboard.kde.org/r/121084/diff/ Testing --- Thanks, Armin K.
Re: desktoptojson and list properties
On Monday, November 17, 2014 18:50:04 Milian Wolff wrote: Or can we nowadays write the .json files directly, i.e. can scripty/ki18n cope with them nowadays? So, any chance we can use .json directly here? That should be possible, you can just drop the json file in there, and reference that in the K_PLUGIN_FACTORY_WITH_JSON macro. So scripty and the ki18n crew can cope with .json files nowadays? No reason to use .desktop at all anymore? That would be an acceptable solution for me. Though I guess desktoptojson should get a big fat warning that this it is deprecated and one should better write .json directly to prevent such issues from occurring randomly in other projects. Had I read your email, I'd have said no. I don't think scripty is ready for that yet, and desktoptojson isn't deprecated (and I doubt it will be for some time). -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Re: Review Request 121161: Set KIO::Integration::AccessManager to null so we don't crash on close.
On Nov. 17, 2014, 9:45 nachm., Thomas Lübking wrote:
attica-kde/kdeplugin/kdeplatformdependent.cpp, line 56
https://git.reviewboard.kde.org/r/121161/diff/2/?file=328928#file328928line56
is
KdePlatformDependent::~KdePlatformDependent()
{
if (QCoreApplication::closingDown())
m_accessManager-setParent(nullptr);
}
sufficient?
Albert Astals Cid wrote:
Isn't that a more complex way of doing the same?
Thomas Lübking wrote:
Depends on whether KdePlatformDependent is ever deleted in a running
application (where the leak would really matter)
If not, then yes: superfluous complication.
Jeremy Whiting wrote:
Did I hear a Ship it! in there somewhere?
Leaving aside that don't maintain the attica plugin, you have a +1 from here,
IF
a) the conditional reparent on destruction is not sufficient (doesn't work)
OR
b) KdePlatformDependent object(s) are not supposed to be deleted during the
runtime of the application
Ie. if the leak is inevitable or only theoretical.
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/#review70542
---
On Nov. 17, 2014, 9:11 nachm., Jeremy Whiting wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/
---
(Updated Nov. 17, 2014, 9:11 nachm.)
Review request for kdelibs and Plasma.
Repository: plasma-desktop
Description
---
Plugin unload order was making the attica_kde plugin crash on close,
this works around it by leaking one AccessManager.
Diffs
-
attica-kde/kdeplugin/kdeplatformdependent.cpp
489c03b18b7bb940007ab51cb197105fbc25de9f
Diff: https://git.reviewboard.kde.org/r/121161/diff/
Testing
---
knewstuff tests no longer crash on close.
Thanks,
Jeremy Whiting
Re: desktoptojson and list properties / i18n of JSON files
On Tuesday 18 November 2014 12:48:42 Sebastian Kügler wrote: On Monday, November 17, 2014 18:50:04 Milian Wolff wrote: Or can we nowadays write the .json files directly, i.e. can scripty/ki18n cope with them nowadays? So, any chance we can use .json directly here? That should be possible, you can just drop the json file in there, and reference that in the K_PLUGIN_FACTORY_WITH_JSON macro. So scripty and the ki18n crew can cope with .json files nowadays? No reason to use .desktop at all anymore? That would be an acceptable solution for me. Though I guess desktoptojson should get a big fat warning that this it is deprecated and one should better write .json directly to prevent such issues from occurring randomly in other projects. Had I read your email, I'd have said no. I don't think scripty is ready for that yet, and desktoptojson isn't deprecated (and I doubt it will be for some time). Criss-cross! So back to the start. What can we do about this? Since this is a serious blocker for KDevelop, I'm open for suggestions and would be willing to spent time on improving the situation. What blocks i18n from working on JSON files, does anyone know more about this? I think fixing that would be time well spent, contrary to improving desktoptojson. Bye -- Milian Wolff m...@milianw.de http://milianw.de
Re: Review Request 121086: Rename jpegcreatorsettings.kcfg to avoid conflicts with KDE4
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121086/#review70583 --- thumbnail/CMakeLists.txt https://git.reviewboard.kde.org/r/121086/#comment49407 whitespace What I'm missing is migration of the setting, is that relevant, any other reason why it's missing? - Sebastian Kügler On Nov. 9, 2014, 4:09 p.m., Armin K. wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121086/ --- (Updated Nov. 9, 2014, 4:09 p.m.) Review request for KDE Runtime, KDE Frameworks and Plasma. Repository: kio-extras Description --- Simply add 5 as a suffix to avoid conflicts with KDE 4 version which is shipped in KDE-Runtime which isn't going away any time now. Diffs - thumbnail/CMakeLists.txt e9ab79b thumbnail/jpegcreator.cpp de4902b thumbnail/jpegcreatorsettings.kcfg 8f68f46 thumbnail/jpegcreatorsettings.kcfgc 3f6cdab thumbnail/jpegcreatorsettings5.kcfg PRE-CREATION thumbnail/jpegcreatorsettings5.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/121086/diff/ Testing --- Thanks, Armin K.
Review Request 121169: Use QFile::decodeName for command in KDEsuDialog to fix encoding
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121169/ --- Review request for KDE Runtime. Repository: kde-runtime Description --- KDEsu command may contain non-latin characters so we need to use QFile::decodeName to display them properly. Diffs - kdesu/kdesu/kdesu.cpp cfd37ec Diff: https://git.reviewboard.kde.org/r/121169/diff/ Testing --- This patch is used in ROSA and ALT Linux for a long time. http://bugs.rosalinux.ru/show_bug.cgi?id=1902 Thanks, Andrey Bondrov
Re: Review Request 121161: Set KIO::Integration::AccessManager to null so we don't crash on close.
On Nov. 17, 2014, 2:45 p.m., Thomas Lübking wrote:
attica-kde/kdeplugin/kdeplatformdependent.cpp, line 56
https://git.reviewboard.kde.org/r/121161/diff/2/?file=328928#file328928line56
is
KdePlatformDependent::~KdePlatformDependent()
{
if (QCoreApplication::closingDown())
m_accessManager-setParent(nullptr);
}
sufficient?
Albert Astals Cid wrote:
Isn't that a more complex way of doing the same?
Thomas Lübking wrote:
Depends on whether KdePlatformDependent is ever deleted in a running
application (where the leak would really matter)
If not, then yes: superfluous complication.
Jeremy Whiting wrote:
Did I hear a Ship it! in there somewhere?
Thomas Lübking wrote:
Leaving aside that don't maintain the attica plugin, you have a +1 from
here, IF
a) the conditional reparent on destruction is not sufficient (doesn't
work)
OR
b) KdePlatformDependent object(s) are not supposed to be deleted during
the runtime of the application
Ie. if the leak is inevitable or only theoretical.
Yep, I'm one of the attica and knewstuff maintainers and got the plugin to load
recently, then hit this crash on close, so trying to fix it. The code in the
plugin is only built with the plugin, so shouldn't be used besides as a plugin.
The conditional reparent probably works too, but as Albert said it's just a
more complicated way of doing the same thing.
- Jeremy
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/#review70542
---
On Nov. 17, 2014, 2:11 p.m., Jeremy Whiting wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/
---
(Updated Nov. 17, 2014, 2:11 p.m.)
Review request for kdelibs and Plasma.
Repository: plasma-desktop
Description
---
Plugin unload order was making the attica_kde plugin crash on close,
this works around it by leaking one AccessManager.
Diffs
-
attica-kde/kdeplugin/kdeplatformdependent.cpp
489c03b18b7bb940007ab51cb197105fbc25de9f
Diff: https://git.reviewboard.kde.org/r/121161/diff/
Testing
---
knewstuff tests no longer crash on close.
Thanks,
Jeremy Whiting
Re: Review Request 121161: Set KIO::Integration::AccessManager to null so we don't crash on close.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121161/#review70601 --- Ship it! Ship It! - Albert Astals Cid On nov. 17, 2014, 9:11 p.m., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121161/ --- (Updated nov. 17, 2014, 9:11 p.m.) Review request for kdelibs and Plasma. Repository: plasma-desktop Description --- Plugin unload order was making the attica_kde plugin crash on close, this works around it by leaking one AccessManager. Diffs - attica-kde/kdeplugin/kdeplatformdependent.cpp 489c03b18b7bb940007ab51cb197105fbc25de9f Diff: https://git.reviewboard.kde.org/r/121161/diff/ Testing --- knewstuff tests no longer crash on close. Thanks, Jeremy Whiting
Re: Review Request 121161: Set KIO::Integration::AccessManager to null so we don't crash on close.
On Nov. 17, 2014, 9:45 nachm., Thomas Lübking wrote:
attica-kde/kdeplugin/kdeplatformdependent.cpp, line 56
https://git.reviewboard.kde.org/r/121161/diff/2/?file=328928#file328928line56
is
KdePlatformDependent::~KdePlatformDependent()
{
if (QCoreApplication::closingDown())
m_accessManager-setParent(nullptr);
}
sufficient?
Albert Astals Cid wrote:
Isn't that a more complex way of doing the same?
Thomas Lübking wrote:
Depends on whether KdePlatformDependent is ever deleted in a running
application (where the leak would really matter)
If not, then yes: superfluous complication.
Jeremy Whiting wrote:
Did I hear a Ship it! in there somewhere?
Thomas Lübking wrote:
Leaving aside that don't maintain the attica plugin, you have a +1 from
here, IF
a) the conditional reparent on destruction is not sufficient (doesn't
work)
OR
b) KdePlatformDependent object(s) are not supposed to be deleted during
the runtime of the application
Ie. if the leak is inevitable or only theoretical.
Jeremy Whiting wrote:
Yep, I'm one of the attica and knewstuff maintainers and got the plugin
to load recently, then hit this crash on close, so trying to fix it. The code
in the plugin is only built with the plugin, so shouldn't be used besides as
a plugin.
The conditional reparent probably works too, but as Albert said it's just
a more complicated way of doing the same thing.
The code in the plugin is only built with the plugin, so shouldn't be used
besides as a plugin.
That's not what I asked.
This:
int main(int, char**) {
KdePlatformDependent dep; // lives as long as the process
}
does not actually leak.
This:
int main(int, char**) {
for (int i = 0; i 100; ++i)
KdePlatformDependent dep; // lives for the iteration only
}
does.
If the change causes an *actual* leak and that's avoidable, I'd suggest to
avoid it.
If not, than it really doesn't matter.
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/#review70542
---
On Nov. 17, 2014, 9:11 nachm., Jeremy Whiting wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/
---
(Updated Nov. 17, 2014, 9:11 nachm.)
Review request for kdelibs and Plasma.
Repository: plasma-desktop
Description
---
Plugin unload order was making the attica_kde plugin crash on close,
this works around it by leaking one AccessManager.
Diffs
-
attica-kde/kdeplugin/kdeplatformdependent.cpp
489c03b18b7bb940007ab51cb197105fbc25de9f
Diff: https://git.reviewboard.kde.org/r/121161/diff/
Testing
---
knewstuff tests no longer crash on close.
Thanks,
Jeremy Whiting
Re: Review Request 121161: Set KIO::Integration::AccessManager to null so we don't crash on close.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121161/ --- (Updated Nov. 18, 2014, 7:30 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and Plasma. Repository: plasma-desktop Description --- Plugin unload order was making the attica_kde plugin crash on close, this works around it by leaking one AccessManager. Diffs - attica-kde/kdeplugin/kdeplatformdependent.cpp 489c03b18b7bb940007ab51cb197105fbc25de9f Diff: https://git.reviewboard.kde.org/r/121161/diff/ Testing --- knewstuff tests no longer crash on close. Thanks, Jeremy Whiting
Re: Review Request 121169: Use QFile::decodeName for command in KDEsuDialog to fix encoding
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121169/#review70605 --- Ship it! Please commit to kdesu framework as well - Lukáš Tinkl On Lis. 18, 2014, 5:47 odp., Andrey Bondrov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121169/ --- (Updated Lis. 18, 2014, 5:47 odp.) Review request for KDE Runtime. Repository: kde-runtime Description --- KDEsu command may contain non-latin characters so we need to use QFile::decodeName to display them properly. Diffs - kdesu/kdesu/kdesu.cpp cfd37ec Diff: https://git.reviewboard.kde.org/r/121169/diff/ Testing --- This patch is used in ROSA and ALT Linux for a long time. http://bugs.rosalinux.ru/show_bug.cgi?id=1902 Thanks, Andrey Bondrov
Re: desktoptojson and list properties / i18n of JSON files
El Dimarts, 18 de novembre de 2014, a les 14:09:00, Milian Wolff va escriure: On Tuesday 18 November 2014 12:48:42 Sebastian Kügler wrote: On Monday, November 17, 2014 18:50:04 Milian Wolff wrote: Or can we nowadays write the .json files directly, i.e. can scripty/ki18n cope with them nowadays? So, any chance we can use .json directly here? That should be possible, you can just drop the json file in there, and reference that in the K_PLUGIN_FACTORY_WITH_JSON macro. So scripty and the ki18n crew can cope with .json files nowadays? No reason to use .desktop at all anymore? That would be an acceptable solution for me. Though I guess desktoptojson should get a big fat warning that this it is deprecated and one should better write .json directly to prevent such issues from occurring randomly in other projects. Had I read your email, I'd have said no. I don't think scripty is ready for that yet, and desktoptojson isn't deprecated (and I doubt it will be for some time). Criss-cross! So back to the start. What can we do about this? Since this is a serious blocker for KDevelop, I'm open for suggestions and would be willing to spent time on improving the situation. What blocks i18n from working on JSON files, does anyone know more about this? I think fixing that would be time well spent, contrary to improving desktoptojson. Why do we need i18n support in json files? Cheers, Albert Bye
Re: desktoptojson and list properties / i18n of JSON files
On Tuesday 18 November 2014 22:31:08 Albert Astals Cid wrote: El Dimarts, 18 de novembre de 2014, a les 14:09:00, Milian Wolff va escriure: On Tuesday 18 November 2014 12:48:42 Sebastian Kügler wrote: On Monday, November 17, 2014 18:50:04 Milian Wolff wrote: Or can we nowadays write the .json files directly, i.e. can scripty/ki18n cope with them nowadays? So, any chance we can use .json directly here? That should be possible, you can just drop the json file in there, and reference that in the K_PLUGIN_FACTORY_WITH_JSON macro. So scripty and the ki18n crew can cope with .json files nowadays? No reason to use .desktop at all anymore? That would be an acceptable solution for me. Though I guess desktoptojson should get a big fat warning that this it is deprecated and one should better write .json directly to prevent such issues from occurring randomly in other projects. Had I read your email, I'd have said no. I don't think scripty is ready for that yet, and desktoptojson isn't deprecated (and I doubt it will be for some time). Criss-cross! So back to the start. What can we do about this? Since this is a serious blocker for KDevelop, I'm open for suggestions and would be willing to spent time on improving the situation. What blocks i18n from working on JSON files, does anyone know more about this? I think fixing that would be time well spent, contrary to improving desktoptojson. Why do we need i18n support in json files? Same reason we have i18n support for .desktop files. KPluginMetaData or what its called uses that e.g. to show the localized names of plugins etc., no? Bye -- Milian Wolff m...@milianw.de http://milianw.de
Re: desktoptojson and list properties / i18n of JSON files
El Dimarts, 18 de novembre de 2014, a les 23:35:48, Milian Wolff va escriure: On Tuesday 18 November 2014 22:31:08 Albert Astals Cid wrote: El Dimarts, 18 de novembre de 2014, a les 14:09:00, Milian Wolff va escriure: On Tuesday 18 November 2014 12:48:42 Sebastian Kügler wrote: On Monday, November 17, 2014 18:50:04 Milian Wolff wrote: Or can we nowadays write the .json files directly, i.e. can scripty/ki18n cope with them nowadays? So, any chance we can use .json directly here? That should be possible, you can just drop the json file in there, and reference that in the K_PLUGIN_FACTORY_WITH_JSON macro. So scripty and the ki18n crew can cope with .json files nowadays? No reason to use .desktop at all anymore? That would be an acceptable solution for me. Though I guess desktoptojson should get a big fat warning that this it is deprecated and one should better write .json directly to prevent such issues from occurring randomly in other projects. Had I read your email, I'd have said no. I don't think scripty is ready for that yet, and desktoptojson isn't deprecated (and I doubt it will be for some time). Criss-cross! So back to the start. What can we do about this? Since this is a serious blocker for KDevelop, I'm open for suggestions and would be willing to spent time on improving the situation. What blocks i18n from working on JSON files, does anyone know more about this? I think fixing that would be time well spent, contrary to improving desktoptojson. Why do we need i18n support in json files? Same reason we have i18n support for .desktop files. KPluginMetaData or what its called uses that e.g. to show the localized names of plugins etc., no? I didn't even know we were using json now. Why did we change from .desktop file to .json ones? What's the benefit? Seems like .desktop files did their job good enough and we have all the tooling available already. Sorry to ask old-man questions :D Cheers, Albert Bye
Re: desktoptojson and list properties / i18n of JSON files
On Tuesday 18 November 2014 23:45:56 Albert Astals Cid wrote: I didn't even know we were using json now. Why did we change from .desktop file to .json ones? What's the benefit? Seems like .desktop files did their job good enough and we have all the tooling available already. Because that's what Qt uses - Qt5 plugins have JSON metadata as standard, meaning it's all in one file (the JSON is embedded in the plugin, although it can be read without loading the plugin). Alex
Re: desktoptojson and list properties / i18n of JSON files
El Dimarts, 18 de novembre de 2014, a les 23:01:14, Alex Merry va escriure: On Tuesday 18 November 2014 23:45:56 Albert Astals Cid wrote: I didn't even know we were using json now. Why did we change from .desktop file to .json ones? What's the benefit? Seems like .desktop files did their job good enough and we have all the tooling available already. Because that's what Qt uses - Qt5 plugins have JSON metadata as standard, meaning it's all in one file (the JSON is embedded in the plugin, although it can be read without loading the plugin). And Qt introduced a technology without making it translatable? What fields do we need to make translatable? Can somebody point me to such a .json file we'd like to translate? Cheers, Albert Alex
Re: desktoptojson and list properties / i18n of JSON files
On Wednesday 19 November 2014 00:09:25 Albert Astals Cid wrote: El Dimarts, 18 de novembre de 2014, a les 23:01:14, Alex Merry va escriure: On Tuesday 18 November 2014 23:45:56 Albert Astals Cid wrote: I didn't even know we were using json now. Why did we change from .desktop file to .json ones? What's the benefit? Seems like .desktop files did their job good enough and we have all the tooling available already. Because that's what Qt uses - Qt5 plugins have JSON metadata as standard, meaning it's all in one file (the JSON is embedded in the plugin, although it can be read without loading the plugin). And Qt introduced a technology without making it translatable? What fields do we need to make translatable? Can somebody point me to such a .json file we'd like to translate? Here's one: http://pastebin.kde.org/p4p38fqr1 That's kdevpatchreview.json, generated from kdevpatchreview.desktop via kcoreaddons_desktop_to_json(...) during the CMake run. Cheers -- Kevin Funk | kf...@kde.org | http://kfunk.org
Re: Review Request 121169: Use QFile::decodeName for command in KDEsuDialog to fix encoding
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121169/ --- (Updated Ноя. 19, 2014, 1:52 д.п.) Status -- This change has been marked as submitted. Review request for KDE Runtime. Repository: kde-runtime Description --- KDEsu command may contain non-latin characters so we need to use QFile::decodeName to display them properly. Diffs - kdesu/kdesu/kdesu.cpp cfd37ec Diff: https://git.reviewboard.kde.org/r/121169/diff/ Testing --- This patch is used in ROSA and ALT Linux for a long time. http://bugs.rosalinux.ru/show_bug.cgi?id=1902 Thanks, Andrey Bondrov
Re: Review Request 121169: Use QFile::decodeName for command in KDEsuDialog to fix encoding
On Ноя. 18, 2014, 7:46 п.п., Lukáš Tinkl wrote: Please commit to kdesu framework as well Done. - Andrey --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121169/#review70605 --- On Ноя. 19, 2014, 1:52 д.п., Andrey Bondrov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121169/ --- (Updated Ноя. 19, 2014, 1:52 д.п.) Review request for KDE Runtime. Repository: kde-runtime Description --- KDEsu command may contain non-latin characters so we need to use QFile::decodeName to display them properly. Diffs - kdesu/kdesu/kdesu.cpp cfd37ec Diff: https://git.reviewboard.kde.org/r/121169/diff/ Testing --- This patch is used in ROSA and ALT Linux for a long time. http://bugs.rosalinux.ru/show_bug.cgi?id=1902 Thanks, Andrey Bondrov
