Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review56494 --- This review has been submitted with commit 7a829e7e270c8f3bdebbaaf2ebcde2e54ee862f3 by Alex Merry to branch master. - Commit Hook On April 14, 2014, 4:04 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 14, 2014, 4:04 p.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Note that we cannot provide a constructor overload that is identical other than omitting the catalog name argument, as that would be ambiguous (if 4 strings are passed, it could be either the new constructor or the old one). So instead, we provide two new constructors: one requiring at least 4 strings and the license type, and one only taking 3 strings. Diffs - autotests/kaboutdatatest.cpp 22b013c0626ebea94f371474048c670b3d50dad6 src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 25, 2014, 9:34 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Note that we cannot provide a constructor overload that is identical other than omitting the catalog name argument, as that would be ambiguous (if 4 strings are passed, it could be either the new constructor or the old one). So instead, we provide two new constructors: one requiring at least 4 strings and the license type, and one only taking 3 strings. Diffs - autotests/kaboutdatatest.cpp 22b013c0626ebea94f371474048c670b3d50dad6 src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review56425 --- Ship it! Ship It! - Kevin Ottens On April 14, 2014, 4:04 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 14, 2014, 4:04 p.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Note that we cannot provide a constructor overload that is identical other than omitting the catalog name argument, as that would be ambiguous (if 4 strings are passed, it could be either the new constructor or the old one). So instead, we provide two new constructors: one requiring at least 4 strings and the license type, and one only taking 3 strings. Diffs - autotests/kaboutdatatest.cpp 22b013c0626ebea94f371474048c670b3d50dad6 src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 14, 2014, 4:04 p.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Changes --- This version adds both short-form and long-form constructors. I've left the organization domain stuff that was discussed for another RR - this RR just preserves existing behaviour. I also extended the tests to actually test the new constructors. Repository: kcoreaddons Description (updated) --- Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Note that we cannot provide a constructor overload that is identical other than omitting the catalog name argument, as that would be ambiguous (if 4 strings are passed, it could be either the new constructor or the old one). So instead, we provide two new constructors: one requiring at least 4 strings and the license type, and one only taking 3 strings. Diffs (updated) - autotests/kaboutdatatest.cpp 22b013c0626ebea94f371474048c670b3d50dad6 src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review55486 --- Ship it! Ship It! - David Faure On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
On April 12, 2014, 9:16 a.m., David Faure wrote: Ship It! Well, I guess the first diff minimizes the porting effort indeed. Also: the domain name can only be passed to QCoreApp if this is the main aboutdata (we also have a KAboutData per plugin). But yeah, that seems to be missing in KAboutData::setApplicationData. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review55486 --- On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
On April 12, 2014, 9:16 a.m., David Faure wrote: Ship It! David Faure wrote: Well, I guess the first diff minimizes the porting effort indeed. Also: the domain name can only be passed to QCoreApp if this is the main aboutdata (we also have a KAboutData per plugin). But yeah, that seems to be missing in KAboutData::setApplicationData. I could also combine them and add both the short-form and long-form constructors. I guess the question is: do we want to push users towards a particular style? The short constructors and setters is what Qt has been moving towards, on the basis it makes more readable code; do we want to do the same with this class? The organization domain stuff has a fallback to kde.org; currently this doesn't really matter, as we don't actually do anything with it. But if we're passing it to QCoreApplication, we should think about in the frameworks world (with a hopefully wider audience for these frameworks) we really want that default there. It's easy enough to make the homepage setter change the organization domain if the org dom was never explicitly set (a bool flag would do the trick). - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review55486 --- On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
On April 12, 2014, 9:16 a.m., David Faure wrote: Ship It! David Faure wrote: Well, I guess the first diff minimizes the porting effort indeed. Also: the domain name can only be passed to QCoreApp if this is the main aboutdata (we also have a KAboutData per plugin). But yeah, that seems to be missing in KAboutData::setApplicationData. Alex Merry wrote: I could also combine them and add both the short-form and long-form constructors. I guess the question is: do we want to push users towards a particular style? The short constructors and setters is what Qt has been moving towards, on the basis it makes more readable code; do we want to do the same with this class? The organization domain stuff has a fallback to kde.org; currently this doesn't really matter, as we don't actually do anything with it. But if we're passing it to QCoreApplication, we should think about in the frameworks world (with a hopefully wider audience for these frameworks) we really want that default there. It's easy enough to make the homepage setter change the organization domain if the org dom was never explicitly set (a bool flag would do the trick). Right, let's have both. Domain name: if unset by app, whether it's kde.org or empty string will be wrong for apps that should get something else, so I would leave the default as is, it caters to the current majority of users of this code, and others have to set something anyway. Getting it from the homepage setter is probably fine though. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review55486 --- On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Changes --- Make the constructor require fewer arguments, not more. Repository: kcoreaddons Description (updated) --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs (updated) - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review54756 --- I think it's fine that you add the other one if you prefer people to use setters, but please don't deprecate the old one. There's little reason for doing so, it's very old API and it has always worked as is. - Aleix Pol Gonzalez On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
On April 1, 2014, 10:55 a.m., Aleix Pol Gonzalez wrote: I think it's fine that you add the other one if you prefer people to use setters, but please don't deprecate the old one. There's little reason for doing so, it's very old API and it has always worked as is. The reason for deprecating the old one is that it forces everyone to set the catalog name, which is useless. The aim is to move everyone away from doing that by KF6. Would the first diff version be more acceptable to you? - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review54756 --- On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 117275: Deprecate the catalog name stuff from KAboutData
On April 1, 2014, 10:55 a.m., Aleix Pol Gonzalez wrote: I think it's fine that you add the other one if you prefer people to use setters, but please don't deprecate the old one. There's little reason for doing so, it's very old API and it has always worked as is. Alex Merry wrote: The reason for deprecating the old one is that it forces everyone to set the catalog name, which is useless. The aim is to move everyone away from doing that by KF6. Would the first diff version be more acceptable to you? Yes I think it's better. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/#review54756 --- On April 1, 2014, 10:09 a.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- (Updated April 1, 2014, 10:09 a.m.) Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. I went with not allowing any optional arguments to the constructor, to encourage users of the class to use setters, which makes for more readable code. I deliberated giving it just one argument, but in the end went with the formerly-required arguments. The organizationDomain is not automatically set from the home page with this new usage style, as that only happened in the constructor and not in the setter. It could be set if the organizationDomain has not been explicitly set. However, the organizationDomain is not passed to QCoreApplication as I assumed it would be - it that intentional? Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 117275: Deprecate the catalog name stuff from KAboutData
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117275/ --- Review request for KDE Frameworks, David Faure, Kevin Ottens, and Michael Pyne. Repository: kcoreaddons Description --- This is another thing on the if only we'd spotted it before beta1 list. Unfortunately, removing the second argument from the constructor is completely ambiguous for calls that do not have at least enough arguments to specify the license. Deprecate the catalog name stuff from KAboutData This is pretty useless - the translation catalog has to be set before KAboutData is constructed in order to translate its arguments. Diffs - src/lib/kaboutdata.h cff1e3f67e33657fdd265a82166ef2a04cbcc3d1 src/lib/kaboutdata.cpp ce64a13aaa89bb4bc077f05e5f8e175d6a441ead Diff: https://git.reviewboard.kde.org/r/117275/diff/ Testing --- Builds, tests pass. Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
