Re: Review Request 114726: Make sure ktoolbar_unittest does not depend on global icon settings
On Dec. 30, 2013, 12:31 p.m., David Faure wrote: I don't really understand why this fails IMHO it *did* make sense for the KToolBar unittest to check which icon size the toolbars will get by default. The value of 2 on build.kde.org, for instance, should never never happen. 2 pixels for a toolbar icon is just unusable... David Faure wrote: Debugged and fixed in http://commits.kde.org/kxmlgui/94e84ebd02c6ab97e6ac4288f22f17337fc7948a + http://commits.kde.org/frameworkintegration/163282ed294f5c50a1a12be9eb50d8b96d87ab84 Please discard this review request. Alex Merry wrote: I still think these checks shouldn't be there; why is a kxmlgui unit test checking the default icon sizes of KIconTheme? It just makes it fragile if those defaults are changed (especially as they are in different repositories). You see as two different frameworks, I see it as one feature from the user's point of view. Toolbar icons start in a given size, then can be configured to different sizes (and the whole thing is quite complex, with XMLGUI and KConfig and layering of settings etc.). So the full test includes what do I start from, which is the default icon size coming from KIconTheme. I don't want to make that test less useful just for the hypothetical situation where one day we might change the default icon size (which sounds rather unlikely). Yes our autotests aren't all unit tests, some of them are more general feature tests. Anything that prevents regressions is good :) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/#review46444 --- On Dec. 29, 2013, 6:04 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- (Updated Dec. 29, 2013, 6:04 p.m.) Review request for KDE Frameworks. Repository: kxmlgui Description --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. NB: I am away until 2nd Jan, so if it gets a ship it, feel free to commit it in my absence in order to get Jenkins green again. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). 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 114726: Make sure ktoolbar_unittest does not depend on global icon settings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- (Updated Jan. 5, 2014, 12:31 p.m.) Status -- This change has been discarded. Review request for KDE Frameworks. Repository: kxmlgui Description --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. NB: I am away until 2nd Jan, so if it gets a ship it, feel free to commit it in my absence in order to get Jenkins green again. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). 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 114726: Make sure ktoolbar_unittest does not depend on global icon settings
On Dec. 30, 2013, 12:31 p.m., David Faure wrote: I don't really understand why this fails IMHO it *did* make sense for the KToolBar unittest to check which icon size the toolbars will get by default. The value of 2 on build.kde.org, for instance, should never never happen. 2 pixels for a toolbar icon is just unusable... David Faure wrote: Debugged and fixed in http://commits.kde.org/kxmlgui/94e84ebd02c6ab97e6ac4288f22f17337fc7948a + http://commits.kde.org/frameworkintegration/163282ed294f5c50a1a12be9eb50d8b96d87ab84 Please discard this review request. I still think these checks shouldn't be there; why is a kxmlgui unit test checking the default icon sizes of KIconTheme? It just makes it fragile if those defaults are changed (especially as they are in different repositories). - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/#review46444 --- On Dec. 29, 2013, 6:04 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- (Updated Dec. 29, 2013, 6:04 p.m.) Review request for KDE Frameworks. Repository: kxmlgui Description --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. NB: I am away until 2nd Jan, so if it gets a ship it, feel free to commit it in my absence in order to get Jenkins green again. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). 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 114726: Make sure ktoolbar_unittest does not depend on global icon settings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/#review46444 --- I don't really understand why this fails IMHO it *did* make sense for the KToolBar unittest to check which icon size the toolbars will get by default. The value of 2 on build.kde.org, for instance, should never never happen. 2 pixels for a toolbar icon is just unusable... - David Faure On Dec. 29, 2013, 6:04 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- (Updated Dec. 29, 2013, 6:04 p.m.) Review request for KDE Frameworks. Repository: kxmlgui Description --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. NB: I am away until 2nd Jan, so if it gets a ship it, feel free to commit it in my absence in order to get Jenkins green again. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). 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 114726: Make sure ktoolbar_unittest does not depend on global icon settings
On Dec. 30, 2013, 12:31 p.m., David Faure wrote: I don't really understand why this fails IMHO it *did* make sense for the KToolBar unittest to check which icon size the toolbars will get by default. The value of 2 on build.kde.org, for instance, should never never happen. 2 pixels for a toolbar icon is just unusable... Debugged and fixed in http://commits.kde.org/kxmlgui/94e84ebd02c6ab97e6ac4288f22f17337fc7948a + http://commits.kde.org/frameworkintegration/163282ed294f5c50a1a12be9eb50d8b96d87ab84 Please discard this review request. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/#review46444 --- On Dec. 29, 2013, 6:04 p.m., Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- (Updated Dec. 29, 2013, 6:04 p.m.) Review request for KDE Frameworks. Repository: kxmlgui Description --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. NB: I am away until 2nd Jan, so if it gets a ship it, feel free to commit it in my absence in order to get Jenkins green again. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 114726: Make sure ktoolbar_unittest does not depend on global icon settings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- Review request for KDE Frameworks. Repository: kxmlgui Description --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). 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 114726: Make sure ktoolbar_unittest does not depend on global icon settings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114726/ --- (Updated Dec. 29, 2013, 6:04 p.m.) Review request for KDE Frameworks. Repository: kxmlgui Description (updated) --- Make sure ktoolbar_unittest does not depend on global icon settings ktoolbar_unittest should not be testing the default settings of KIconTheme anyway. NB: I am away until 2nd Jan, so if it gets a ship it, feel free to commit it in my absence in order to get Jenkins green again. Diffs - autotests/ktoolbar_unittest.cpp 2ee490d35b517a8121aa0aeda0d6ebb4256caad0 Diff: https://git.reviewboard.kde.org/r/114726/diff/ Testing --- Tests pass on my local machine (but they did before as well). Thanks, Alex Merry ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
