Bug 366400

Comment 1 Markus Keller 2011-12-12 08:52:06 EST

This becomes much easier to implement after bug 9503 is fixed, since you don't have to care about corner cases with brackets on both sides of the caret like in

int i = ((1+2)-(3+"a".hashCode()));

Comment 2 Raksha Vasisht 2012-01-23 12:55:58 EST

Moving to M6 and reassigning to inbox. I'll look into if time permits in M6.

Comment 3 Deepak Azad 2012-02-08 00:17:50 EST

Created attachment 210706 [details]
screenshot

Dani, yesterday we were discussing the idea of using a combo to show the preferences i.e. Label + [No | Both | Matching Only | Enclosing].

However, the combo does not feel right to me

1. Quoting from Windows user experience guidelines (http://msdn.microsoft.com/en-us/library/windows/desktop/aa511458.aspx#rightcontrol)
 "Is seeing the selected option far more important than seeing the alternatives? Consider using a drop-down list if you don't want to encourage users to make changes by hiding the alternatives."

I think 'Highlight enclosing brackets' preference is important enough to not be 'hidden' in a combo. 

2. Quoting from Mac OS X Human Interface guidelines (https://developer.apple.com/library/mac/#documentation/userexperience/conceptual/applehiguidelines/Controls/Controls.html)
- "It’s best when the default selection (which may not be the first item in the list) provides a clue to the hidden choices"

The default selection (either Both or Matching only) does not really give a hint as to what other choices are hidden.

3. The attached screenshot does not look too bad to me.

Comment 4 Dani Megert 2012-02-08 04:59:30 EST

> 3. The attached screenshot does not look too bad to me.

The problem is that the sub-options also depend on each other.

Comment 5 Dani Megert 2012-02-08 10:32:09 EST

(In reply to comment #4)
> > 3. The attached screenshot does not look too bad to me.
> 
> The problem is that the sub-options also depend on each other.

Discussed this with Deepak: we can try this but with a different naming of the first option and auto-updating each other.

Comment 6 Deepak Azad 2012-02-09 07:58:31 EST

Created attachment 210792 [details]
patch for jdt ui

Comment 7 Deepak Azad 2012-02-09 08:04:31 EST

Created attachment 210794 [details]
patch for platform text

Discussed this with Dani:
 
[x] Highlight matching brackets
     [ ] Highlight bracket at caret location
     [x] Highlight enclosing brackets

With the above settings, when you place the caret at a bracket location only the matching bracket is highlighted i.e. 'Highlight bracket at caret location' means exactly what the wording says in all scenarios.

I think this simplifies the preference story - both the implementation and their meaning.

Note: These 2 patches also include fixes for bug 27372 and bug 40580.

Comment 8 Dani Megert 2012-02-10 04:47:33 EST

Almost works fine. There seems to be some state issue regarding the 'Highlight bracket at caret location' preference though:

1. paste this into the Java editor: "(buggy!)"
2. place caret between the two 'g's
   ==> both brackets highlighted (good)
3. use the 'Left Arrow' key to navigate to the left
   ==> bracket still highlighted when caret is to its right (wrong)
4. move one further to the left
   ==> bracket no longer highlighted when caret is to its left (good)
5. use the 'Right Arrow' key to navigate to the right
   ==> bracket highlighted never comes back for left bracket (bad)

Please take a look. In the meantime I'll continue with the review.

Comment 9 Deepak Azad 2012-02-10 05:01:23 EST

Created attachment 210840 [details]
patch for platform text

(In reply to comment #8)
> Almost works fine. There seems to be some state issue regarding the 'Highlight
> bracket at caret location' preference though:
Small bug in MatchingCharacterPainter. Fixed in the attached patch.

Comment 10 Dani Megert 2012-02-10 05:31:27 EST

Code Review:
- new code / API is missing tests
- manifests for jdt.ui and workbench.texteditor must include version 3.8 of 
  jface.text
- please add @since tags to all new stuff
- please add Javadoc to all code we tend to do this in Text
- 'fPrevSelections': please use readable variable names ==> fPreviousSelections
- don't make members accessible if not needed outside (e.g. 'isOpeningBracket')
- old 'setMatchingCharacterPainterPreferenceKeys' method needs clarification
  regarding the new options and we should reset the new options when the old
  API gets called.
- we list the extensions in the original interface, see e.g. 'IDocument'
- findEnclosingPeerCharacters: the 'null' case should be explained in @return
- 'iDocument' -> 'document'
- 'i' -> 'offset'
- findEnclosingPeerCharacters in the class should not get Javadoc, but a normal
  comment (delete the JAvadoc, then press Alt+Shift+J).


Since those are all additive/polish items I've committed the current state plus the small fix from comment 9 to master:
text: b8d0417ed59519a46d28cac812ad3b7393dd83e9
jdt:  a84b7b2087d40d3379292f09611cebb78a28e6be

Comment 11 Deepak Azad 2012-02-15 04:13:37 EST

Created attachment 211026 [details]
additional patch for platform text

I committed the JDT/UI changes - 89386d2947ac860c54615ee2e9b1de6871d36403

Comment 12 Dani Megert 2012-02-15 08:31:11 EST

(In reply to comment #11)

> I committed the JDT/UI changes - 89386d2947ac860c54615ee2e9b1de6871d36403
> - 'fPrevSelections': please use readable variable names
Of course this also applies to methods. Fixed in master.


> Created attachment 211026 [details] [diff]
> additional patch for platform text

Thanks, Deepak looks good. Some minor details:
- The implementation should use the same parameter names as in the interface
- "...characters and <code>null...": we use "or" since the method cannot return 
  both at the same time ;-)
- findEnclosingPeerCharacters: no need to repeat the null case in the normal 
  text.

I fixed those issues and committed the patch to master:
27da83b9a695fd137ec84f15c00c478813e3394a

Comment 13 Dani Megert 2012-03-15 05:02:15 EDT

Verified in I20120314-1800.