Bug 319560

Comment 1 Rajesh 2010-08-11 05:22:27 EDT

Created attachment 176311 [details]
Patch for 319560 & 27996

Based on the original comment in the bug, here is what this patch implements -

- Pressing Shift-Enter does the search in the reverse direction from the selected direction (Bug 27996)
- Pressing Enter or Shift-Enter while focus is in Find, Replace fields or on any other control in the dialog will not change the focus to the Find button
- Pressing the mnemonic key for any of the buttons will not change the focus to the button
- Clicking the Replace/Replace All buttons does not change focus to Find button
- All calls to setFocus (except during initialization) to specific fields have been removed and focus mgmt within button section has been fixed.
- Tests for all of the above.

Comment 2 Dani Megert 2010-08-12 11:48:09 EDT

Thanks for the patch Rajesh!

Works quite nicely except for the tests which don't compile: you added references (imports) to things which are not present at that level.

Also, each bug should get its own patch (e.g. in case we have to revert one of them at a later point or we decide to backport one of them).

Some feedback to the code itself:
- calls to extractAndStoreButtonMnemonic should be inside makeButton (less code)
- if (e.stateMask == SWT.SHIFT) else ==> directly use the condition as argument
- rename new parameter searchDirectionModifier ==> forwardSearch, that makes
  the methods more generic
- why did you remove updateButtonState() from performReplaceSelection()?
- @since tag is needed on all new members
- Copyright date needs to be updated

Comment 3 Markus Keller 2010-08-12 13:10:53 EDT

The mnemonics test will most probably fail on the Mac, since that platform doesn't support mnemonics.

org.eclipse.jface.util.Util.isWindows() and isLinux() are your friends.

Comment 4 Rajesh 2010-08-13 00:55:47 EDT

