Re: Review Request 122438: Notiy config leak fix try 2
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/ --- (Updated Feb. 5, 2015, 9:34 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Martin Klapetek. Repository: knotifications Description --- Similar to review 122396. Well... though knotifyconfig is not installed but ABI checker seems not happy with it. So let's just use the crappy knotifyconfig for now. Diffs - src/notifybytaskbar.cpp b52e180 src/knotificationmanager.cpp 3cf637c src/knotificationplugin.h 3438e7d src/notifybypopup.h a605a77 src/notifybypopup.cpp 2f08146 src/CMakeLists.txt 3e5821a Diff: https://git.reviewboard.kde.org/r/122438/diff/ Testing --- works. Thanks, Xuetian Weng ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122438: Notiy config leak fix try 2
On Feb. 5, 2015, 10:31 a.m., Mark Gaiser wrote: src/knotificationplugin.h, line 60 https://git.reviewboard.kde.org/r/122438/diff/1/?file=347092#file347092line60 Why wait? You can do that now and deprecate this function, right? You just can't remove it till KF6. Same for the other TODO lines you added. What do you want me to do? Adding an implementation to ::notify to plugin (to make it non-pure) and add new pure virtual function something like ::notify2? Another problem is readEntry in knotifyconfig doesn't work with const reference (while it should). It's a plugin interface, adding a new virtual function would make this interface confusing. And after discussed with martin, we may deprecate this class and have a plugin v2, since there's still some problem with existing knotifyconfig. There might be a new interface in 5.7 to clean all this up, but till then I think it's ok to keep it TODO like this. - Xuetian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/#review75461 --- On Feb. 5, 2015, 9:43 a.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/ --- (Updated Feb. 5, 2015, 9:43 a.m.) Review request for KDE Frameworks and Martin Klapetek. Repository: knotifications Description --- Similar to review 122396. Well... though knotifyconfig is not installed but ABI checker seems not happy with it. So let's just use the crappy knotifyconfig for now. Diffs - src/notifybytaskbar.cpp b52e180 src/knotificationmanager.cpp 3cf637c src/knotificationplugin.h 3438e7d src/notifybypopup.h a605a77 src/notifybypopup.cpp 2f08146 src/CMakeLists.txt 3e5821a Diff: https://git.reviewboard.kde.org/r/122438/diff/ Testing --- works. Thanks, Xuetian Weng ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122438: Notiy config leak fix try 2
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/#review75460 --- Ship it! Thank you! - Martin Klapetek On Feb. 5, 2015, 10:43 a.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/ --- (Updated Feb. 5, 2015, 10:43 a.m.) Review request for KDE Frameworks and Martin Klapetek. Repository: knotifications Description --- Similar to review 122396. Well... though knotifyconfig is not installed but ABI checker seems not happy with it. So let's just use the crappy knotifyconfig for now. Diffs - src/notifybytaskbar.cpp b52e180 src/knotificationmanager.cpp 3cf637c src/knotificationplugin.h 3438e7d src/notifybypopup.h a605a77 src/notifybypopup.cpp 2f08146 src/CMakeLists.txt 3e5821a Diff: https://git.reviewboard.kde.org/r/122438/diff/ Testing --- works. Thanks, Xuetian Weng ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122438: Notiy config leak fix try 2
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/#review75461 --- src/knotificationplugin.h https://git.reviewboard.kde.org/r/122438/#comment52186 Why wait? You can do that now and deprecate this function, right? You just can't remove it till KF6. Same for the other TODO lines you added. - Mark Gaiser On feb 5, 2015, 9:43 a.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122438/ --- (Updated feb 5, 2015, 9:43 a.m.) Review request for KDE Frameworks and Martin Klapetek. Repository: knotifications Description --- Similar to review 122396. Well... though knotifyconfig is not installed but ABI checker seems not happy with it. So let's just use the crappy knotifyconfig for now. Diffs - src/notifybytaskbar.cpp b52e180 src/knotificationmanager.cpp 3cf637c src/knotificationplugin.h 3438e7d src/notifybypopup.h a605a77 src/notifybypopup.cpp 2f08146 src/CMakeLists.txt 3e5821a Diff: https://git.reviewboard.kde.org/r/122438/diff/ Testing --- works. Thanks, Xuetian Weng ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
