Bug 229735 - [ui] Strange behavior of check boxes when filtering available software view
Summary: [ui] Strange behavior of check boxes when filtering available software view
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4 RC1   Edit
Assignee: Susan McCourt CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 231306 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-04-30 18:54 EDT by Susan McCourt CLA
Modified: 2008-06-04 14:05 EDT (History)
6 users (show)

See Also:
john.arthorne: review+


Attachments
patch to ContainerCheckedTreeViewer and DeferredFetchFilteredTree (13.83 KB, patch)
2008-05-13 18:50 EDT, Susan McCourt CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Susan McCourt CLA 2008-04-30 18:54:26 EDT
+++ This bug was initially created as a clone of Bug #229305 +++

I'm opening a separate bug to track all the check box oddities associated with filtering in the available IU view.  The original bug was fixed, in that we no longer pass bogus selections to the install when we are filtered.

However there are still a number of weird problems involving the selection state itself:

1 - odd change of selection state when opening install dialog
a - filter so that only releng tools is showing
b - check mark releng tools
c - push install
d - install dialog comes up with releng tools in it (good)
e - but as a result of something in that dialog (probably the sizing operation?) the releng tools becomes unchecked.

2 - odd selections when unfiltering releng tools
a - filter so that only releng tools is showing
b - check mark releng tools
c - now backspace in the filter so that more items will show
d - the items are all checked (weird, but...okay, I'll deal with that)
e - push the install button again, still only releng tools is in the wizard (ummm...that is what I would have wanted, but I would have not wanted to see everything else checked).

I'm sure there are others.
The bottom line (I think after a lot of exploration) is that when ContainerCheckedTreeViewer is trying to preserve the selections, it remembers the selected state of the elements, and then tries to restore them.  But often the TreeItems in the tree are not yet expanded and associated with their actual elements, so some selections are not preserved.

The dependent bugs describe other known problems with this viewer.
Comment 1 Susan McCourt CLA 2008-04-30 18:55:31 EDT
I plan to examine this a bit in RC1 to see what workarounds we can do.  My initial attempts (and there were quite a few) failed so I took them out.
Comment 2 Susan McCourt CLA 2008-05-09 12:22:17 EDT
*** Bug 231306 has been marked as a duplicate of this bug. ***
Comment 3 Susan McCourt CLA 2008-05-09 12:23:14 EDT
From the duplicate bug:
1. Go to help, Software updates, Available Software
2. for a valid site, select a number of items
3. enter some search text in the filter textbox
4. check that the previously slected items are not selected anymore
5. Remove filter text
6. Tree shows site as containing selections, but no item as selected
Comment 4 Susan McCourt CLA 2008-05-09 12:25:30 EDT
fyi, I've been struggling with this one on and off for a few days.  

The intended behavior is that any leaf nodes that are selected will remain selected through a filter or unfilter, and the status of parent nodes will be recomputed based on the leaves.

The latest use case is interesting because even if the checkbox viewer did not have bug #170521, I think it would take special handling to ensure that filtered items remained selected.  
Comment 5 Susan McCourt CLA 2008-05-13 12:03:03 EDT
>The latest use case is interesting because even if the checkbox viewer did not
>have bug #170521, I think it would take special handling to ensure that
>filtered items remained selected.  

PDE handles this with its own tree viewer that caches check state of all items.  (See bug #207064).  I'm going to annotate bug #170521 with that info, as this might be the basis for a platform UI solution in the future.
Comment 6 Susan McCourt CLA 2008-05-13 18:50:40 EDT
Created attachment 100078 [details]
patch to ContainerCheckedTreeViewer and DeferredFetchFilteredTree

Proposed patch.
Comment 7 Susan McCourt CLA 2008-05-13 19:09:42 EDT
I had to give up on a solution that could be contained to the viewer itself and instead propose a more surgical patch that alters the filter job for our use case.

John, can you please review this patch?
It does the following:
1) modifies our copy of ContainerCheckedTreeViewer to record the check state of the tree before a refresh and restore it afterward.  Resetting selections after a refresh is generally how clients have worked around bug #17907, and I put the code in internalRefresh to catch most cases.  This solves the problem that the wrong checkmarks are preserved over a refresh, but does not solve the general problem that a filtered check box tree doesn't bother remembering check marks that are out of the filter.  I also corrected some comments that were no longer valid, so the changes look bigger than they are.

2) modifies DeferredFetchFilteredTree to copy/alter the superclass' filter job.  In the filter job we do an additive snapshot of selections before any refreshes.  This solves the more general problem that selections don't get remembered when an item is filtered out of the list.  The job also fully expands the tree when refreshing to ensure that the correct elements are in the tree items so that the snapshot restore works properly, rather than recursively expanding the tree and bailing out of the job when the timeout is reached.  This is a performance tradeoff for correctness, and always knowing we have restored the checkmarks completely.

