[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: Prevent krunner to run the wrong command when
From:       wilderkde () gmail ! com
Date:       2009-02-24 20:15:14
Message-ID: 20090224201514.30205.61980 () localhost
[Download RAW message or body]


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/172/
-----------------------------------------------------------

(Updated 2009-02-24 12:15:13.669339)


Review request for Plasma.


Changes
-------

in a previous commit (which actually applied a patch that I proposed, *doh* ) \
m_queryRunning was set to false even if the last query did not start regardless its \
previous state; this patch solves *this* problem: if i type the /complete/ queryterm \
and hit enter the m_queryRunning is erroneously set to false regardless of its \
current state, so the old query it's free to run.  
Still, I don't know if the original problem is solved, old results are still \
delivered to setQueryMatches so in principle if one manages to  set m_delayedRun \
BEFORE the call, the call for old matches will still set m_queryRunning to false and \
run the item, regardless of the bug above.  Is that situation possible? 

I'm reluctant to use ||= to avoid lazy OR things or other optimization quirks. [I \
kind of remember that (a || b) can be compiled differently than (b||a); am I wrong?]

I can commit the fix once I have your green light.

--J


Summary
-------

This is an attempt to solve https://bugs.kde.org/show_bug.cgi?id=169283

The problem with the current implementation is that the mechanism for blocking a run \
of a result of a previous query does not work, as explained in my comment (the last \
one) in the br. 

The proposed way to solve this issue is to store the query term in Plasma::QueryMatch \
and then check that it matches the last query before actually running the item. A \
patch for kdelibs will follow.

In this way, I believe that m_queryRunning becomes irrelevant, but I still have to \
think about it for some more.

I tried to follow all guidelines for binary compatibility in kdelibs, but please \
double check that. Also I think I can add the declaration of a new method wherever I \
want, but just to be sure I put them at the end. If you confirm that their position \
won't matter I'll put them in a more consistent manner. 

--J


This addresses bugs 169283, et and similia.
    https://bugs.kde.org/show_bug.cgi?id=169283
    https://bugs.kde.org/show_bug.cgi?id=et
    https://bugs.kde.org/show_bug.cgi?id=similia


Diffs (updated)
-----

  /branches/KDE/4.2/kdebase/workspace/krunner/interfaces/default/interface.cpp 931111 \


Diff: http://reviewboard.kde.org/r/172/diff


Testing
-------

The patch appears to solve the bug.


Thanks,

wilder

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel


[prev in list] [next in list] [prev in thread] [next in thread]