(In reply to comment #2)

Dani/Markus - Thanks for the comments. Made all the changes suggested (I think) and attached. Couple of comments below -

> - rename new parameter searchDirectionModifier ==> forwardSearch, that makes
>   the methods more generic

In this case, if we name it forwardSearch, it could be misleading as even when its value is 'true' the actual search could be backwards. Though I do agree with your generic method principle.

> - why did you remove updateButtonState() from performReplaceSelection()?

When the Replace/Find button is clicked, performReplaceSelection first does the replace after which there is no current selection so the call to updateButtonState causes Replace/Find to be disabled causing focus to be shifted to the default which is the Find button. This scenario was being countered in the 3.6 code by calling an explicit setFocus back to Replace/Find. Now setFocus had to be removed so that focus wouldn't shift to Replace/Find while handling its mnemonic. There are 3 ways I could see this being handled :
- updateButtonState should be called in the Replace/Find selection handler after the PerformSearch and not in the performReplaceSelection.
- Pass a flag to performReplaceSelection to let it know if it should updateButtonState. This way all the perform methods do updateButtonState. 
- The Replace/Find selection handler needs to know if its a mnemonic event and also where the focus is currently. This seemed unnecessary.

I will separate the patches later today.

Comment 5 Rajesh 2010-08-13 01:30:13 EDT

Created attachment 176519 [details]
Patch2 - 319560 & 27996

Comment 6 Dani Megert 2010-08-13 02:38:20 EDT

The workbench.text layer must neither have a dependency on file buffers nor on IDE and hence we also follow that rule in the tests. Only at the ui.editors layer we can start to use file buffers and IDE stuff.

>In this case, if we name it forwardSearch, it could be misleading as even when
>its value is 'true' the actual search could be backwards.
It depends what you pass in: you could pass (!)isForwardSearch() into the perform method. I would also add performSearch(boolean forwardSearch) where performSearch() calls performSearch(isForwardSearch());.

Something else:
- I see that e.detail= SWT.TRAVERSE_NONE is needed to stop further mnemonic traversal but I would also set e.doit to false. Any reason why you set it to true?
- char mnemonic; is defined outside its scope. But actually okToUse(button) is not needed there at all since we're in the middle of creating the dialog and hence it's definitely OK to use the button.					


I'm not sure Markus's comment is valid since I would expect that on a machine (like Mac) where mnemonics aren't supported, we don't get TRAVERSE_MNEMONIC events.

Comment 7 Markus Keller 2010-08-13 05:37:39 EDT

> I'm not sure Markus's comment is valid since I would expect that on a machine
> (like Mac) where mnemonics aren't supported, we don't get TRAVERSE_MNEMONIC
> events.

Sorry, my comment wasn't too clear. I was only referring to the test case. The business code should indeed not be platform-dependent (you will just never get a mnemonic traversal event on the Mac). The last patch looks OK with this regard.

Comment 8 Rajesh 2010-08-17 03:56:41 EDT

Created attachment 176760 [details]
Patch3 - 319560 & 27996

(In reply to comment #6)

I have made the suggested changes. Some comments below - 

> The workbench.text layer must neither have a dependency on file buffers nor on
> IDE and hence we also follow that rule in the tests. Only at the ui.editors
> layer we can start to use file buffers and IDE stuff.

Done

> >In this case, if we name it forwardSearch, it could be misleading as even when
> >its value is 'true' the actual search could be backwards.
> It depends what you pass in: you could pass (!)isForwardSearch() into the
> perform method. I would also add performSearch(boolean forwardSearch) where
> performSearch() calls performSearch(isForwardSearch());.

Agreed. Done.
 
> Something else:
> - I see that e.detail= SWT.TRAVERSE_NONE is needed to stop further mnemonic
> traversal but I would also set e.doit to false. Any reason why you set it to
> true?

The e.doit is false when the application receives the event, this is because the traversal code in the Control class sends the event sequentially to all the controls (starting with parent) till the right target is found. Along with e.detail being set to  TRAVERSE_NONE, setting the e.doit to true causes the traversal to stop immediately because it does the TRAVERSE_NONE event right away.

> - char mnemonic; is defined outside its scope. But actually okToUse(button) is
> not needed there at all since we're in the middle of creating the dialog and
> hence it's definitely OK to use the button. 

Agreed.                   

> 
> 
> I'm not sure Markus's comment is valid since I would expect that on a machine
> (like Mac) where mnemonics aren't supported, we don't get TRAVERSE_MNEMONIC
> events.

Markus's comment #7 explains this.

Comment 9 Dani Megert 2010-08-17 10:07:34 EDT

>Created an attachment (id=176760) [details] [diff]
>Patch3 - 319560 & 27996
I'll wait for a single patch.

Comment 10 Rajesh 2010-08-23 00:21:56 EDT

Created attachment 177194 [details]
Patch contains fix and test

Comment 11 Rajesh 2010-08-23 02:01:14 EDT

Created attachment 177195 [details]
Patch contains fix and test

Just saw Markus's fix to handle bug 323272. Had seen similar code in DisplayHelper but wasn't sure why the delay was required, now I know :) Besides the fixed delay not being completely reliable on GTK, the other problem ofcourse is that the delay means user can click away and break the focus tests. I tried again to look for other options but couldn't find any.

Comment 12 Dani Megert 2010-08-23 04:24:57 EDT

Is the patch against HEAD? I have conflicts in the test project.

Comment 13 Rajesh 2010-08-23 05:25:01 EDT

Created attachment 177198 [details]
Patch contains fix and test

Now its has Markus's changes, previously missed updating before creating patch.

Comment 14 Dani Megert 2010-08-23 06:23:06 EDT

Please check your patch again. It contains superfluous trailing whitespace at various places.

Comment 15 Rajesh 2010-08-23 09:43:38 EDT

Created attachment 177215 [details]
Patch contains fix and test

Comment 16 Dani Megert 2010-08-24 05:54:57 EDT

Committed to HEAD with two modifications:
- "b" is not a good variable name ==> "button"
- Javadoc comments should end with '.'

Available in builds >= I20100824-0800.