Re: Review Request: trivial fixes for some warnings of clang++ (2.99.9999)
> On Nov. 27, 2011, 10:22 a.m., Ben Cooksley wrote: > > This fix causes a nasty regression which causes the plugin list in KRunner > > and other apps to be nearly unusable when compiled with gcc. Please adjust > > the KPluginSelector component of this fix as it must be causing a behaviour > > change. > > Jaime Torres Amate wrote: > Could you explain more about the problems you've noticed or fill a bug > report? > Do I have some time to try to fix the problem you've found in KRunner, > instead of reverting the code to a wrong status (as not intended by the > developer) again? > > > Ben Cooksley wrote: > When trying to configure KRunner with this patch applied, CPU usage by > KRunner jumps to about 90% on my system. Checking with gdb shows that quite a > few geometry change events are being generated and widgets are being hidden > and shown again in very quick succession. > > Reverting your patch and recompiling leads KRunner to use 0-1% of the CPU > when browsing the list of plugins in it's configure dialog. > > Unfortunately there is no definitive backtrace for this issue. > I think you do have some time to find a proper fix for the issue, yes. > > You can reproduce by opening KRunner and entering it's configuration > dialog. The whole of KRunner will become very unresponsive and CPU usage > should shoot up to high levels. > > Jaime Torres Amate wrote: > 100% reproducible. I'll do my best. gdb to the rescue. > > Thomas Lübking wrote: > try pushbutton->minimumSizeHint() I'll try that (minimumSizeHint()). What I've done so far: * kill my krunner process * valgrind --tool=callgrind krunner * Alt+f2, config * whait 2 minutes * Kill the window with Ctrl+Alt+Esc * See the usage with kcachegrind. So far I've seen that (without detecting cycles in kcachegrind) : void KCategorizedView::Private::topToBottomVisualRect(const QModelIndex &index, Item &item, const Block &block, const QPoint &blockPos) const and QRect KCategorizedView::visualRect(const QModelIndex &index) const Are being called too much (mutual recursion?) - Jaime Torres --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103165/#review8503 --- On Nov. 17, 2011, 2:37 p.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103165/ > --- > > (Updated Nov. 17, 2011, 2:37 p.m.) > > > Review request for kdelibs. > > > Description > --- > > Trivial c++ fixes for some warnings of clang++ (2.99.) > memset the structure size, not the pointer size (fortunately, the structure > size is greater than pointer size in this case). > remove double parenthesis > change false to 0 as pointer parameter > use parenthesis to clarify preference between ? and + > > > Diffs > - > > kdecore/sycoca/kmemfile.cpp a466bde > kdeui/fonts/kfontchooser.cpp 541f7db > kdeui/tests/ktabwidgettest.cpp fdc3161 > kross/modules/form.cpp 5e260c1 > kutils/kemoticons/kemoticonstheme.cpp c145741 > kutils/kpluginselector.cpp 0a39fcc > > Diff: http://git.reviewboard.kde.org/r/103165/diff/diff > > > Testing > --- > > Still compiles with g++ > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: trivial fixes for some warnings of clang++ (2.99.9999)
> On Nov. 27, 2011, 10:22 a.m., Ben Cooksley wrote: > > This fix causes a nasty regression which causes the plugin list in KRunner > > and other apps to be nearly unusable when compiled with gcc. Please adjust > > the KPluginSelector component of this fix as it must be causing a behaviour > > change. > > Jaime Torres Amate wrote: > Could you explain more about the problems you've noticed or fill a bug > report? > Do I have some time to try to fix the problem you've found in KRunner, > instead of reverting the code to a wrong status (as not intended by the > developer) again? > > > Ben Cooksley wrote: > When trying to configure KRunner with this patch applied, CPU usage by > KRunner jumps to about 90% on my system. Checking with gdb shows that quite a > few geometry change events are being generated and widgets are being hidden > and shown again in very quick succession. > > Reverting your patch and recompiling leads KRunner to use 0-1% of the CPU > when browsing the list of plugins in it's configure dialog. > > Unfortunately there is no definitive backtrace for this issue. > I think you do have some time to find a proper fix for the issue, yes. > > You can reproduce by opening KRunner and entering it's configuration > dialog. The whole of KRunner will become very unresponsive and CPU usage > should shoot up to high levels. > > Jaime Torres Amate wrote: > 100% reproducible. I'll do my best. gdb to the rescue. try pushbutton->minimumSizeHint() - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103165/#review8503 --- On Nov. 17, 2011, 2:37 p.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103165/ > --- > > (Updated Nov. 17, 2011, 2:37 p.m.) > > > Review request for kdelibs. > > > Description > --- > > Trivial c++ fixes for some warnings of clang++ (2.99.) > memset the structure size, not the pointer size (fortunately, the structure > size is greater than pointer size in this case). > remove double parenthesis > change false to 0 as pointer parameter > use parenthesis to clarify preference between ? and + > > > Diffs > - > > kdecore/sycoca/kmemfile.cpp a466bde > kdeui/fonts/kfontchooser.cpp 541f7db > kdeui/tests/ktabwidgettest.cpp fdc3161 > kross/modules/form.cpp 5e260c1 > kutils/kemoticons/kemoticonstheme.cpp c145741 > kutils/kpluginselector.cpp 0a39fcc > > Diff: http://git.reviewboard.kde.org/r/103165/diff/diff > > > Testing > --- > > Still compiles with g++ > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: trivial fixes for some warnings of clang++ (2.99.9999)
> On Nov. 27, 2011, 10:22 a.m., Ben Cooksley wrote: > > This fix causes a nasty regression which causes the plugin list in KRunner > > and other apps to be nearly unusable when compiled with gcc. Please adjust > > the KPluginSelector component of this fix as it must be causing a behaviour > > change. > > Jaime Torres Amate wrote: > Could you explain more about the problems you've noticed or fill a bug > report? > Do I have some time to try to fix the problem you've found in KRunner, > instead of reverting the code to a wrong status (as not intended by the > developer) again? > > > Ben Cooksley wrote: > When trying to configure KRunner with this patch applied, CPU usage by > KRunner jumps to about 90% on my system. Checking with gdb shows that quite a > few geometry change events are being generated and widgets are being hidden > and shown again in very quick succession. > > Reverting your patch and recompiling leads KRunner to use 0-1% of the CPU > when browsing the list of plugins in it's configure dialog. > > Unfortunately there is no definitive backtrace for this issue. > I think you do have some time to find a proper fix for the issue, yes. > > You can reproduce by opening KRunner and entering it's configuration > dialog. The whole of KRunner will become very unresponsive and CPU usage > should shoot up to high levels. 100% reproducible. I'll do my best. gdb to the rescue. - Jaime Torres --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103165/#review8503 --- On Nov. 17, 2011, 2:37 p.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103165/ > --- > > (Updated Nov. 17, 2011, 2:37 p.m.) > > > Review request for kdelibs. > > > Description > --- > > Trivial c++ fixes for some warnings of clang++ (2.99.) > memset the structure size, not the pointer size (fortunately, the structure > size is greater than pointer size in this case). > remove double parenthesis > change false to 0 as pointer parameter > use parenthesis to clarify preference between ? and + > > > Diffs > - > > kdecore/sycoca/kmemfile.cpp a466bde > kdeui/fonts/kfontchooser.cpp 541f7db > kdeui/tests/ktabwidgettest.cpp fdc3161 > kross/modules/form.cpp 5e260c1 > kutils/kemoticons/kemoticonstheme.cpp c145741 > kutils/kpluginselector.cpp 0a39fcc > > Diff: http://git.reviewboard.kde.org/r/103165/diff/diff > > > Testing > --- > > Still compiles with g++ > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: trivial fixes for some warnings of clang++ (2.99.9999)
> On Nov. 27, 2011, 10:22 a.m., Ben Cooksley wrote: > > This fix causes a nasty regression which causes the plugin list in KRunner > > and other apps to be nearly unusable when compiled with gcc. Please adjust > > the KPluginSelector component of this fix as it must be causing a behaviour > > change. > > Jaime Torres Amate wrote: > Could you explain more about the problems you've noticed or fill a bug > report? > Do I have some time to try to fix the problem you've found in KRunner, > instead of reverting the code to a wrong status (as not intended by the > developer) again? > When trying to configure KRunner with this patch applied, CPU usage by KRunner jumps to about 90% on my system. Checking with gdb shows that quite a few geometry change events are being generated and widgets are being hidden and shown again in very quick succession. Reverting your patch and recompiling leads KRunner to use 0-1% of the CPU when browsing the list of plugins in it's configure dialog. Unfortunately there is no definitive backtrace for this issue. I think you do have some time to find a proper fix for the issue, yes. You can reproduce by opening KRunner and entering it's configuration dialog. The whole of KRunner will become very unresponsive and CPU usage should shoot up to high levels. - Ben --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103165/#review8503 --- On Nov. 17, 2011, 2:37 p.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103165/ > --- > > (Updated Nov. 17, 2011, 2:37 p.m.) > > > Review request for kdelibs. > > > Description > --- > > Trivial c++ fixes for some warnings of clang++ (2.99.) > memset the structure size, not the pointer size (fortunately, the structure > size is greater than pointer size in this case). > remove double parenthesis > change false to 0 as pointer parameter > use parenthesis to clarify preference between ? and + > > > Diffs > - > > kdecore/sycoca/kmemfile.cpp a466bde > kdeui/fonts/kfontchooser.cpp 541f7db > kdeui/tests/ktabwidgettest.cpp fdc3161 > kross/modules/form.cpp 5e260c1 > kutils/kemoticons/kemoticonstheme.cpp c145741 > kutils/kpluginselector.cpp 0a39fcc > > Diff: http://git.reviewboard.kde.org/r/103165/diff/diff > > > Testing > --- > > Still compiles with g++ > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: trivial fixes for some warnings of clang++ (2.99.9999)
> On Nov. 27, 2011, 10:22 a.m., Ben Cooksley wrote: > > This fix causes a nasty regression which causes the plugin list in KRunner > > and other apps to be nearly unusable when compiled with gcc. Please adjust > > the KPluginSelector component of this fix as it must be causing a behaviour > > change. Could you explain more about the problems you've noticed or fill a bug report? Do I have some time to try to fix the problem you've found in KRunner, instead of reverting the code to a wrong status (as not intended by the developer) again? - Jaime Torres --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103165/#review8503 --- On Nov. 17, 2011, 2:37 p.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103165/ > --- > > (Updated Nov. 17, 2011, 2:37 p.m.) > > > Review request for kdelibs. > > > Description > --- > > Trivial c++ fixes for some warnings of clang++ (2.99.) > memset the structure size, not the pointer size (fortunately, the structure > size is greater than pointer size in this case). > remove double parenthesis > change false to 0 as pointer parameter > use parenthesis to clarify preference between ? and + > > > Diffs > - > > kdecore/sycoca/kmemfile.cpp a466bde > kdeui/fonts/kfontchooser.cpp 541f7db > kdeui/tests/ktabwidgettest.cpp fdc3161 > kross/modules/form.cpp 5e260c1 > kutils/kemoticons/kemoticonstheme.cpp c145741 > kutils/kpluginselector.cpp 0a39fcc > > Diff: http://git.reviewboard.kde.org/r/103165/diff/diff > > > Testing > --- > > Still compiles with g++ > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: Further fix for Bug 285731
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103260/#review8537 --- Sorry didn't see the review request and did almost the same one ;) only difference is that i did put the check on the outer if, makes it behave slightly different but i don't hink it matters since when there is no scene is broken anyways (this as far i seen it does indeed happen during destruction) - Marco Martin On Nov. 26, 2011, 11:12 p.m., Xuetian Weng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103260/ > --- > > (Updated Nov. 26, 2011, 11:12 p.m.) > > > Review request for KDE Runtime and Plasma. > > > Description > --- > > Though marked as Fixed, 285731 still exists on kde-runtime master. > There is another scene variable need to be tested. > > > This addresses bug 285731. > http://bugs.kde.org/show_bug.cgi?id=285731 > > > Diffs > - > > plasma/declarativeimports/core/tooltip.cpp 4eaa5b8 > > Diff: http://git.reviewboard.kde.org/r/103260/diff/diff > > > Testing > --- > > No more crash > > > Thanks, > > Xuetian Weng > >
Re: Review Request: Fix KDateComboBox shrinking in size after a date is entered
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103133/#review8535 --- Unless there are objections before then, I propose to apply this patch on 30th November to meet the 4.7.4 deadline. - David Jarvie On Nov. 14, 2011, 10:58 p.m., David Jarvie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103133/ > --- > > (Updated Nov. 14, 2011, 10:58 p.m.) > > > Review request for kdelibs and John Layt. > > > Description > --- > > After the user enters a date by means of the date picker or by up/down > arrows, the edit field in KDateComboBox shrinks so that it is too small to > display all the characters in the date. In addition, when first displayed, > the widget visibly resizes, which doesn't look particularly good. This patch > fixes these issues. > > Note that the patch is really a workaround for the issue - I haven't managed > to find out exactly why the shrinkage occurs. However, it is a simple fix, so > unless somebody else comes up with a better way, I think it's good enough. > > > Diffs > - > > kdeui/widgets/kdatecombobox.cpp fc239bc > > Diff: http://git.reviewboard.kde.org/r/103133/diff/diff > > > Testing > --- > > Tested successfully in KAlarm's alarm edit dialog. > > > Thanks, > > David Jarvie > >
Re: Fwd: Requesting freeze exception for JtG
On Sunday, November 27, 2011 01:03:34 Pau Garcia i Quiles wrote: > On Sun, Nov 27, 2011 at 12:38 AM, Thomas Zander wrote: > > On Saturday 26 November 2011 14.09.21 Boudewijn Rempt wrote: > >> A little flexibility, a little actual testing of the patch > >> would be much more useful than stubbornly maintaining that > >> rules are rules. > > > > For those that missed it; the actual testing has been done and reported > > on kde-core-devel. (it has the same topic as this thread) > > This is my last mail on this. > > No, no testing has been shown and I'd say no testing has even been > performed. Sebas even admitted to that in another mailing list. Yes, I've not applied, compiled or run the patch. I can do it if it _really_ makes you feel better, though I prefer spending my time otherwise. In fact, I fully trust you that the patch applies and builds fine, and that my testing is no better than yours. That's not my issue however. Judging from reading it, your explanation and the screenshots, I think it's too intrusive (not really technically risky, but intrusive UI-wise -- we're adding a menu option to *all* KDE applications, more or less, and I think it goes in the wrong place for the wrong reaons). Putting the dialog into the About dialog solves this for me and makes the patch acceptable because then it's just an update of the support KDE pointers, not an entirely new piece of UI, which clutters the menu without clear value for the user. > > The flexibility has been shown by various people; to quote sebas as one > > example; > > «I personally don't mind a non-intrusive patch (one that just changes > > the text in there, for example), if the translation and docs team is OK > > with that. This does not constitute a new feature to me.» > > That is not flexibility. The result would be useless, so no, that > "solution" is unacceptable to me. I am not going to make a commit that > modifies a hidden text. Feel free to do it yourself, I already got the > freeze exception from kde-i18n-doc. I think it would be far from useless. If that solution is unacceptable to you, we end up with no change at all, and I really wonder how that is better for you than at least the JtG pointers in the About dialog. I fear that this all- or-nothing attitude doesn't reall help anybody, doesn't relax the situation, or is helpful at all to the purpose of your patch (promote the join the game campaign). Cheers, -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Review Request: Further fix for Bug 285731
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103260/ --- Review request for KDE Runtime and Plasma. Description --- Though marked as Fixed, 285731 still exists on kde-runtime master. There is another scene variable need to be tested. This addresses bug 285731. http://bugs.kde.org/show_bug.cgi?id=285731 Diffs - plasma/declarativeimports/core/tooltip.cpp 4eaa5b8 Diff: http://git.reviewboard.kde.org/r/103260/diff/diff Testing --- No more crash Thanks, Xuetian Weng
Re: Review Request: trivial fixes for some warnings of clang++ (2.99.9999)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103165/#review8503 --- This fix causes a nasty regression which causes the plugin list in KRunner and other apps to be nearly unusable when compiled with gcc. Please adjust the KPluginSelector component of this fix as it must be causing a behaviour change. - Ben Cooksley On Nov. 17, 2011, 2:37 p.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103165/ > --- > > (Updated Nov. 17, 2011, 2:37 p.m.) > > > Review request for kdelibs. > > > Description > --- > > Trivial c++ fixes for some warnings of clang++ (2.99.) > memset the structure size, not the pointer size (fortunately, the structure > size is greater than pointer size in this case). > remove double parenthesis > change false to 0 as pointer parameter > use parenthesis to clarify preference between ? and + > > > Diffs > - > > kdecore/sycoca/kmemfile.cpp a466bde > kdeui/fonts/kfontchooser.cpp 541f7db > kdeui/tests/ktabwidgettest.cpp fdc3161 > kross/modules/form.cpp 5e260c1 > kutils/kemoticons/kemoticonstheme.cpp c145741 > kutils/kpluginselector.cpp 0a39fcc > > Diff: http://git.reviewboard.kde.org/r/103165/diff/diff > > > Testing > --- > > Still compiles with g++ > > > Thanks, > > Jaime Torres Amate > >
