Bugzilla – Full Text Bug Listing
| Summary: | Abandon the combo box of preferenceDialog | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | shujie liao <shujie_liao> |
| Component: | UI | Assignee: | Karice McIntyre <Karice_McIntyre> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | douglas.pollock, gunnar, michaelvanmeekeren, pwebster, susan, thorbjoern, Tod_Creasey |
| Version: | 3.1 | ||
| Target Milestone: | 3.2 M3 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
shujie liao
Created attachment 24326 [details]
A patch for 102081
Apply from:
org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/dialogs
Created attachment 24383 [details]
A patch for 102081
Apply against :
org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/dialogs/
Second patch released Some comments on this patch: 1) open the preferences dialog - type in "a" - press ok - return to the dialog and now 'a' is in the list but it was not a successful search 2) enter a search "key", hit enter to cause "key" to be in the history - now go back to the text and type 'k', arrow down to the list and select "key" pressing Enter now should select this NOTE: as I arrow through the list the text in the text widget should not change until i press enter, if I hit ESC, the drop down list is closed 3) using arrow down after putting the cursor to the far left of some text causes an exception, see below ---------- SWT Exception ------------- org.eclipse.swt.SWTException: Widget is disposed at org.eclipse.swt.SWT.error(SWT.java:2942) at org.eclipse.swt.SWT.error(SWT.java(Inlined Compiled Code)) at org.eclipse.swt.SWT.error(SWT.java(Inlined Compiled Code)) at org.eclipse.swt.widgets.Widget.error(Widget.java(Inlined Compiled Code)) at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java(Compiled Code)) at org.eclipse.swt.widgets.Control.isVisible(Control.java:1327) at org.eclipse.ui.internal.dialogs.FilteredTextTree$2.keyReleased(FilteredTextTree.java:118) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:128) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java(Compiled Code)) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java(Inlined Compiled Code)) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java(Compiled Code)) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:852) at org.eclipse.swt.widgets.Widget.sendKeyEvent(Widget.java:880) at org.eclipse.swt.widgets.Text.sendKeyEvent(Text.java:1154) at org.eclipse.swt.widgets.Widget.sendKeyEvent(Widget.java:876) at org.eclipse.swt.widgets.Widget.wmKeyUp(Widget.java:1575) at org.eclipse.swt.widgets.Control.WM_KEYUP(Control.java(Inlined Compiled Code)) at org.eclipse.swt.widgets.Control.windowProc(Control.java(Compiled Code)) at org.eclipse.swt.widgets.Text.windowProc(Text.java(Compiled Code)) at org.eclipse.swt.widgets.Display.windowProc(Display.java(Compiled Code)) at org.eclipse.swt.internal.win32.OS.DispatchMessageW(Native Method) at org.eclipse.swt.internal.win32.OS.DispatchMessage(OS.java(Inlined Compiled Code)) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java(Compiled Code)) at org.eclipse.jface.window.Window.runEventLoop(Window.java(Compiled Code)) at org.eclipse.jface.window.Window.open(Window.java:787) at org.eclipse.ui.internal.OpenPreferencesAction.run(OpenPreferencesAction.java:66) at org.eclipse.jface.action.Action.runWithEvent(Action.java:996) at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:538) at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:488) at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:400) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java(Compiled Code)) ------- end of Exception ---------- 4) It appears that the filtered search in the Colors and Fonts page no longer works. 5) when the text receives focus it should select all text (same as the default behaviour provided by the combo previously), unless the focus is given by clicking in the text with the mouse in which case the cursor should be placed at the location of the click and no selection made. 6) the text matching should not be case sensitive for the drop down list 7) the sizing of the list is not quite right. It should always try to show all text in the horizonal space if possible and make the drop down be some maximum length (7) but this should be configurable. 8) see bug 102911, the popup should always stay on the screen Created attachment 24783 [details] A patch for 102081 Apply against: org.eclipse.ui.workbench Fixed all problems except problem 5) in comment 4,it looks that when the focus gained by mouse down, method "selectAll()" in Text area has kind of conflict with the way text works. Created attachment 25296 [details] A patch based on comment 4 Problem 3 can be replicated 1) Pull down the list by typing something 2) arrow into the list 3) Hit esc 4) hit down arrow This patch fixes 1,2,4,6,8 Created attachment 25430 [details] A patch to fix problem 3, 5 and 7 in comment 4 Run the patch against: org.eclipse.ui.workbench Created attachment 25584 [details] A patch to fix problem 3, 5 and 7 in comment 4 Run the patch against: org.eclipse.ui.workbench Patch released for build >20050802 I would like to recommend a rollback of this bug for 3.2 M1. Bug 106165 is not trivial to fix, and seems to be quite bad on Linux. The fix has been rolled back for I20050808-1200. I propose we take another crack at this. How about: step 1 - we start by removing the combo in favour of a text step 2 - add support for an optional dropdown with completions Karice this is the bug I mentioned with the code to replace the combo with a text box in the filtered list Created attachment 27648 [details]
apply patch to org.eclipse.ui.workbench
This patch changes the combo to a text in the preferences dialog and the show
view dialog. Micheal, can you take a look at this patch and let me know if
this is what you had in mind?
there seems to be a missing FilteredTextTree class in this patch oops... the patch did not apply properly... let me try again Created attachment 27653 [details]
apply to org.eclipse.ui.workbench
Try this patch. I didn't make any changes to the generated patch file.
upping priority since this is part of an M3 plan item. Created attachment 27838 [details]
apply patch to org.eclipse.ui.workbench
Some refinements to my original patch (attachments 27648 and 27653). This
should select the first relevant selectable element in the tree, fixed a
scrolling problem, and general re-factoring. Still need to integrate with
filtered text on New Keys preference page (it has a grouping combo).
Oops, I was supposed to make this a P2 in comment #20, not a P1. Created attachment 27868 [details]
Reworked patch due to apply problems
Doug, can you check this patch? Use the one Tod attached. Created attachment 27957 [details]
apply patch to org.eclipse.ui.workbench
Added bold label provider to show view dialog
Created attachment 27967 [details]
NullPointerException trace
The most recent patch causes this NPE when hitting enter to close the dialog.
Created attachment 27979 [details]
apply patch to org.eclipse.ui.workbench
I couldn't actually replicate the NPE but I think I found/fixed the problem.
Unless you can replicate the problem in the Show View dialog.
Sorry this took so long, but this is a large section of code. I found a few problems, but in general it's good. I'm not sure that all of them have to be fixed before committing this code, but at the very least bugs should be opened to track any issues that won't be fixed immediately. General Comment + Code should be formatted. New classes should be entirely formatted. Old classes should have the sections that have been modified formatted. FilteredTree + Why is the KeyAdapater responding to keyReleased and not keyPressed? + textChanged is being called even if the key pressed was an arrow_down. This results is an extraneous scheduling of the refresh job. + The traverse listener clears the text field if there are no matches. Is this intentional? + I believe there is a race condition between the refresh job and the effects of pressing enter. It's possible that the refresh job has not yet run (in response to filter text changes) when the "enter" key is handled by the traverse listener. + getElementFont should probably describe what the parameters are PatternFilter + isElementMatch is missing some javadoc and is a bit rough (e.g., the double blind casting) PreferencePatternFilter + It is possible for an IPreferenceNode to return null from getLabelText, which can lead to an NPE as wordMatches(String) does not handle null. This problem occurs twice. mvm - I'm looking at step 2 in comment #14 (optional dropdown with completions) as a use case for my work on task assistance in text fields (bug #106199). My question is - is the intention that the dropdown contain a list of the previous filters that have been used (as the combo did before), or is the intention that the dropdown contain the currently filtered list of possibilities so that the user can navigate quickly to the item of interest (such as the type-ahead mentioned in comment #0)? I don't mean to throw a wrench in this, but I've got some general questions/concerns about this approach for the Show View dialog (not Karice's code, but the UI intent). 1) In the preference dialog, filtering the list can be useful for reducing the tree and then browsing the remaining elements in the tree to find just the preference you want. But in ShowView there is nothing to "browse" when you click on the view name, so I'm not sure all the noise of bolded expressions in the filtered tree is a good model for selecting the view you want. And if the view category matches the filter, but none of its views match the filter, should it be shown at all? There's nothing to choose or see if you select it (unlike preferences where there is page associated with each element in the tree). In this case, it seems like a content proposal dropdown should show all views that match the filter and you could just select from there, without filtering the tree. The tree is still there if you are truly browsing the categories to see what's available in general, but I'm not even sure that filtering it is useful if you have the proposals in a popup. 2) I don't think we should assume that content proposal is a simple replacement for a combo. For example, the text find/replace dialog provides both a combo for showing previous search expressions, as well as content assist for completing/inserting special expressions in the current field. In both the preferences and show view case, is the intent of content proposal to show you things you've typed in there before, or to show you the list of results that match the filter? I think I prefer the latter. If you really wanted to remember previous filters, you could keep the combo. In a type- ahead completion model, the proposals could show all preference page/preference node names (or view names) that match the filter, and you could select directly from the proposals. You could still filter the tree in the preferences to support browsing reduced choices. But in ShowView there is nothing to "browse" when you click on the view name, so I'm not sure all the noise of bolded expressions in the filtered tree is a good model for selecting the view you want. > agree, it is overkill to bold in the show view scenario as the hierarchy is not deep And if the view category matches the filter, but none of its views match the filter, should it be shown at all? > no, this is a bug In this case, it seems like a content proposal dropdown should show all views that match the filter and you could just select from there, without filtering the tree. > we tried this with preferences and it led to some success, in the end we just kept it simpler by modifying the actual list, I think duplicating the content in a drop down makes sense when the amount of content is so huge that you can't possibly update it in realtime. The tree is still there if you are truly browsing the categories to see what's available in general, but I'm not even sure that filtering it is useful if you have the proposals in a popup. > see above, I think in the end what the user gets is similar 2) I don't think we should assume that content proposal is a simple replacement for a combo. > agree, the combo also implies you want to remember how you did something before, i.e. you have a clearer sense of what you might want to do again. For example, the text find/replace dialog provides both a combo for showing previous search expressions, as well as content assist for completing/inserting special expressions in the current field. In both the preferences and show view case, is the intent of content proposal to show you things you've typed in there before, or to show you the list of results that match the filter? > no surprise that preferences are having the combo removed because it does not make sense to show previous entries (which is what it is intended to do) > I believe that in general the intent could be to do either case, but for both prefs. and show view that is not necessary as the scenario is just not complex enough... others might be .. like where you have the Keys preference page allowing a user to filter on 100s of commands. I think I prefer the latter. If you really wanted to remember previous filters, you could keep the combo. In a type- ahead completion model, the proposals could show all preference page/preference node names (or view names) that match the filter, and you could select directly from the proposals. You could still filter the tree in the preferences to support browsing reduced choices. > see above, I'd vote we start by not supporting proposals in either of those two cases to begin with, proposals will make more sense in a wizard where you're entering a package name for example for a class... oh. and I just read comment #29 , I guess the answer is that we could put them in the preferences or show view dialog as completions on the existing items which could then be selected, and NOT filter the list in that case, but I'd rather see a popup used in a more sensible place... how about the new file dialog where you are entering a workspace path for the parent folder of the file? We could do completion based on the folders... much like many save/open dialogs do. Thanks for the explanation. I won't worry about these scenarios anymore for my text field/assistance work. I'll crawl back to bug #106199 and look for other interesting use cases, such as those you mention. Created attachment 28401 [details] apply patch to orgeclipse.ui.workbench This patch addresses the feedback from comment #28, except for the race condition. Also the bold fonts in the show view dialog were removed. The bug where a matching category appears but has no children in the show view dialog was not yet addressed. Doug, can you take a look at the latest patch? Paul: Can you look at Karice's patch? The patch looks good ... PreferencePatternFilter and ViewPatternFilter need to have the IBM copyright added to the front ... it should be ready to go once we add those. Additional comments: It looks like the NPE was fixed, also from comment #28: + I believe there is a race condition between the refresh job and the effects of pressing enter. It's possible that the refresh job has not yet run (in response to filter text changes) when the "enter" key is handled by the traverse listener. We should open a new bug to track this ... should the traverse listener cancel any outstanding refresh jobs? + isElementMatch is missing some javadoc and is a bit rough (e.g., the double blind casting) The double blind casting is still there, possibly updated if this class needs to be refactored to support different content types? Later, PW Created attachment 28603 [details]
apply patch to org.eclipse.ui.workbench
Added copyrights.
As for the double-blind casting in PatternFilter.java, since we are only using
this class for filtering trees it should be fine for now. Note this code was
present in the orignal version of the class (Apr 2004), this patch just moved
it to another method.
Remaining issues: investigate race condition, make filtered tree work with
categories (example, experimental Keys preference page), identify other useful
places to use a filtered tree.
Released in HEAD >20051021 PW bug 113908 was opened to address the remaining issues. verified in 20051101 *** Bug 87901 has been marked as a duplicate of this bug. *** |