Bugzilla – Full Text Bug Listing
| Summary: | [Dialogs] clearing filter in FilteredTree does not collapse nodes | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Steffen Pingel <steffen.pingel> | ||||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | daniel_megert | ||||||||
| Version: | 3.7 | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | stalebug | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 323525 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Steffen Pingel
Created attachment 181819 [details]
potential fix
The patch changes the behavior in the following way:
* FilteredTree.textChanged() only triggers a refresh if the text actually changes.
* When the filter is cleared only nodes that do not have selected children are collapsed to retain selections on clear.
Does it keep the expanded nodes if one of them is selected? PW Yes, nodes that are selected or have selected children are kept expanded. Has anyone had a chance to look at the patch? I would be happy to make adjustments if necessary. Ping. It's a fairly trivial patch and the steps to reproduce are simple. Any hope to get this fixed for 3.7? yep, sorry Steffen. I've had this in my inbox as a to-do for some time now. Will take a look when the M6 fun is done. Created attachment 193527 [details]
refined patch
This patch refines the original patch in the following ways:
- get rid of redundant call to getFilterString() now that we cache it in the method
- do not preserve the expansion state of a root node if none of the children are selected. (The original patch would preserve expansion even if the root were selected. This one ensures it is preserved only if a child is selected).
I spent a fair bit of time writing an automated test case for the bug (and this expansion variant) but due to the nature of the refresh job, I'm not able to run it automatically (I can prove through debug that the cases work, but not in an automated fashion, and joining on the refresh job in the test case hung the test suite).
I've verified the original problem, and that this fixes it. The original FilteredTree tests still pass (though they are fairly trivial so this doesn't give any added confidence). I have a test case which sets and unsets the filter and counts expansions, and I can step through it, but it can't run in an automated fashion due to the refresh job delay (and joining it hung the suites). That test found the bug in the original patch, but I can't add it to the suite. My only concern with this fix is that we are fixing it in a protected method and it's possible (but unlikely) that subclasses rely on the old behavior. However, this fix seems reasonable to me. I'm cc'ing Dani for comment because he did the "new look" work with the clear button and may be able to think of some additional corner cases I should test before releasing this patch. The filtered tree has that bug since its beginning (3.1). Touching it 4 days before RC0 candidate build seems too risky to me, especially because it not only touches the "clear" scenario/code. The patch changes textChanged() so that it no longer refreshes in certain cases. This can break subclasses that expect an unconditional refresh when calling that method. Having said that, I think the real problem is that textChanged() is called two times when the clear button is clicked. Removing "textChanged();" from clearText() should do the trick. (In reply to comment #9) > The filtered tree has that bug since its beginning (3.1). Touching it 4 days > before RC0 candidate build seems too risky to me, especially because it not > only touches the "clear" scenario/code. The patch changes textChanged() so that > it no longer refreshes in certain cases. This can break subclasses that expect > an unconditional refresh when calling that method. That was my concern, too. (Sorry Steffen that I waited so long to look at this.) > > Having said that, I think the real problem is that textChanged() is called two > times when the clear button is clicked. Removing "textChanged();" from > clearText() should do the trick. Right, it is called via the modify event (but a modify event typically is fired only when the text is actually modified) and then again by clearText(). So the primary risk to this solution is that a subclass may have relied on textChanged to fire on clearText() even if the text was already empty. The problem with this fix is that although it fixes the problem as reported here by Steffen (clearing filter does not collapse nodes), it does not preserve the selection when nodes collapse, which I think was the original bug reported against Mylyn. So the question is...does this fix help you, Steffen? If so, I would recommend we fix it as suggested by Dani. I will post a new patch for consideration. Created attachment 193719 [details]
patch which collapses on clear but does not keep selections open
> So the primary risk to this solution is that a subclass may have relied on
> textChanged to fire on clearText() even if the text was already empty.
Yep, but that risk way smaller than the other patch and it is restricted to only that scenario.
will look at this for 3.8. The open question is whether it's enough to fix the clear/collapse problem (which the latest fix addresses) or if this bug is also requesting to keep nodes open which have selections (the original patch). This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |