Community
Participate
Working Groups
If you open the Find dialog (^F) and type a string and hit return it searches forwards (assuming you have the Forwards option checked). This is a request to make it search *backwards* if you are holding the shift key when you hit return. That way if you're like me and hit return too many times you can shift- return to back up to the one you want. For precedents see TextPad and VisualStudio.
Marking as remind. Will be activated when bug 21464 is fixed.
What's the status on this one, can it be reopened? I don't quite understand the relationship with bug 21464, is it something to do with the SHIFT mask not being available to distinguish ENTER from SHIFT+ENTER? The OS should provide it, at least on Win32, because the two other editors I mention supported it.
The relationship is that the modifier is not set (see bug 21464).
This bug really depends on bug 65679. Bug 21464 is specifically about the detail in the SelectionEvent of a scrollbar on GTK+.
Get rid of deprecated state.
.
Why is this invalid? Shift+Enter can easily be detected in the Combo by listening for the Traverse event. Shift+Click on "Find" button to search backwards is not easy to implement due to bug 65679, but that's not requested in this bug.
Sorry but bug 65679 is exactly describing the problem: the stateMask is not set no matter whether I click the button or invoke it via pressing the Return key (or whatever triggers the selection event on the default button - this can be platform dependent). I won't work around this. Your suggestion is handling one specific case i.e. when the selection is still on the text field. It would be pretty unexpected that it doesn't work when the selection is on a checkbox or anywhere else and I won't add a listener to every control in the dialog.
Or we get bug 71538 which allows me to query the state myself.
Created attachment 176772 [details] Patch contains fix and test
>Created an attachment (id=176772) [details] [diff] >Patch contains fix and test Initial comments on the patch can be found in bug 319560. Here some more comments after detailed review and testing: Looking at the code it looks like incremental search is broken now: it always searches backwards. Testing confirms that. Since we also use 'Shift' on the 'Find' button we also have to do the same for 'Replace/Find'. This is currently not the case. The test works but it is a very fragile test since it relies on events. If you that that you need to add more safety to it. E.g. clicking on the host workspace after lunching the test lets it fail. Also, the new code uses spaces for indentation at some places. Please always use tabs for indentation. >@param forwardSearch the search direction It's usually good to use/copy the same pattern as already done in the file. However, for boolean parameters we now always explicitly mention true/false using the following pattern: @param forwardSearch <code>true</code> if searching forwards, <code>false</code> otherwise >e.stateMask == SWT.SHIFT) ? !isForwardSearch() : isForwardSearch() Less verbose: e.stateMask == SWT.SHIFT ^ isForwardSearch() Also note that there should be an empty line before the Javadoc parameters.
Created attachment 176845 [details] Patch 27996 (In reply to comment #11) > >Created an attachment (id=176772) [details] [details] [diff] > >Patch contains fix and test > Initial comments on the patch can be found in bug 319560. Here some more > comments after detailed review and testing: > > Looking at the code it looks like incremental search is broken now: it always > searches backwards. Testing confirms that. The last change to use 'forwardSearch' as parameter name caused this. My bad. Fixed. > > Since we also use 'Shift' on the 'Find' button we also have to do the same for > 'Replace/Find'. This is currently not the case. Implemented. > > The test works but it is a very fragile test since it relies on events. If you > that that you need to add more safety to it. E.g. clicking on the host > workspace after lunching the test lets it fail. Also, the new code uses spaces > for indentation at some places. Please always use tabs for indentation. While, forcing focus before generating events did improve reliability, it still didn't seem good enough. Replaced posting of events with direct calls. > > >@param forwardSearch the search direction > It's usually good to use/copy the same pattern as already done in the file. > However, for boolean parameters we now always explicitly mention true/false > using the following pattern: > @param forwardSearch <code>true</code> if searching forwards, > <code>false</code> otherwise Done. > > >e.stateMask == SWT.SHIFT) ? !isForwardSearch() : isForwardSearch() > Less verbose: e.stateMask == SWT.SHIFT ^ isForwardSearch() neat idea. > > Also note that there should be an empty line before the Javadoc parameters.
> > Also note that there should be an empty line before the Javadoc parameters. Not done ;-) Committed runtime code to HEAD with above fix. Test: ===== Works fine. There are some minor issues: 1. setUp/tearDown runs for each test. Since we only use the big setup for one test, we should not punish the other tests with additional work. 2. When creating UI stuff, you should always close or dispose them when no longer needed. In your patch all viewers accumulate until the whole workbench goes down. >private Display display; Not using 'f' prefix for fields, but the field is overkill here anyway. >Please always use tabs for indentation. Not done. Next patch only needs test code.
Created attachment 177028 [details] Patch contains test only
Committed tests to HEAD. Rajesh, there was one minor issue: if someone adds a new test that doesn't need to open the Find/Replace dialog (or closes it in its tests), then it would result in an NPE during tear down.
The test failed on GTK because - shells activation events are only sent when the event loop is run, so the dialog shell was always null - bug 323272 in SWT (traverse event notification too late) I've released workarounds for this to HEAD. Sorry Dani, I've accidentally used your connection on the build machine.
Verified for 3.7M2 on Linux with I20100914-0100.
