Community
Participate
Working Groups
Brace and parenthesis matching tries to be too useful in that it shows up regardless of whether the cursor is to the left or right of the brace/paren. It should only show up if double-clicking in that same spot would cause a brace/paren. selection to occur. The following is BROKEN, where <CARET> indicates caret location, and [] surrounds the highlighted paren.: (<CARET>(MyType[)]variable) In this case, the last paren should be highlighted, not the first. If I double- click where the caret is, it will select from that point until the last paren. The same goes for: new Rectangle(p, new Dimension[(])<CARET>);
To be completely thorough: (new TreeItem(parent, 0))<CARET>; should not highlight anything. My reasoning again is that double-clicking there does not perform any smart selection, and that excessive highlighting is unwanted.
My apology for the excessive appends. It seems that double-click selection is also not working as it did in VA-Java. (<DOUBLE_CLICK<(Type)variable) Will cause the word "Type" to be highlighted. In Va-Java, the entire "(Type) variable" would have been highlighted. I will argue that the current way is inferior because the index where I double-clicked (position '1') and the range that gets selected (2 through 6)do not touch. It is more intuitive to click at a location that is also one of the boundaries of what you are trying to select.
Related to bug #16879 and bug #9151.
*** Bug 23089 has been marked as a duplicate of this bug. ***
From bug 23089 (just marked as a duplicate of this bug): --------------- Put the cursor in between the following two sets of parentheses in a java file in JDT: "()()" The editor highlights the set of parentheses to the left. But I wanted to see where the "(" paren on the right matched! It should show both matches or something. ------------------- I also added cc's of people found in the following related bugs: bug 16879 - Double click after { does not always select the right block of code. bug 9151 - Bracket matching and related selections should be configurable via preference bug 5359 - Bracket matching precedence? bug 23089 - Paren-matching behavior arbitrary when in between a ")" and a "("
*** Bug 25282 has been marked as a duplicate of this bug. ***
Closing for lack of activity. Note sure if this is even an issue any more.
*** This bug has been marked as a duplicate of bug 9151 ***
*** Bug 345918 has been marked as a duplicate of this bug. ***
*** Bug 121013 has been marked as a duplicate of this bug. ***
Let's fix the position to follow more logical the term "enclosing".
*** Bug 159836 has been marked as a duplicate of this bug. ***
(In reply to comment #11) > Let's fix the position to follow more logical the term "enclosing". Concretely, change the matching for closing brackets such that double-click, matching bracket highlighting, and 'Navigate > Go To > Matching Bracket' target the closing ])} on the *right* of the caret/clicked position. Examples: [ Caret pos.: | Matching brackets pos.: ^ ^ ] Old: New: ( ( 1 + 2 ) ) ( ( 1 + 2 ) ) ^| ^ ^| ^ [unchanged for opening brackets] ( ( 1 + 2 ) ) ( ( 1 + 2 ) ) | [no match] ^ |^ [match closing bracket at right] ( ( 1 + 2 ) ) ( ( 1 + 2 ) ) ^ ^| [match at left] ^ |^ [match closing bracket at right] ( ( 1 + 2 ) ) ( ( 1 + 2 ) ) ^ ^| [match at left] | [no match]
Moving to M6..
Created attachment 210597 [details] patch for platform text
Created attachment 210598 [details] patch for jdt ui Dani, please release the patches if they look OK to you.
Created attachment 210603 [details] patch for jdt ui oops, missed 1 file in last patch.
Code changes are good but we need to make the new behavior opt-in i.e. don't enforce our new desired behavior on existing DefaultCharacterPairMatcher clients.
Created attachment 210646 [details] patch for jdt ui
Created attachment 210651 [details] patch for platform text (In reply to comment #18) > Code changes are good but we need to make the new behavior opt-in i.e. don't > enforce our new desired behavior on existing DefaultCharacterPairMatcher > clients. Done. About changes in the tests 1. AbstractPairMatcherTest had the actual test methods which were run with both JavaPairMatcherTest and DefaultPairMatcherTest. => I have moved these test* methods down to DefaultPairMatcherTest so that they are run only once. 2. I have added tests for the new matching behavior in DefaultPairMatcherTest1, which is largely similar to DefaultPairMatcherTest. => The existing tests remain untouched, except of course the push down mentioned in point 1.
The code (non-test) changes look good. I've pushed them to master: text: 242dba4fc651a8d29175e9583fe9601ef0d7fc6e jdt: 345ebc8ae2e73a08ac5d390790bdb6d33f0d4f1a However, I don't like the tests, especially the duplication with (false/true based on the new flag). We should keep the tests abstract and e.g. in createTestCase(...) craft the test case depending on the new flag. What was the motivation to remove the position/length assertions in JavaPairMatcherTest?
Created attachment 210731 [details] patch for jdt ui tests
Created attachment 210732 [details] patch for platform text tests
Closer ;-). I don't like setting the state in fields. The state should be passed via method parameters. E.g. performReaderTest(...) should take the arguments and pass them to createTestCase(...). This will also make createTestCase(...) easier to read: currently is uses 'str' and 'fChars' which both have the same value.
(In reply to comment #24) > I don't like setting the state in fields. The state should be passed via method > parameters. E.g. performReaderTest(...) should take the arguments and pass them > to createTestCase(...). That would also mean passing the arguments to AbstractPairMatcherTest.performMatch(...) and then duplicating the state in all the test* methods... I am also not chuffed about using fields here, but I think that's the simplest solution here. > This will also make createTestCase(...) easier to read: > currently is uses 'str' and 'fChars' which both have the same value. Not true. 'str' is the test case string e.g. "#( )%", while fChars is "()[]{}".
(In reply to comment #25) > (In reply to comment #24) > > I don't like setting the state in fields. The state should be passed via method > > parameters. E.g. performReaderTest(...) should take the arguments and pass them > > to createTestCase(...). > That would also mean passing the arguments to > AbstractPairMatcherTest.performMatch(...) and then duplicating the state in all > the test* methods... > > I am also not chuffed about using fields here, but I think that's the simplest > solution here. Maybe simplest but not nice regarding maintainability. I know you can do better :-).
Created attachment 210772 [details] patch for jdt ui tests
Created attachment 210773 [details] patch for platform text tests Here we go again...
It's still having too much duplication. I've now tweaked it to my liking and pushed them to master: text: be38ff2e5af29ddc9d015d906172a02870264ffb jdt: e1fb770a8bd4360b0a3f1fd7a51f8e794107a359 Please take a look what I had in mind.
(In reply to comment #29) ... and you brought back the field :-)
(In reply to comment #30) > (In reply to comment #29) > ... and you brought back the field :-) Yes, the one which separates the two test classes. But no field for states between tests, like fChar.
*** Bug 103095 has been marked as a duplicate of this bug. ***
