Community
Participate
Working Groups
Eclipse : 3.1M5a Right now to format a function, you have to highlight the function and choose 'Source --> Format' or 'Ctrl+Shift+F'. This can be cumbersome if the function is spanning more than a screenful. What would be cool is to be able to select the function in Outline view and choose 'Format'.
If so, we should also offer it in other views, e.g. Package Explorer or Members view.
*** Bug 99704 has been marked as a duplicate of this bug. ***
>If so, we should also offer it in other views, e.g. Package Explorer or Members >view. That's too much work but we can offer it from the Outline view quite easily, see org.eclipse.jdt.internal.ui.javaeditor.JavaEditor.FormatElementAction
Created attachment 178327 [details] fix Patch does 2 things - Add 'Format Element' action to the editor's Source sub menu (right click inside a Java editor and select source) - Make the same editor's sub menu available from the Outline view => As a result other actions are also available from outline view. Markus can you please take look if this behavior is ok?
> - Add 'Format Element' action to the editor's Source sub menu (right click > inside a Java editor and select source) That looks good. > - Make the same editor's sub menu available from the Outline view => As a > result other actions are also available from outline view. There are way too many actions in the context menu now. In the editor, it makes sense to also show actions that operate on the editor input, but in the Outline view, we need to stay more specific. Furthermore, I noticed the following problems: - the quick menu shortcut (Alt+Shift+S) doesn't work any more (see error log) - the Format Element action is not enabled in the main menu when an Outline element is selected - when the caret is inside a Javadoc, the action on the Outline menu formats only the Javadoc. That is expected in the editor, but not in the Outline - the behavior is unexpected when the editor has a non-zero selection - multi-element selection should either be handled or action should be disabled => Too many problems because the FormatElementAction has been implemented specifically for the editor. I think we would be better off implementing a separate action for the Outline (and maybe other views).
(In reply to comment #0) > Right now to format a function, you have to highlight the function and choose > 'Source --> Format' or 'Ctrl+Shift+F'. An easier way is "Source > Format Element" from the main menu, to which you can also assign a shortcut (e.g. Ctrl+Alt+Shift+F).
>An easier way is "Source > Format Element" from the main menu, to which you can >also assign a shortcut (e.g. Ctrl+Alt+Shift+F). Right. Let's first do that before we start to add more items to the context menu.
> >An easier way is "Source > Format Element" from the main menu, to which you can > >also assign a shortcut (e.g. Ctrl+Alt+Shift+F). > Right. Let's first do that before we start to add more items to the context > menu. I actually meant that as a tip that already works in the Java editor today. But I agree that the goal for this bug is to create a new action that works in the Outline (using the same command ID). Implementation will be similar to FormatElementAction, but without the editor-specific parts that depend on the current text selection. Main menu is enough for now, addition to context menu would be a separate enhancement request.
Created attachment 180015 [details] Fix Activates the 'Format Element' in the 'Source' main menu when a element in the Outline has been selected. It will only format the selected outline element(s) and the associated javadocs.
Seems to work as expected. - The action has useless code, assuming it's just for the Java Outline. - Redraw off/on is expensive and should only be done once and not for each element. - It looks like you might format the same element more than once. - It should be package visible in 'org.eclipse.jdt.internal.ui.javaeditor' - Why do you use a field for the format action? - It's good practice to remove the listener from the selection provider. ==> Ha! good you already have a field ;-) - Copyright is wrong (new files only have the start year). - Superfluous @since 3.7 tags: on new types only the type gets the @since tag. Once those things are addressed I'll review the format action in more detail.
(In reply to comment #9) > Activates the 'Format Element' in the 'Source' main menu when a element in the > Outline has been selected. We don't want to add 'Format Element' to the context menus in Outline View and in the Java Editor?
(In reply to comment #11) > (In reply to comment #9) > > Activates the 'Format Element' in the 'Source' main menu when a element in the > > Outline has been selected. > We don't want to add 'Format Element' to the context menus in Outline View and > in the Java Editor? Please read comment 7.
Created attachment 180128 [details] Fix (In reply to comment #10) > Seems to work as expected. > > - The action has useless code, assuming it's just for the Java Outline. The code was initially written to work for all views, missed modifying the action just for Outline. Addressed the other issues as well.
Since the action is for the Java Outline, you know on which Java editor the action works, so it's best to pass it directly upon creation instead of getting/searching the editor inside the action. The algorithm to avoid duplicate formatting is incorrect. You first need to compute the extent. I don't like the elementsAlreadyFormatted field: it's holding state between two methods ==> pass it along as parameter. viewer.setRedraw(true) must always be inside a finally block. All fields should be private.
Created attachment 180574 [details] Patch The patch also contains a CodeFormatterUtil test which fails due to a possible formatter issue. Have fixed the issues mentioned in the previous review.
The patch mostly works but has some issues: - The action must not be enabled if the element is read only and then do a NOP. - You need to handle change from/to read only. - You don't need a map to solve the problem. - JavaPlugin.isDebug() || !e.isDoesNotExist()) should not be there as this runs in the UI thread on selected elements. - I don't like the following constructs too much: - while (true) - assignments directly inside the condidtion - Minor tweaks: - rename FormatViewElementAction to FormatElementAction - move it into JavaOutlinePage (like the one on the JavaEditor) > The patch also contains a CodeFormatterUtil test which fails due to a possible > formatter issue. The formatter works as designed. It's not just a problem with fields but with the leading whitespace in general. I think the simplest fix is to move each region start to the beginning of the line - but only if that's all whitespace.
Created attachment 181355 [details] Patach (In reply to comment #16) > > The patch also contains a CodeFormatterUtil test which fails due to a possible > > formatter issue. > The formatter works as designed. It's not just a problem with fields but with > the leading whitespace in general. I think the simplest fix is to move each > region start to the beginning of the line - but only if that's all whitespace. Dani - Thanks for the review comments. The fix you mentioned above for the Formatter issue works only when there is 1 region to format. If there are multiple regions to be formatted, the formatter still fails for the field declaration region (even after including the preceding whitespace till the beginning of the line). I have modified the test to show this. The other review comments have been addressed.
Did you test it? It doesn't work for me: when I open a read-only file the action is enabled and running it does nothing. Expected: either don't enable the action or ask the user to make the file writable (latter is preferred: try out Source > Format while the type is selected in the Outline). >- JavaPlugin.isDebug() || !e.isDoesNotExist()) should not be there as this Now you swallow the exception. That's not good. > - rename FormatViewElementAction to FormatElementAction This also applies to the field. It's misleading if fJavaSourceViewer is a field in the action and fEditor is grabbed from the enclosing instance. getElementRegion(...) overcomplicated and inefficient. Simply walk back from the current position until you have a non-whitespace or you reach the line beginning. Filed bug 328362 for the formatter issue.
Created attachment 181628 [details] Patch (In reply to comment #18) > Did you test it? It doesn't work for me: when I open a read-only file the > action is enabled and running it does nothing. Expected: either don't enable > the action or ask the user to make the file writable (latter is preferred: try > out Source > Format while the type is selected in the Outline). Amongst testing all the changes, I assumed that my implementation had similar behavior as Format Element in the Java Editor, but apparently not. I have now implemented the 'ask the user' way. My bad. > > getElementRegion(...) overcomplicated and inefficient. Simply walk back from > the current position until you have a non-whitespace or you reach the line > beginning. Since various conditions that have to be checked I don't think the 'walking backwards' approach is any less complicated, but it is slightly more efficient in one case. Anyway, I have attached the new approach.
Sorry, but the patch is still not fixing the read-only problem: 1. open read-only file 2. Source > Format Element ==> it formats the read-only file and puts the editor in dirty state. Expected: Behave similar to Source > Format. Also, please make sure that your patches only contain the changes needed to fix the bug. The recent patch has lots of unrelated changes that make a review harder than it needs to be.
(In reply to comment #20) > Sorry, but the patch is still not fixing the read-only problem: > 1. open read-only file > 2. Source > Format Element > ==> it formats the read-only file and puts the editor in dirty state. > Expected: Behave similar to Source > Format. May bad: I had the focus on the editor instead of the Outline. Rajesh, can you open a new bug report to track that problem?
Created attachment 181652 [details] Patch The file JavaOutlinePage.java contains numerous indentation and whitespace issues which is why the previous patch had lots of redundant changes. I will open a bug to fix that.
Comment on attachment 181652 [details] Patch Thanks Rajesh, the last patch is good.
Committed to HEAD. Available in builds >= I20101025-1300.
Verified for 3.7M3 on Linux-GTK with I20101025-1800.