This seems to correct most of the cases involving the checkmarks not matching the selections, remembering of checkmarks across multiple filters, etc.  It does not correct the very first glitch reported in this bug, so I've opened bug #231768 to track that one, which is not so important.

With the patch, selections are preserved correctly across random refreshes.  They are also preserved across filtering, with the caveat that we don't consider non-visible selections if you click a button such as Install...

Example (you must be connected to Ganymede update site to follow the example).  
- type "CVS" 
- check "Eclipse CVS Client" and "Mylyn Bridge PDE, CVS, and Ant"
- now type "AJ" on top of the CVS to replace the filter text
- check "Rich Ajax Platform"
- clear the filter (using the button or by clearing the text)
- press Install...
- All three items will be chosen for install.
- Type "Aj" again.
- Press Install... 
- now only ajax is chosen (because you can't see the others)
- clear the filter and press install...
- now you get them all again.

I also tried changing the view (by site->flat, flat->category) and the selections were preserved across these refreshes that are unrelated to filtering.

I also did some add/remove repo and the selections were preserved.

I think this is a huge improvement and while it's more code than I typically would want to insert into an RC1, it is at least contained and straightforward.

Comment 8 John Arthorne CLA 2008-05-14 17:43:06 EDT
Looks good. I confess I couldn't make it break no matter what I tried. 

The one issue I kept hitting was the change of focus causing mnemonics on other buttons to be hit ('a' for add site, etc). However that is captured in another bug.
Comment 9 Susan McCourt CLA 2008-05-14 18:20:54 EDT
thanks, John.  
Marking fixed and removing bug dependencies since this was worked around.
Fixed in HEAD >20080514, should be in tonight's I build.

For those watching this bug who want to try to break it, the remaining checkbox/filter bugs we already know about are:
bug 231998 - the mnemonic problem John mentioned.  Planned for 3.4, hopefully RC1.
bug 232112 - the first checkmark on an unloaded site will expand/load it but not check it.  This won't be fixed in 3.4.
bug 231768 - opening an install wizard will affect the visible check state of the item (but not the real check state).  This won't be fixed in 3.4, but should disappear by virtue of us closing the dialog when the install wizard comes up (bug  231590).

Comment 10 Susan McCourt CLA 2008-06-04 14:05:22 EDT
I referred to the wrong bug number in my summary.  Changing here and also adding the bug number for the other issue mentioned:

As of 3.4 RC4, the remaining checkbox/filter bugs we already know about are:

bug 232112 - the first checkmark on an unloaded site will expand/load it but
not check it.  This won't be fixed in 3.4.

bug 231968 - opening an install wizard will affect the visible check state of
the item (but not the real check state).  This won't be fixed in 3.4, but
should disappear by virtue of us closing the dialog when the install wizard
comes up (bug  231590).

bug 235654 - items selected but filtered out of the current view will not be considered part of the selection.  Clearing the filter will restore them.  This is a side-effect of the implementation (the fact that the viewer itself does not keep track of selections filtered out of view).