Community
Participate
Working Groups
Many users expect Ctrl-Shift-F with no selection to do different things: 1. Format the entire file. 2. Do nothing. 3. Format the line the cursor is currently on. It would be good to have a preference where users could specify which of action they would like 'Format' with no selection to perform. Currently a user who expects 'Format' with no selection to do nothing will be very surprised if it formats their entire source file (especially if the area that is currently visible in the editor is unchanged but other places in the file are formatted, perhaps incorrectly) and they do not notice until it is too late to just undo the formatting.
One questions that occurs to me: why would I hit format if I expect it to do nothing?
(In reply to Sebastian Zarnekow from comment #1) > One questions that occurs to me: why would I hit format if I expect it to do > nothing? There is always a possibility to hit Shift+Ctrl+F accidentally, for example by trying to search for references (Shift+Ctrl+G).
(In reply to Sergey Prigogin from comment #2) > (In reply to Sebastian Zarnekow from comment #1) > > One questions that occurs to me: why would I hit format if I expect it to do > > nothing? > > There is always a possibility to hit Shift+Ctrl+F accidentally, for example > by trying to search for references (Shift+Ctrl+G). Then simply hit Ctrl+Z to undo this. *** This bug has been marked as a duplicate of bug 248059 ***
(In reply to Dani Megert from comment #3) As comment #0 explains, the user may not realize that the file was formatted when Shift+Ctrl+F was hit accidentally if the visible part of the file did not change. The user may continue to work until (s)he realizes what happened. At that point using Undo would erase important changes made after the accidental reformatting.
If someone tends to often accidentally hit this (or some other) command, then the right thing is to assign different key sequences and not add preferences to disable the functionality.
(In reply to Dani Megert from comment #5) Accidental formatting of the whole file doesn't have to happen often to be very painful. The amount of pain is determined by the size of the file and the extent of changes the user made before discovering the accidental reformatting. Very few Eclipse commands have a potential of introducing large-scope changes without user even noticing. It is somewhat surprising that Dani is in a different camp on this bug compared to bug 388476. Is it somehow related to code ownership?
(In reply to Sergey Prigogin from comment #6) > (In reply to Dani Megert from comment #5) > > Accidental formatting of the whole file doesn't have to happen often to be > very painful. The amount of pain is determined by the size of the file and > the extent of changes the user made before discovering the accidental > reformatting. Very few Eclipse commands have a potential of introducing > large-scope changes without user even noticing. There are many other commands that can change lots of text (also outside the visible area) when invoked, e.g. Delete Line, Replace With, etc. > It is somewhat surprising that Dani is in a different camp on this bug > compared to bug 388476. Is it somehow related to code ownership? That bug is a different in the sense that existing functionality got lost. I would not need/ask for a preference if it was just restored again. Please don't reopen, as we won't add this, even if it is kept open.
(In reply to Dani Megert from comment #7) > There are many other commands that can change lots of text (also outside the > visible area) when invoked, e.g. Delete Line, Replace With, etc. Sorry, I don't understand how Delete Line can delete text outside of the visible area unless user select a part of the file and then scrolls to a different part of the file. Replace With commands are also not a good analogy since they either require a second user action, or not used frequently enough to deserve a singe-keystroke key binding.
(In reply to Sergey Prigogin from comment #8) > (In reply to Dani Megert from comment #7) > > There are many other commands that can change lots of text (also outside the > > visible area) when invoked, e.g. Delete Line, Replace With, etc. > > Sorry, I don't understand how Delete Line can delete text outside of the > visible area unless user select a part of the file and then scrolls to a > different part of the file. Replace With commands are also not a good > analogy since they either require a second user action, or not used > frequently enough to deserve a singe-keystroke key binding. Maybe not perfect examples, but they show how text can change accidentally. Your point/request is about accidentally doing harm on Format, right? If so, why should we format when there's a selection? You could also accidentally invoke the action in that case. *** This bug has been marked as a duplicate of bug 248059 ***
I would accept a patch/solution as proposed in bug 419399 comment 3.
Once the patch for bug 441448 is submitted, I'm going to implement a solution similar to https://git.eclipse.org/r/#/c/31338/.
Moving to M4 since M3 is this week.
Hi Sergey, I assume we move this to M5, right?
(In reply to Dani Megert from comment #13) > Hi Sergey, I assume we move this to M5, right? Yes. Harry Terkelsen agreed to work on this.
(In reply to Sergey Prigogin from comment #14) > (In reply to Dani Megert from comment #13) > > Hi Sergey, I assume we move this to M5, right? > > Yes. Harry Terkelsen agreed to work on this. Please note that this week is the last week of development for M5.
(In reply to Dani Megert from comment #15) > (In reply to Sergey Prigogin from comment #14) > > (In reply to Dani Megert from comment #13) > > > Hi Sergey, I assume we move this to M5, right? > > > > Yes. Harry Terkelsen agreed to work on this. > > Please note that this week is the last week of development for M5. Sorry, we have this and also next week for M5.
The mirror patches for JDT are here: UI Patch: https://git.eclipse.org/r/#/c/40079/ Core Patch: https://git.eclipse.org/r/#/c/40078/
(In reply to Harry Terkelsen from comment #17) > The mirror patches for JDT are here: > > UI Patch: > https://git.eclipse.org/r/#/c/40079/ > > Core Patch: > https://git.eclipse.org/r/#/c/40078/ Thanks. I'm sorry, I have to move this to M6. Some more urgent patched ended up in my inbox.
(In reply to Dani Megert from comment #18) > (In reply to Harry Terkelsen from comment #17) > > The mirror patches for JDT are here: > > > > Core Patch: > > https://git.eclipse.org/r/#/c/40078/ We don't want to change the behavior of the formatter. The region must be set outside in JDT UI. If you need the AST, then you should get it from the SharedASTProvider.
I was thinking that each formatter could choose how to handle formatting an empty region. In the C++ formatter, for example, when using the built-in formatter, formatting an empty region expands the region to be formatted to the surrounding statements, but when using clang-format it passes the empty region so that clang-format may decide how to handle it. If we expand the region in the UI then this does not allow third-party formatters to handle empty regions as they normally would.
(In reply to Harry Terkelsen from comment #20) > I was thinking that each formatter could choose how to handle formatting an > empty region. In the C++ formatter, for example, when using the built-in > formatter, formatting an empty region expands the region to be formatted to > the surrounding statements, but when using clang-format it passes the empty > region so that clang-format may decide how to handle it. If we expand the > region in the UI then this does not allow third-party formatters to handle > empty regions as they normally would. The UI asks the user what to do in case of an empty selection, hence it needs to pass that information to the formatter. Unrelated to this discussion, the change in JDT Core is a breakage compared to the published API.
I wasn't part of the earlier discussion but it looks like we decided not to pursue this bug fix? Can someone confirm?
