Re: Review Request: fix infinite recurssion in kcategorizedview
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103313/#review8695 --- kdeui/itemviews/kcategorizedview.cpp <http://git.reviewboard.kde.org/r/103313/#comment7323> I would say 'no halfway exit always' especially in long functions. Midfunc exit is too easily overlooked. That said, if most of processing is done in else loop if you elide the midfunc exit then the function is even more a refactoring candidate. If readability suffers... yet another reason for refactoring. - Ruurd Pels On Dec. 4, 2011, 8:42 a.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103313/ > --- > > (Updated Dec. 4, 2011, 8:42 a.m.) > > > Review request for kdelibs and Rafael Fernández López. > > > Description > --- > > Basically, what I do is: > If there are one or zero columns, hide the horizontalScrollBar until it is > needed. (it has worked in the past, but in another file). > Apply the same strategy with files. > > Additional stuff: > Moved the common calculus of itemSize outside of the if then else. > > > This addresses bugs 213068 and 287847. > http://bugs.kde.org/show_bug.cgi?id=213068 > http://bugs.kde.org/show_bug.cgi?id=287847 > > > Diffs > - > > kdeui/itemviews/kcategorizedview.cpp 46a1cde > kutils/kpluginselector.cpp ca0691d > > Diff: http://git.reviewboard.kde.org/r/103313/diff/diff > > > Testing > --- > > Krunner config does not loop (neither kgetnewstuff from kstars). I can not > test with amarok (I've hit by an amarok start bug). > Please, test with other programs. > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: fix infinite recurssion in kcategorizedview
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103313/#review8683 --- Refactor it. Maybe even add a message since this might be a solution for some problem elsewhere... kdeui/itemviews/kcategorizedview.cpp <http://git.reviewboard.kde.org/r/103313/#comment7316> Argh. Exit method halfway. I'd prefer reworking the trailing part of the function (refactoring a bit that is) instead of exiting halfway. Should be as easy as moving the rest of the function in the else clause AFAICS on short notice. - Ruurd Pels On Dec. 3, 2011, 10:55 a.m., Jaime Torres Amate wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103313/ > --- > > (Updated Dec. 3, 2011, 10:55 a.m.) > > > Review request for kdelibs. > > > Description > --- > > Basically, what I do is: > If there are one or zero columns, hide the horizontalScrollBar until it is > needed. (it has worked in the past, but in another file). > Apply the same strategy with files. > > Additional stuff: > Moved the common calculus of itemSize outside of the if then else. > > > This addresses bugs 213068 and 287847. > http://bugs.kde.org/show_bug.cgi?id=213068 > http://bugs.kde.org/show_bug.cgi?id=287847 > > > Diffs > - > > kdeui/itemviews/kcategorizedview.cpp 46a1cde > kutils/kpluginselector.cpp ca0691d > > Diff: http://git.reviewboard.kde.org/r/103313/diff/diff > > > Testing > --- > > Krunner config does not loop (neither kgetnewstuff from kstars). I can not > test with amarok (I've hit by an amarok start bug). > Please, test with other programs. > > > Thanks, > > Jaime Torres Amate > >
Re: Review Request: Fix crash in KonqView
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103176/#review8281 --- Ship it! OK ship it although it might mask off deeper troubles. konqueror/src/konqview.h <http://git.reviewboard.kde.org/r/103176/#comment7071> OK I can agree with returning an empty list if m_service == null however I also think we need to come up with reasons WHY m_service == null before serviceTypes() is called. From the original code it looks like m_service should always be initialized beforehand, so why is it that that did not happen for that particular website. Maybe an errormessage is in order? - Ruurd Pels On Nov. 17, 2011, 8:33 p.m., Dawit Alemayehu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103176/ > --- > > (Updated Nov. 17, 2011, 8:33 p.m.) > > > Review request for KDE Base Apps and David Faure. > > > Description > --- > > Though I cannot reproduce the bug given above, the backtrace attached to the > report seems to show a NULL object is being accessed. This patch is intended > to prevent such scenarios and in the process fix bugs like the one reported > in bug# 285844. > > > This addresses bug 285844. > http://bugs.kde.org/show_bug.cgi?id=285844 > > > Diffs > - > > konqueror/src/konqrun.cpp 9e9310c > konqueror/src/konqview.h 45c5bde > > Diff: http://git.reviewboard.kde.org/r/103176/diff/diff > > > Testing > --- > > > Thanks, > > Dawit Alemayehu > >
Re: detection if applet is running
On Friday 28 October 2011 12:46:38 Andriy Rysin wrote: > Yes, but I can get the keyboard layouts from x11 directly (that's what > applets and systray is doing), that'll be more efficient and would not > require dbus or even kded daemon running I think it would be preferred to use KDE functionality in this case. Why pass by KDE and Qt and go directly to X11 to do that? That would also mean that you would have to take into account that KDE and Qt run on different platforms that handle keyboard layouts differently. You must abide by separation of concern. In your application your concern is to determine wether a particular applet is running. It is NOT your concern how that fact is asserted. If it is necessary to add functionality to be able to let others determine if you are running so be it - then add that functionality to the applet either by letting the applet expose something over DBUS or by any other means necessary and add that to KDE and/or Qt. -- Ruurd Pels, Boogerd 1, 1791 GW Den Burg - Texel, The Netherlands ru...@kde.nl http://about.me/ruurdpels +31612914545
Re: detection if applet is running
On Thursday 27 October 2011 23:35:52 Andriy Rysin wrote:
> I did a quick research and I see two ways to solve this in
> keyboard_layout_widget (embedded component used in lockdlg):
> 1) link plasma libs and do something like (in pseudo-code)
> foreach(containment, new Corona()->containments()) {
> foreach(applet, containment->applets()) {
> if( applet->pluginName() == "..." ) {
> // show indicator
> }
> }
> }
No.
> 2) make keyboard layout applet exposes some dbus to make it detectable
> when running (feels a bit too "heavy")
Yes. Better. Why does it feel heavy? I'd venture the thought that doing so
could expose the keybaord layout applet in such a way that it also could do
something usefull (like changing layouts for example :-) )
> It felt like there must be a way currently to use dbus on plasma to
> query running applets (which would the easiest way to implement what I
> need) but could not find anything like that in org.kde.plasma-desktop
--
Ruurd Pels, Boogerd 1, 1791 GW Den Burg - Texel, The Netherlands
ru...@kde.nl http://about.me/ruurdpels +31612914545
Re: Review Request: Avoid possible null pointer dereferences in khtml
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102794/#review7169
---
Ship it!
Ship It!
- Ruurd Pels
On Oct. 7, 2011, 4:25 p.m., Jaime Torres Amate wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102794/
> ---
>
> (Updated Oct. 7, 2011, 4:25 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> ---
>
> The real changes are:
> Avoid possible null pointer dereferences in khtml.
> The common check
> if (a && something(a)) do return bla(a) else blabla(a) uses blabla(a)
> with null a. changed to
> if (a) { if something(a)) do return bla(a) else blabla(a) }
>
> As a side effect:
> kate has removed a lot of tailing spaces in the edited files.
> Avoid a possible crash checking the index limit before accesing the array.
> Move some variables inside the #ifdef block where they are used
>
>
> Diffs
> -
>
> khtml/css/css_valueimpl.cpp 3fb2898
> khtml/ecma/kjs_window.cpp 0e7394b
> khtml/html/htmltokenizer.cpp b64e83d
> khtml/java/kjavaappletserver.cpp 234c6f3
> khtml/khtml_part.cpp 53929fa
> khtml/khtmlimage.cpp c6e6366
> khtml/khtmlview.cpp 28dbac3
> khtml/rendering/render_form.cpp c15247a
> khtml/rendering/render_table.cpp 5b07714
> khtml/xpath/util.cpp 079008d
>
> Diff: http://git.reviewboard.kde.org/r/102794/diff/diff
>
>
> Testing
> ---
>
> no regressions in kdelibs tests.
>
>
> Thanks,
>
> Jaime Torres Amate
>
>
Re: Review Request: Avoid possible null pointer dereferences in khtml
> On Oct. 7, 2011, 2:35 p.m., Ruurd Pels wrote:
> > Revert the whitespace changes with git. Other than that I dislike multiple
> > exit points even in trivially short functions. It really does not hurt to
> > create a local variable holding the result of the function and then return
> > that as the result of a function. Another pet-peeve is negative tests, for
> > example ... if ( !xxx->isEmpty() ) ... and would prefer conditions always
> > yielding true, for example if ( xxx->isEmpty() == false) ) instead.
>
> Christoph Feck wrote:
> If you read the "!isEmpty" as "is not Empty", the first version sounds
> clearer, natural, and easier to understand than the second version.
Free after my fellow countryman Edsger Dijkstra: Negate considered harmful. It
is easy to overlook the !-operator and negative tests require an extra mental
step to be able to follow the logic. And yes it is a pet peeve.
- Ruurd
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102794/#review7165
---
On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102794/
> ---
>
> (Updated Oct. 7, 2011, 2:13 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> ---
>
> The real changes are:
> Avoid possible null pointer dereferences in khtml.
> The common check
> if (a && something(a)) do return bla(a) else blabla(a) uses blabla(a)
> with null a. changed to
> if (a) { if something(a)) do return bla(a) else blabla(a) }
>
> As a side effect:
> kate has removed a lot of tailing spaces in the edited files.
> Avoid a possible crash checking the index limit before accesing the array.
> Move some variables inside the #ifdef block where they are used
>
>
> Diffs
> -
>
> khtml/css/css_valueimpl.cpp 3fb2898
> khtml/ecma/kjs_window.cpp 0e7394b
> khtml/html/htmltokenizer.cpp b64e83d
> khtml/java/kjavaappletserver.cpp 234c6f3
> khtml/khtml_part.cpp 53929fa
> khtml/khtmlimage.cpp c6e6366
> khtml/khtmlview.cpp 28dbac3
> khtml/rendering/render_form.cpp c15247a
> khtml/rendering/render_table.cpp 5b07714
> khtml/xpath/util.cpp 079008d
>
> Diff: http://git.reviewboard.kde.org/r/102794/diff/diff
>
>
> Testing
> ---
>
> no regressions in kdelibs tests.
>
>
> Thanks,
>
> Jaime Torres Amate
>
>
Re: Review Request: Avoid possible null pointer dereferences in khtml
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102794/#review7166
---
Revert the whitespace changes with git. Other than that I dislike multiple exit
points even in trivially short functions. It really does not hurt to create a
local variable holding the result of the function and then return that as the
result of a function. Another pet-peeve is negative tests, for example ... if (
!xxx->isEmpty() ) ... and would prefer conditions always yielding true, for
example if ( xxx->isEmpty() == false) ) instead.
- Ruurd Pels
On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102794/
> ---
>
> (Updated Oct. 7, 2011, 2:13 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> ---
>
> The real changes are:
> Avoid possible null pointer dereferences in khtml.
> The common check
> if (a && something(a)) do return bla(a) else blabla(a) uses blabla(a)
> with null a. changed to
> if (a) { if something(a)) do return bla(a) else blabla(a) }
>
> As a side effect:
> kate has removed a lot of tailing spaces in the edited files.
> Avoid a possible crash checking the index limit before accesing the array.
> Move some variables inside the #ifdef block where they are used
>
>
> Diffs
> -
>
> khtml/css/css_valueimpl.cpp 3fb2898
> khtml/ecma/kjs_window.cpp 0e7394b
> khtml/html/htmltokenizer.cpp b64e83d
> khtml/java/kjavaappletserver.cpp 234c6f3
> khtml/khtml_part.cpp 53929fa
> khtml/khtmlimage.cpp c6e6366
> khtml/khtmlview.cpp 28dbac3
> khtml/rendering/render_form.cpp c15247a
> khtml/rendering/render_table.cpp 5b07714
> khtml/xpath/util.cpp 079008d
>
> Diff: http://git.reviewboard.kde.org/r/102794/diff/diff
>
>
> Testing
> ---
>
> no regressions in kdelibs tests.
>
>
> Thanks,
>
> Jaime Torres Amate
>
>
Re: Review Request: Avoid possible null pointer dereferences in khtml
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102794/#review7165
---
Revert the whitespace changes with git. Other than that I dislike multiple exit
points even in trivially short functions. It really does not hurt to create a
local variable holding the result of the function and then return that as the
result of a function. Another pet-peeve is negative tests, for example ... if (
!xxx->isEmpty() ) ... and would prefer conditions always yielding true, for
example if ( xxx->isEmpty() == false) ) instead.
- Ruurd Pels
On Oct. 7, 2011, 2:13 p.m., Jaime Torres Amate wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102794/
> ---
>
> (Updated Oct. 7, 2011, 2:13 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> ---
>
> The real changes are:
> Avoid possible null pointer dereferences in khtml.
> The common check
> if (a && something(a)) do return bla(a) else blabla(a) uses blabla(a)
> with null a. changed to
> if (a) { if something(a)) do return bla(a) else blabla(a) }
>
> As a side effect:
> kate has removed a lot of tailing spaces in the edited files.
> Avoid a possible crash checking the index limit before accesing the array.
> Move some variables inside the #ifdef block where they are used
>
>
> Diffs
> -
>
> khtml/css/css_valueimpl.cpp 3fb2898
> khtml/ecma/kjs_window.cpp 0e7394b
> khtml/html/htmltokenizer.cpp b64e83d
> khtml/java/kjavaappletserver.cpp 234c6f3
> khtml/khtml_part.cpp 53929fa
> khtml/khtmlimage.cpp c6e6366
> khtml/khtmlview.cpp 28dbac3
> khtml/rendering/render_form.cpp c15247a
> khtml/rendering/render_table.cpp 5b07714
> khtml/xpath/util.cpp 079008d
>
> Diff: http://git.reviewboard.kde.org/r/102794/diff/diff
>
>
> Testing
> ---
>
> no regressions in kdelibs tests.
>
>
> Thanks,
>
> Jaime Torres Amate
>
>
Re: X11 expert help needed
On Tuesday 26 July 2011 22.43.33 Alexander Neundorf wrote: > > Should the stuff we do just be merged into the cmake version ? > > Can somebody who knows more about X11 please have a look at these two > > files, one in kdelibs, the other one in cmake 2.8.5 or git HEAD ? > > http://cmake.org/gitweb?p=cmake.git;a=summary What happens if you use de cmake version (copy over kde version then try a cmake... How much extra does de KDE version do and is it possible to do that in a sort of X11ExtraCheck cmake? R
