Community
Participate
Working Groups
Created attachment 181574 [details] proposed patch For these strings it probably makes sense to escape the slash on paste "Hello ä ☃, \u00e4 \u2603" "C:\Program Files\Java" But I am not too sure for a string like "\u00e4 \t \u2603 \n". The user probably copied a string from a properties file itself. The patch will leave a completely valid properties file text as it is. Markus, can you please take a look if the patch makes sense.
(In reply to comment #0) > For these strings it probably makes sense to escape the slash on paste > "Hello ä ☃, \u00e4 \u2603" > "C:\Program Files\Java" argh...the patch does not work for "C:\Program Files\Java". The code needs to be smarter, I will take a look..
Created attachment 181613 [details] fix + tests This patch should be good.. \es are escaped only when the pasted text is not perfectly correct for a .properties file, i.e. if the text contains - a single <code>\</code> that is not followed by t,n,f,r,u,\ - a character which requires Unicode escapes as defined by PropertiesFileEscapes#needsUnicodeEscape(char) Essentially I am trying to guess if the text was copied from a properties file or not, and this behavior looks nice to me. Markus, comments ?
I don't think we should do this. It's quite hard to understand for the user why certain strings get escaped on paste and others are pasted verbatim. And if we take the wrong guess, the user has to remove the double escapes again. In the Java editor, we have exactly the same problem when the user pastes into a String, and in the end, we added a preference (which is at least predictable and configurable).
(In reply to comment #3) > In the Java editor, we have exactly the same problem when the user pastes into > a String, and in the end, we added a preference (which is at least predictable > and configurable). ok, how about adding a preference for Smart paste and have it turned on by default. We could also add actions to explicitly escape/unescape \es in a typed/pasted text.
Fixed in HEAD in PreferenceMessages.java PropertiesFileEditorPreferencePage.java PreferenceMessages.properties PropertiesFileAutoEditStrategy.java PropertiesFileSourceViewerConfiguration.java PreferenceConstants.java PropertiesFileAutoEditStrategy.java The fix essentially adds a preference for this smart paste and this preference is turned on by default.
(In reply to comment #5) > Fixed in HEAD in > PreferenceMessages.java > PropertiesFileEditorPreferencePage.java > PreferenceMessages.properties > PropertiesFileAutoEditStrategy.java > PropertiesFileSourceViewerConfiguration.java > PreferenceConstants.java > PropertiesFileAutoEditStrategy.java The last file is PropertiesFileAutoEditStrategyTest.java
Please revert this. The story is a bit more difficult here than in the Java editor. There, - escaping is full predictable - we only escape characters that would be invalid inside a string literal - if the user copy/pastes complete lines, we never escape (because no line can start as a string literal) - if the user wants to paste without escaping, he can easily do so by pasting outside a string literal and adding the "s later In the Properties file editor, there's no area where a user can easily paste something without getting escapes, so we cannot automatically escape. Copy/paste of a line (with and without line delimiters) must always exactly duplicate the line. Test case to see the confusing behavior of the patch: x= ..\rababa y= ..\alibaba z= Looking at the history of this issue (bug 43204, bug 268070, bug 286536), I only see 2 sustainable solutions: a) add a new "Pasted Escaped Text" command b) on Paste, perform a normal paste. If it could be interesting to escape the pasted string, show some kind of popup that lets the user escape the string after the fact (e.g. enter linked mode and show the option in a Ctrl+1-like window). Whichever solution we choose, we should put that into the text editor, so that all text-based editors can use the same mechanism.
(In reply to comment #7) > Please revert this. > > The story is a bit more difficult here than in the Java editor. There, > - escaping is full predictable > - we only escape characters that would be invalid inside a string literal > - if the user copy/pastes complete lines, we never escape (because no line can > start as a string literal) > - if the user wants to paste without escaping, he can easily do so by pasting > outside a string literal and adding the "s later Bug 268070 comment 0 says > sometimes I paste some text that is already escaped (e.g. from another > String) and it gets escaped for a second time Paste "Hello \n World" (the problematic string from Bug 43204, Bug 268070) - in a string literal in Java Editor => "Hello \\n World" - in Properties file editor => "Hello \n World" : No escaping is done as the string is already valid and could have come from a properties file itself. I think we should make the feature in Java Editor smarter, instead of removing this feature from properties file editor. (If someone does not like this smart auto-escaping of backslashes they can always turn off the preference.) > In the Properties file editor, there's no area where a user can easily paste > something without getting escapes, so we cannot automatically escape. > Copy/paste of a line (with and without line delimiters) must always exactly > duplicate the line. Test case to see the confusing behavior of the patch: > x= ..\rababa > y= ..\alibaba > z= I would call this test case a further enhancement. Currently the decision to escape or not is based on the entire pasted text. Making the decision per line (or maybe per key value pair ?) should be easy enough. e.g. w= .. Hello \n World x= ..\rababa y= ..\alibaba z= कतरक could be pasted as (the first 2 lines are not escaped) w= .. Hello \n World x= ..\rababa y= ..\\alibaba z= \u0915\u0924\u0930\u0915 > Looking at the history of this issue (bug 43204, bug 268070, bug 286536), I > only see 2 sustainable solutions: > a) add a new "Pasted Escaped Text" command > b) on Paste, perform a normal paste. If it could be interesting to escape the > pasted string, show some kind of popup that lets the user escape the string > after the fact (e.g. enter linked mode and show the option in a Ctrl+1-like > window). I would not want to remember a command for this, escaping slashes by hand is an easier option. We could offer quick fixes or actions on context menu which escape or unescape selected text. This would also help if someone just wants to fix typed text (as opposed to pasted text). (Note that Bug 106721 will make it easier to identify special characters e.g. \t,\n) > Whichever solution we choose, we should put that into the text editor, so that > all text-based editors can use the same mechanism. hmm.. other text-based editors will have different escape sequences than Java, and will also need to take actions based on the structure and location of the pasted text, e.g. inside a string literal or not in Java Editor, key value pairs in properties file editor. But I think we can provide an auto edit strategy for text editors which can be configured/tweaked by a particular editor.
-1 for additional command and code in the text editor. I suggest to discuss this on a call.
The behavior needs to be predictable. The default must be that copy/paste of any selection results in the same content as before. That's also the case in the Java Editor, where the smart paste is disabled by default. Re text editor vs. language-specific editors: Of course, the concrete implementation has to be in each language-specific editor, and the default text editor cannot offer an implementation. But as always, we try to re-use implementations and use the same UI in all editors.
Created attachment 182449 [details] fix + tests As discussed on the call - The fix is good except that the auto-escaping must not happen silently. The user must be informed and allowed to undo the auto-escaping. - Implement 'Linked Mode' to inform the user. - I have reverted the fix in HEAD for now.
Created attachment 182556 [details] fix v0.5 Patch does the following - Always paste as is (except using unicode escapes depending on file encoding) - If backslashes should be escaped, enter linked mode and show a popup with the proposal (the end result is also shown) - I have just added a single proposal - 'Escape backslashes'. Do we need to show two - 'Leave as is' and 'Escape backslashes' ? - No preference to configure The patch works quite nicely if you paste a single line. (I did not especially like the UI for linked mode in case of multi line paste - maybe this can be improved.) Markus, can you please try this patch and see how it feels. The linked mode still needs a little polish, I will do that if this looks promising.
I tried out the patch. It goes into the right direction but has some issues: - The default (pressing 'Enter') should do a raw paste ==> need second proposal - After selecting the proposal there shouldn't be any left-over of the linked mode: currently I have to press 'Enter' or 'Esc' again. This is unexpected for a paste operation. - Canceling the shell with 'Esc' shouldn't leave any left-over of the linked mode: currently I have to press 'Enter' or 'Esc' again to get into normal state. This is unexpected for a paste operation. - The (yellow) color is wrong: we use that to display a list of choices/values. For shells that show actions or allow to select a proposal we use white background. - Placement of the shell is wrong in case of multi-line paste (it ends up in the middle of the pasted string.
(In reply to comment #13) > - Placement of the shell is wrong in case of multi-line paste (it ends up > in the middle of the pasted string. Yes, that was one thing I did not like. Also each pasted line is surrounded by a rectangular box. This is ok? (I would probably prefer to have the entire pasted text being surrounded by a single polygon instead of a rectangle for each line.)
> Also each pasted line is surrounded by > a rectangular box. This is ok? No.
(In reply to comment #15) > > Also each pasted line is surrounded by > > a rectangular box. This is ok? > No. That's just how the painting of multi-line annotations works. You could try how it feels for multi-line paste if you only create a zero-length linked position at the end of the pasted string. You should stop the linked mode as soon as the user starts modifying the document in any way (types a character, backspace, delete, tab,...). The preview is currently too intrusive. Either we also need the 'Leave as is' entry, or you have to remove the preview (which btw. doesn't show line breaks correctly). I also find it strange that I get the popup when I paste a= C:\abc.txt but I don't get it for r= C:\rest.txt I would expect it in both cases.
(In reply to comment #16) > I also find it strange that I get the popup when I paste > a= C:\abc.txt > but I don't get it for > r= C:\rest.txt > > I would expect it in both cases. You want the popup to come whenever there is a \ in the text ? Even in strings like "Hello \n World", "Hello \t World" etc? Note that in these 2 strings the user almost never wants to escape \es. Or you want more smartness in the code to figure out when \es in \n,\t,\r etc need to be escaped? :)
> You want the popup to come whenever there is a \ in the text ? Even in strings > like "Hello \n World", "Hello \t World" etc? Note that in these 2 strings the > user almost never wants to escape \es. Why not? If the string is a path or an example text, then escapes also make sense there. Again, it's about predictability and not surprising the user. > - The (yellow) color is wrong: we use that to display a list of > choices/values. For shells that show actions or allow to select a proposal > we use white background. Hmm, maybe we should just drop the linked mode and implement this as a quick assist. This would also be useful if you already have some text that you need to escape: You could just select it, press Ctrl+1, and choose the quick assist. On paste, the quick assist popup would just open automatically at the end of the pasted string. But in this automatic mode, it should not need the pasted string to be selected.
> Hmm, maybe we should just drop the linked mode and implement this as a quick > assist. This would also be useful if you already have some text that you need > to escape: You could just select it, press Ctrl+1, and choose the quick assist. > > On paste, the quick assist popup would just open automatically at the end of > the pasted string. But in this automatic mode, it should not need the pasted > string to be selected. I like that!
(In reply to comment #19) > > Hmm, maybe we should just drop the linked mode and implement this as a quick > > assist. This would also be useful if you already have some text that you need > > to escape: You could just select it, press Ctrl+1, and choose the quick assist. > > > > On paste, the quick assist popup would just open automatically at the end of > > the pasted string. But in this automatic mode, it should not need the pasted > > string to be selected. > I like that! I also agree to this :) (In reply to comment #18) > > You want the popup to come whenever there is a \ in the text ? Even in strings > > like "Hello \n World", "Hello \t World" etc? Note that in these 2 strings the > > user almost never wants to escape \es. > > Why not? If the string is a path or an example text, then escapes also make > sense there. Again, it's about predictability and not surprising the user. The automatic popup should come when the code thinks that there is an excellent chance of the user wanting to escape the text. A properties file would contain text like '\uxxxx', 'C:\\abc.txt' , 'Hello \n World' - this text is already escaped and it is very likely that user does not want to do anything with it so why bother the user with a popup. Plus with the quick assist in place, even if the code makes a wrong guess the user can take a corrective action.
> The automatic popup should come when the code thinks that there is an excellent > chance of the user wanting to escape the text. A properties file would contain > text like '\uxxxx', 'C:\\abc.txt' , 'Hello \n World' - this text is already > escaped and it is very likely that user does not want to do anything with it so > why bother the user with a popup. Just to add to this... If and when we implement this feature in Java Editor as well, the popup should come only when the pasted string would actually be wrong and the JE would complain about it with an error 'Invalid escape sequence'. Its just that Properties file editor does not show an error but will silently ignore invalid escape sequences.
Created attachment 183841 [details] quick assist implementaiton Patch does the following 1) Adds 2 quick assists - Escape Backslashes, Unescape Backslashes 2) Ctrl+1 => If some text is selected in the editor then the quick assists work on the selection 3) Ctrl+1 => If no text is selected then the quick assists work on the 'value' or the 'key' in which the caret is placed. (I think the quick assists are good and we can go ahead with these irrespective of the auto-popup feature) 4) On paste the 'Escape Backslashes' quick assist is popped up if it is interesting. With this patch the logic for escape on paste is a bit messed up, however it works nicely for simple cases e.g. "C:\Program Files\Java". Also in case of the multi-line paste the popup covers all pasted lines except the first one. Markus can you please try this patch to see the look and feel of this UI.
I like the quick assists and the auto-popup. > Also in case of the multi-line paste the popup covers all pasted lines except > the first one. I take that as known bug, and the fix will be to show the popup below the last of the selected lines, right? This seems to be an old bug in the quick fix infrastructure (also happens e.g. in the Java editor). Please file a separate bug for this and look into a solution. This was not a big problem before, but with the auto-popup, it's much more visible. Assorted problems I saw in the UI (did not look at the code yet): - Spelling correction quick fixes don't work any more - When I paste "\u26" (without quotes), I'd expect to get the auto-popup (since the escape is illegal) - Could you open the auto-assist popup at the caret position, rather than at beginning of the pasted string? That would make the behavior the same as when the user invokes Quick Assist. Without a selection, the positioning is hard to understand.
I also like the direction and agree with Markus's comments. Since most people will want to escape the pasted string, it's easy to do by pressing 'Enter'. If the quick fix is used in 50% or less cases then I wonder whether having to press 'Esc' is good. In those cases we should probably show both actions, so that I can continue by pressing ('DownArrow'/'Tab' key + ) 'Enter'. One more thing I noticed: the quick fix is not coming when invoking on the last line of the file (not line delimiter).
Created attachment 184015 [details] fix + tests v0.9 This patch should be good. - Multi line paste works nicely (auto-assist popup opens at the caret position) - Spelling correction quick fixes work Do we need a preference to let users turn off this auto-popup feature?
Created attachment 184025 [details] fix + tests v0.9 Sorry, the last patch did not include the tests. (In reply to comment #24) > One more thing I noticed: the quick fix is not coming when invoking on the last > line of the file (not line delimiter). The last line does not have a line delimiter at the end (it would not be the last line if it had), and you invoked the quick assist after the last character on the line ?
> (In reply to comment #24) > > One more thing I noticed: the quick fix is not coming when invoking on the last > > line of the file (not line delimiter). > The last line does not have a line delimiter at the end (it would not be the > last line if it had), and you invoked the quick assist after the last character > on the line ? Yes. Have this in the file: c:\foo\bar c:\foo\bar<file ends here> If you do Ctrl+1 at the end of line 1 you get quick assists, but not when you do the same at the end of line 2.
(In reply to comment #27) > Yes. Have this in the file: > c:\foo\bar > c:\foo\bar<file ends here> > If you do Ctrl+1 at the end of line 1 you get quick assists, but not when you > do the same at the end of line 2. This is because of the way document partitions are set up. I do not have a solution for this right now. But I have added bug 328707 comment 5 , so that this could be looked into in future.
(In reply to comment #26) > Created an attachment (id=184025) [details] [diff] > fix + tests v0.9 - copyright of PropertiesCorrectionProcessor should be only 2010 - Why do we need EscapeBackslashCompletionProposal and UnescapeBackslashCompletionProposal? One class with a boolean to toggle should be enough. - What's up with the FIXME in PropertiesCorrectionProcessor? - The preview should show newlines and should not render HTML, e.g. k=This is a bold <b>back\slash</b>. x: second \n prop => See NLSStringHover#toHtml(String, String) or make sure you create a DefaultInformationControl that doesn't process simple HTML. - quick assist doesn't show up when selection starts inside spelling error, e.g. with selection from after '=' to end of line: key=Festbladde\eclipse - quick fix popup doesn't save/restore shell size. See new code in JavaSourceViewerConfiguration.getQuickAssistAssistant(ISourceViewer) - We need a consistent strategy when pasting keys and continuation characters. E.g. paste this: ------------------ key=value\ second li\ne ------------------ => The quick assist escapes both \s, but doesn't escape the '='. What are the real use cases we try to address? If the whole clipboard should be considered as a key, then we should also escape the '=' and the newline character. When I paste this as the value of another key, then I would expect the newlines to be converted to \n. (I think the patch is fine if we just wanted to escape '\', but that's IMO not a relevant use case).
Created attachment 184574 [details] fix + tests v0.91 (In reply to comment #29) > - The preview should show newlines and should not render HTML, e.g. > k=This is a bold <b>back\slash</b>. > x: second \n prop > > => See NLSStringHover#toHtml(String, String) or make sure you create a > DefaultInformationControl that doesn't process simple HTML. Created a DefaultInformationControl that doesen't process simple HTML. Markus there was a bug related to continuation characters with the previous patch. e.g if you paste the following, we should not auto show the quick assist as the backslash is a valid continuation character ----------------- key=value\ second line ----------------- > - We need a consistent strategy when pasting keys and continuation characters. > E.g. paste this: > ------------------ > key=value\ > second li\ne > ------------------ > => The quick assist escapes both \s, but doesn't escape the '='. What are the > real use cases we try to address? If the whole clipboard should be considered > as a key, then we should also escape the '=' and the newline character. When I > paste this as the value of another key, then I would expect the newlines to be > converted to \n. (I think the patch is fine if we just wanted to escape '\', > but that's IMO not a relevant use case). I am not sure how would we reliably detect if the user is pasting a value, or a key, or a key value pair, or multiple key value pairs. Maybe another quick assist can be provided to convert selected/pasted text into a value or key ? This quick assist can escape any whitespace characters, key value separators, continuation characters. But I do not think that this is too interesting. More interesting could be to provide another quick assist to escape \es only in invalid escape sequences. Currently we escape *all* \es in the pasted/selected text. e.g. on pasting ----------------- key=value\ secon\d li\ne ----------------- Escape selectively=> ----------------- key=value\ secon\\d li\ne ----------------- Escape all=> ----------------- key=value\\ secon\\d li\\ne -----------------
Created attachment 184585 [details] fix + tests v1.0 Some more minor tweaks as suggested by Markus. Plus I added a verify key listener in PropertiesFileAutoEditStrategy, hence if you quickly type something /move caret after a paste you do not see the popup. (no document change listener was needed)
Created attachment 184598 [details] fix + tests v1.0 Added document change listener plus added code to remove the listeners.
(In reply to comment #32) > Created an attachment (id=184598) [details] [diff] > fix + tests v1.0 +1 for the second v1.0. Dani, please have a look whether you're OK with the UI.
Looks good to me. Found bug 331914 and bug 331916 but those are not blocking us from putting this into M4.
Created attachment 184619 [details] final fix Same as last patch, just moved the tests from o.e.j.ui.tests to o.e.j.text.tests.
Fixed in HEAD.
Verified in I20101206-1800.
(In reply to comment #37) > Verified in I20101206-1800.
