Bugzilla – Full Text Bug Listing
| Summary: | [rulers] Show code blocks (was: matching brackets) in ruler | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Dani Megert <daniel_megert> | ||||||||||||||
| Component: | Text | Assignee: | Deepak Azad <deepakazad> | ||||||||||||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P2 | CC: | daniel_megert, deepakazad, gdtaylor, markus.kell.r | ||||||||||||||
| Version: | 3.0.2 | ||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Dani Megert
In progress.. patch coming up. Moving to M5 as its the last week of M4. I think by default we should show this in the vertical ruler like the method magnet. Moving to M6 and reassigning to inbox. I'll work on it if time permits in M6. (In reply to comment #2) > I think by default we should show this in the vertical ruler like the method > magnet. By 'like method magnet' you mean, just shade the vertical ruler and no icons? If yes, then what happens when a user wants to see the annotations for matching brackets in overview ruler as well? Assuming we use icons, what happens when the matching pair is on one line? e.g. 1. int x= ( ( 1 + 2 ) ); 2. Map<List<String>, List<List<String>>> m=null; 3. if ( a < 0) { return null; } - Show only one annotation in the ruler? (This may look odd..) - Show none? (In reply to comment #4) > (In reply to comment #2) > > I think by default we should show this in the vertical ruler like the method > > magnet. > By 'like method magnet' you mean, just shade the vertical ruler and no icons? Yes. > If yes, then what happens when a user wants to see the annotations for > matching brackets in overview ruler as well? The user goes to the 'Annotations' preference page and clicks the check box. > Assuming we use icons, what happens when the matching pair is on one line? We won't use icons ;-) Once the feature is implemented, we can play with it and decide on the defaults. (In reply to comment #5) > > If yes, then what happens when a user wants to see the annotations for > > matching brackets in overview ruler as well? > The user goes to the 'Annotations' preference page and clicks the check box. We use shading in Overview ruler as well, as opposed to showing 2 distinct markers at opening and closing braces? I guess we are going for something similar to 'Quick diff' which uses shading in both rulers and by default is enabled only on vertical ruler. (In reply to comment #7) > (In reply to comment #5) > > > If yes, then what happens when a user wants to see the annotations for > > > matching brackets in overview ruler as well? > > The user goes to the 'Annotations' preference page and clicks the check box. > We use shading in Overview ruler as well, as opposed to showing 2 distinct > markers at opening and closing braces? I'd start with that approach. This will allow us to only add 1 annotation. If we are not happy with this we can go for two. > I guess we are going for something similar to 'Quick diff' which uses shading > in both rulers and by default is enabled only on vertical ruler. No, we are not. Quick Diff is different and is not shown in the vertical ruler. We won't go down that road for the brackets. (In reply to comment #8) > No, we are not. Quick Diff is different and is not shown in the vertical ruler. Ah ok. It is shown in the line number column, which is adjacent to the vertical ruler... A question on preferences - Currently the Matching/Enclosing brackets is controlled by options on Java > Editor page. - When we add an annotation the corresponding options go on General > Editors > Text Editors > Annotations page Now, how should these preferences work together? I have the following in mind - The color used to highlight matching brackets is same as the annotation color, i.e. Matching Bracket Highlight color option is removed from Java > Editor page. - The annotation is shown only when the feature is enabled on Java > Editor page and a pair of matching brackets is highlighted in the editor. Another model could be to show the annotation independent of the options on Java > Editor page, but I think that would be wrong. Any objections? Created attachment 212377 [details]
patch for jdt ui - prototype
Created attachment 212378 [details]
patch for platform text - prototype
This is a quick prototype of see how the annotation will feel like.
I agree about removing the color from Java > Editor. The annotation should always be shown, like other annotations that can be toggled elsewhere (e.g. Occurrences). I like the 1/4 bar in the vertical ruler, but I'm not sure yet about showing the whole range in the overview ruler. The annotation name "Matching Bracket" in the overview ruler also needs some thought. For enclosing brackets, the term "matching" alone is quite far-fetched. Maybe just "Brackets"? (In reply to comment #13) > The annotation should always be shown, like other annotations that can be > toggled elsewhere (e.g. Occurrences). What do you mean? Once you turn off 'Mark Occurrences' in the toolbar or in Java > Editor > Mark Occurrences, the annotation is no longer shown. > but I'm not sure yet about showing > the whole range in the overview ruler. What are the other options? Show 1/4 bar there as well? Show only the beginning and end markers? (In reply to comment #14) I thought you wanted to remove the annotation from the list on the preference page, but it seems you didn't want to do that. Overview ruler: Showing only begin/end markers. (In reply to comment #15) > Overview ruler: Showing only begin/end markers. That would mean - Adding two annotations for a pair, instead of one. Also, that would mean not being able to show a bar in vertical ruler. (At least I don't know at this point how we can show a bar in vertical ruler and 2 markers in Overview ruler) - Or, we have to add infrastructure to control painting of an annotation in Overview ruler. I like showing the whole range in Overview ruler because - It is consistent with other annotations which show a range like Quick diff - It is also consistent with showing the range in Vertical ruler - You can click anywhere on the range in the overview ruler, the entire text within the enclosing brackets is highlighted. This is consistent with double-clicking next to a bracket in the editor. Created attachment 212435 [details]
patch for jdt ui
Created attachment 212436 [details] patch for platform text These patches should be quite close to complete. (The patches are on top of patches from bug 372128.) I have used defaults which I think are reasonable - Blue color for the annotation (looks nice in the vertical ruler), and not so different from the gray color previously used in the editor. - Annotation is shown only in vertical ruler by default. Also, - The whole range is shown in the Overview ruler (I just like it that way, hence I am sticking to that for now :-) ) - The old bracket highlight preference is migrated to the new annotation color. Minor TODOs- - Add a link to the Java Editor preference page to point to the annotations page - Changing the color for the annotation does not update the color for already painted annotation in the overview ruler (have to restart editor). Comments are welcome! Created attachment 212451 [details]
patch for jdt ui
Created attachment 212452 [details]
patch for platform text
That should do it!
- Patches are against master.
- All remaining issues that I was aware of have been fixed.
- There is an API addition in MatchingCharacterPainter.
Thanks for the patch Deepak. Markus and I played with the patch and are no longer convinced that showing the range covered by two matching brackets is what the user wants and that this is the relevant information. With the current patch the UI gets very noisy as the ruler indicators grow and shrink while moving through a method (e.g. when crossing comments or expressions). More useful would be to highlight the enclosing element as delivered by the AST. This will also be closer to what was actually requested in comment 0: > show the closest matching brackets surrounding the current block ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Possible changes: - allow to show the magnet (range indicator in the overview ruler) - in JDT allow to set the scope for the magnet to the enclosing element or introduce a second magnet Besides that fundamental problem, merging the concept of two positions with the range covered by those positions doesn't play well. We should keep them separate. We need to discuss this in more detail and see whether we can deliver a good solution during M7. BTW: Why is the change in the Overview ruler needed? Is it a separate bug? (In reply to comment #21) > With the current patch the UI gets very noisy as the > ruler indicators grow and shrink while moving through a method (e.g. when > crossing comments or expressions). The range indicator is updated after a delay i.e. when you scroll via Arrow_Down key the range indicator is updated only when you stop scrolling. Whereas the brackets annotation is updated as you move the caret. Is this what you refer to as noisy? > Possible changes: > - allow to show the magnet (range indicator in the overview ruler) That would make sense. > - in JDT allow to set the scope for the magnet to the enclosing element or > introduce a second magnet Not sure what you mean here, doesn't the magnet already mark the enclosing element i.e method or type? > BTW: Why is the change in the Overview ruler needed? Is it a separate bug? Yes, I think this is a bug in Overview ruler. Without this change the annotation for matching brackets is not rendered as a range in the overview ruler, even though you can 'click' in the entire region that annotation should have been rendered. (In reply to comment #21) > Possible changes: > - allow to show the magnet (range indicator in the overview ruler) > - in JDT allow to set the scope for the magnet to the enclosing element or > introduce a second magnet "Enclosing element" in the sense of "Edit > Expand Selection To > Enclosing Element", not in the sense of IJavaElement (the magnet already implements the latter). The new feature would use a scope that can be smaller than an IMember and that is based on AST nodes, not just brackets. E.g. a whole for-loop or if-else block, not only the body. I just realized this has some overlap with bug 60929, where the ranges would be turned into foldable sections. Maybe we should implement the foldable code blocks plus an option to render the folding block that contains the caret? I.e. permanently highlight the range that appears when you move the mouse over the folding ruler on the caret line. (In reply to comment #23) > I just realized this has some overlap with bug 60929, where the ranges would be > turned into foldable sections. I would say, there is a bit of overlap with bug 69455. Visualizing code blocks is a goal here and bug 69455 asks for exactly that. > Maybe we should implement the foldable code > blocks plus an option to render the folding block that contains the caret? I.e. > permanently highlight the range that appears when you move the mouse over the > folding ruler on the caret line. .. though this would also not be too bad. > > BTW: Why is the change in the Overview ruler needed? Is it a separate bug?
> Yes, I think this is a bug in Overview ruler. Without this change the
> annotation for matching brackets is not rendered as a range in the overview
> ruler, even though you can 'click' in the entire region that annotation should
> have been rendered.
Please try to find a test case within the existing SDK and file a separate bug.
(In reply to comment #23) > (In reply to comment #21) > > Possible changes: > > - allow to show the magnet (range indicator in the overview ruler) > > - in JDT allow to set the scope for the magnet to the enclosing element or > > introduce a second magnet > > "Enclosing element" in the sense of "Edit > Expand Selection To > Enclosing > Element", not in the sense of IJavaElement (the magnet already implements the > latter). Yes, that's what I had in mind. > I just realized this has some overlap with bug 60929, where the ranges would be > turned into foldable sections. Maybe we should implement the foldable code > blocks plus an option to render the folding block that contains the caret? I.e. > permanently highlight the range that appears when you move the mouse over the > folding ruler on the caret line. Sounds worth investigating. We need to see how it feels to get this information delayed when typing (since it's based on the AST). (In reply to comment #22) > (In reply to comment #21) > > With the current patch the UI gets very noisy as the > > ruler indicators grow and shrink while moving through a method (e.g. when > > crossing comments or expressions). > The range indicator is updated after a delay i.e. when you scroll via > Arrow_Down key the range indicator is updated only when you stop scrolling. > Whereas the brackets annotation is updated as you move the caret. Is this what > you refer to as noisy? No (but of course it would be preferred if the magnet and the block range indicator update at the same time). What I find noisy is that the enclosing bracket range grows and shrinks while typing and while moving the caret, e.g. from code into comment and back. (In reply to comment #27) > No (but of course it would be preferred if the magnet and the block range > indicator update at the same time). What I find noisy is that the enclosing > bracket range grows and shrinks while typing and while moving the caret, I don't see how highlighting the enclosing element range will be (significantly) less noisy as compared to highlighting enclosing brackets. The enclosing element range will also grow as you type, and shrink as you delete stuff... > e.g. from code into comment and back. That is bug 373978. Re "noisy": Even with bug 373978 fixed, the same problem shows up when the caret moves into the argument list of a method invocation. This usually makes the range collapse into a single line, and the annotation is often not even visible any more while you're typing, because a compile error is drawn on top of it. Re "delayed updating": The implementation could hook into the existing semantic coloring support, so that code blocks stay as stable as possible and are updated together with semantic colorings. (In reply to comment #29) > Re "noisy": Even with bug 373978 fixed, the same problem shows up when the > caret moves into the argument list of a method invocation. This usually makes > the range collapse into a single line, and the annotation is often not even > visible any more while you're typing, because a compile error is drawn on top > of it. Agree. I just wanted to understand what you were calling as 'noisy'. In terms of visualization, there are 2 options 1. Show something in the rulers like the existing magnet, as described in comment 23. 2. Show something within the editor i.e. something on the lines of bug 69455. I am leaning towards the 2nd option. Today we highlight the current line in the editor, similarly we can also highlight the current code block. > I am leaning towards the 2nd option. Today we highlight the current line in the
> editor, similarly we can also highlight the current code block.
Agreed.
|