Bug 91476 - KListViewSearchLine can sometimes show items it shouldn't
Summary: KListViewSearchLine can sometimes show items it shouldn't
Status: RESOLVED FIXED
Alias: None
Product: kdelibs
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Scott Wheeler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-10-16 21:58 UTC by Richard Smith
Modified: 2005-07-12 01:05 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Smith 2004-10-16 21:58:25 UTC
Version:            (using KDE Devel)
Installed from:    Compiled sources

Take a nested message thread in KMail. Find a search string which applies to the message at the start of the thread, but none of the rest of the messages in it.
Enter that string, and enough random text to hide all messages. Now delete the random text, so that message comes back.
All its children appear too. They should not.

The reason for this is a bug here:

bool KListViewSearchLine::checkItemParentsVisible(QListViewItem *item)
{
    bool visible = false;
    for(; item; item = item->nextSibling()) {
        if((item->firstChild() && checkItemParentsVisible(item->firstChild())) ||
           itemMatches(item, d->search))
        {
            item->setVisible( true );
            visible = true;
        }
        else
            item->setVisible(false);
    }
    return visible;
}

The trouble is, calling item->setVisible( true ); sets all the children of that item to be visible too. Since the above recursive function shows/hides the children before it processes the parent, the children that should be hidden end up not being hidden.

In my case (in Kopete, which has the above code copy-pasted for other reasons), I put another call to checkItemParentsVisible(...) after the setVisible( true ), and I now get the expected results, but this makes the code exponential-time (at least in theory). It can still be done in linear time, but it requires a two-pass approach, first deciding on item visibility recursively, then setting item visibility recursively.

I'd fix it myself, but I've got a lot on my plate at the moment. If you need clarification on anything, just ask.
Comment 1 Daniel Teske 2005-07-12 01:04:59 UTC
SVN commit 433828 by teske:

KListViewSearchLine didn't unhide the right items.
Fix that with a new algorithm.
Thanks to Richard Smith for his help. He reviewed a earlier patch and
ensured that the algorithm is alot more readable.

BUG: 91476




 M  +38 -5     klistviewsearchline.cpp  
 M  +1 -1      klistviewsearchline.h  


--- trunk/KDE/kdelibs/kdeui/klistviewsearchline.cpp #433827:433828
@@ -353,15 +353,48 @@
     }
 }
 
-bool KListViewSearchLine::checkItemParentsVisible(QListViewItem *item)
+#include <kdebug.h>
+
+/** Check whether \p item, its siblings and their descendents should be shown. Show or hide the items as necessary.
+ *
+ *  \p item  The list view item to start showing / hiding items at. Typically, this is the first child of another item, or the
+ *              the first child of the list view.
+ *  \p highestHiddenParent  The highest (closest to root) ancestor of \p item which is hidden. If 0, all parents of
+ *                           \p item must be visible.
+ *  \return \c true if an item which should be visible is found, \c false if all items found should be hidden. If this function
+ *             returns true and \p highestHiddenParent was not 0, highestHiddenParent will have been shown.
+ */
+bool KListViewSearchLine::checkItemParentsVisible(QListViewItem *item, QListViewItem *highestHiddenParent)
 {
     bool visible = false;
-    for(; item; item = item->nextSibling()) {
-        if((item->firstChild() && checkItemParentsVisible(item->firstChild())) ||
-           itemMatches(item, d->search))
+    QListViewItem * first = item;
+    for(; item; item = item->nextSibling())
+    {
+        //What we pass to our children as highestHiddenParent:
+        QListViewItem * hhp = highestHiddenParent ? highestHiddenParent : item->isVisible() ? 0L : item;
+        bool childMatch = false;
+        if(item->firstChild() && checkItemParentsVisible(item->firstChild(), hhp))
+            childMatch = true;
+        // Should this item be shown? It should if any children should be, or if it matches.
+        if(childMatch || itemMatches(item, d->search))
         {
-            item->setVisible( true );
             visible = true;
+            if (highestHiddenParent)
+            {
+                highestHiddenParent->setVisible(true);
+                // Calling setVisible on our ancestor will unhide all its descendents. Hide the ones
+                // before us that should not be shown.
+                for(QListViewItem *hide = first; hide != item; hide = hide->nextSibling())
+                    hide->setVisible(false);
+                highestHiddenParent = 0;
+                // If we matched, than none of our children matched, yet the setVisible() call on our
+                // ancestor unhid them, undo the damage:
+                if(!childMatch)
+                    for(QListViewItem *hide = item->firstChild(); hide; hide = hide->nextSibling())
+                        hide->setVisible(false);
+            }
+            else
+                item->setVisible(true);
         }
         else
             item->setVisible(false);
--- trunk/KDE/kdelibs/kdeui/klistviewsearchline.h #433827:433828
@@ -190,7 +190,7 @@
      * It makes a recursive call to all children.  It returns true if at least
      * one item in the subtree with the given root item is visible.
      */
-    bool checkItemParentsVisible(QListViewItem *item);
+    bool checkItemParentsVisible(QListViewItem *item, QListViewItem *highestHiddenParent = 0);
 
 private slots:
     void itemAdded(QListViewItem *item) const;