Community
Participate
Working Groups
Juno M4. We should allow to always show enclosing brackets. This would be an additional option in the bracket highlighter and a new preference on the Java > Editor page (below 'Highlight matching brackets').
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()));
Moving to M6 and reassigning to inbox. I'll look into if time permits in M6.
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.
> 3. The attached screenshot does not look too bad to me. The problem is that the sub-options also depend on each other.
(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.
Created attachment 210792 [details] patch for jdt ui
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.
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.
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.
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
Created attachment 211026 [details] additional patch for platform text I committed the JDT/UI changes - 89386d2947ac860c54615ee2e9b1de6871d36403
(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
Verified in I20120314-1800.
