Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 358384

Summary: [quick assist][nls tooling] Add Quick Assists to synchronize Java properties file and corresponding message class
Product: [Eclipse Project] JDT Reporter: Deepak Azad <deepakazad>
Component: TextAssignee: Deepak Azad <deepakazad>
Status: VERIFIED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: daniel_megert, deepakazad, gdtaylor, raksha.vasisht, vciontu
Version: 3.7Flags: daniel_megert: review-
daniel_megert: review+
Target Milestone: 3.8 M3   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
fix
none
fix+tests none

Description Deepak Azad CLA 2011-09-21 05:35:56 EDT 
Discussion between Grant and Dani
"
Grant Taylor: When I edit a messages.properties file, I would like to have a way to insert a variable in the corresponding Messages.java file (or vice-versa). Maybe a "Sync Messages.java with messages.properties" menu item on either file in the editor, so that new entries in one file are created in the other (maybe also an option to put entries Messages.java in the same order so that new entries can be put in the same location in both files). Another option would be to right click an entry in messages.properties and select "Create variable in Messages.java", and vice-versa in Messages.java - "Create property in messages.properties".

Daniel Megert: The natural way of externalizing strings is to first use them in the code and when ready externalize them using the Externalize Strings wizard. Do you know that wizard? If so, what does not work for you with the described work flow?

Grant Taylor: The external wizard is known by us. We regard this more as a broad-based tool. If we are making a small change, then we usually add the string to the properties file first, then change the Messages.java file, then make use of the variable in the code. The second step is always annoying... we have search for the same spot in the Messages.java file (if its organization is the same as the properties file) and then carefully copy the key from the properties file. If there were an option to do it straight from the properties editor, it would save time and be less error-prone. So it's kind of like a "light" version of the wizard, or a supplemental/alternative approach.

Daniel Megert: I see. Would it help if you could invoke the Externalize Strings. wizard even if all strings are externalized and then be able to add new strings in the wizard?

Another approach could be to offer Quick Assist that allow to add the corresponding entry in the *.java or *.properties file.

Grant Taylor: I think the quick assist could be the desired approach here.  The idea is really to add the ability to easily and quickly keep the .properties and .java files in synch, with entries appearing in the same position (or relative position).
"
Comment 1 Deepak Azad CLA 2011-09-21 05:50:48 EDT 
The quick assist solution could also be useful while moving strings from one properties file to another. 

Messages.java and messages.properties can go out of sync as a result of
- new entry in Messages.java or messages.properties - a quick assist can create the corresponding entry in the other file.
- deletion of an entry in Messages.java or messages.properties - This is probably same as bug 183334.
- change of order of an entry in Messages.java or messages.properties - not sure if we want to do anything here.
Comment 2 Deepak Azad CLA 2011-09-29 06:07:13 EDT 
Here is my proposed solution

Messages.java and messages.properties need to be synced only for Eclipse style NLS.

We can add the following two quick assists in properties editor 
- Select one or more key value pairs and create the corresponding fields in Messages.java
- Select one or more key value pairs and remove them along with corresponding fields in Messages.java. In case a string is still in use, an error message can be shown to the user via a dialog or maybe via the status line.

Note that for these quick assists to work the names of the properties file and the accessor class have to be similar e.g. messages.properties and Messages.java.  Dissimilar names like abc.properties and XYZ.java will not work. This is similar to the behavior when a properties file is selected and 'Find broken externalized strings' action is invoked. This is because there is no information in the properties file about the accessor class. (I can try to do a search for accessor classes and then inspect it to find the correct match, but this may affect the responsiveness of the quick assists.)

I would not add a quick assists to Messages.java, because the user will still need to go to properties file to enter the actual string. 

Grant, Dani the above proposal looks ok to you?
Comment 3 Grant Taylor CLA 2011-09-29 08:42:04 EDT 
That proposal sounds great.
Comment 4 Dani Megert CLA 2011-10-04 03:34:29 EDT 
> We regard this more as a
> broad-based tool. If we are making a small change, then we usually add the
> string to the properties file first, then change the Messages.java file, then
> make use of the variable in the code. 

Grant, one question: why don't you simply start to use the string in the code and then use the Quick Fix to open the NLS wizard which lets you enter the new definition easily and quickly?
Comment 5 Dani Megert CLA 2011-10-04 03:34:39 EDT 
> Grant, Dani the above proposal looks ok to you?
Yes, looks good to me.
Comment 6 Grant Taylor CLA 2011-10-04 09:10:08 EDT 
(In reply to comment #4)
> > We regard this more as a
> > broad-based tool. If we are making a small change, then we usually add the
> > string to the properties file first, then change the Messages.java file, then
> > make use of the variable in the code. 
> 
> Grant, one question: why don't you simply start to use the string in the code
> and then use the Quick Fix to open the NLS wizard which lets you enter the new
> definition easily and quickly?

I guess this wasn't an obvious way to do it for us.  For developers who "know" what they are doing, it's natural to add the string to the translation file right away, without ever putting a non-translated string into the code (that would be introducing a potential problem if you forget to externalize it).  I do see your point that the externalize wizard and quick fixes could be leveraged in such a way to do what we need.  However, I find that many of us naturally work in a way that would make this new feature useful.
Comment 7 Deepak Azad CLA 2011-10-18 11:32:02 EDT 
Created attachment 205432 [details]
fix

The patch adds 3 quick assists to properties file if Eclipse style NLS is used
- Create field in 'Messages.Java' - Creates one or more fields at a time depending on selection
- Remove key - Removes one or more keys at a time depending on selection (this just deletes the key from the properties file and the corresponding field from Messages.java)
- Rename key - Rename one key at a time

TODOs:
- Add tests (I am working on this)
- Improve the additional proposal info, or do not show anything. Not sure what is the best approach here.

Dani, comments?
Comment 8 Dani Megert CLA 2011-10-19 15:38:27 EDT 
> - Improve the additional proposal info, or do not show anything. Not sure what
> is the best approach here.
Showing nothing is not a good idea. It should at least give a hint what it does. A preview is not needed.


I only tried the UI but didn't have time to do a full code review (sorry). The overall direction looks good. Issues I found while testing it:

1) Oops: doesn't look like it works when I choose custom names for the class and 
   properties file.

2) Quick Assists (e.g. rename) should not save the editor if it is dirty. We
   also don't do this when renaming a Java member.

3) An IAE (not sure how to reproduce):

!ENTRY org.eclipse.ui 4 4 2011-10-19 21:26:46.720
!MESSAGE "Quick Fix" did not complete normally.  Please see the log for more information.

!ENTRY org.eclipse.ui 4 0 2011-10-19 21:26:46.729
!MESSAGE java.lang.IllegalArgumentException
!STACK 0
java.lang.IllegalArgumentException
	at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:194)
	at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2031)
	at org.eclipse.jdt.internal.corext.refactoring.nls.AccessorClassModifier.getNewFinalStringFieldDeclaration(AccessorClassModifier.java:331)
	at org.eclipse.jdt.internal.corext.refactoring.nls.AccessorClassModifier.addKey(AccessorClassModifier.java:291)
	at org.eclipse.jdt.internal.corext.refactoring.nls.AccessorClassModifier.addKey(AccessorClassModifier.java:282)
	at org.eclipse.jdt.internal.corext.refactoring.nls.AccessorClassModifier.addFields(AccessorClassModifier.java:191)
	at org.eclipse.jdt.internal.ui.propertiesfileeditor.PropertiesCorrectionProcessor.getCreateFieldsInAccessorClassProposals(PropertiesCorrectionProcessor.java:287)
	at org.eclipse.jdt.internal.ui.propertiesfileeditor.PropertiesCorrectionProcessor.collectAssists(PropertiesCorrectionProcessor.java:183)
	at org.eclipse.jdt.internal.ui.propertiesfileeditor.PropertiesCorrectionProcessor.computeQuickAssistProposals(PropertiesCorrectionProcessor.java:113)
	at org.eclipse.jface.text.quickassist.QuickAssistAssistant$ContentAssistProcessor.computeCompletionProposals(QuickAssistAssistant.java:75)
	at org.eclipse.jface.text.contentassist.ContentAssistant.computeCompletionProposals(ContentAssistant.java:1830)
	at org.eclipse.jface.text.contentassist.CompletionProposalPopup.computeProposals(CompletionProposalPopup.java:556)
	at org.eclipse.jface.text.contentassist.CompletionProposalPopup.access$16(CompletionProposalPopup.java:553)
	at org.eclipse.jface.text.contentassist.CompletionProposalPopup$2.run(CompletionProposalPopup.java:488)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:70)
	at org.eclipse.jface.text.contentassist.CompletionProposalPopup.showProposals(CompletionProposalPopup.java:482)
	at org.eclipse.jface.text.contentassist.ContentAssistant.showPossibleCompletions(ContentAssistant.java:1656)
	at org.eclipse.jface.text.quickassist.QuickAssistAssistant.showPossibleQuickAssists(QuickAssistAssistant.java:128)
	at org.eclipse.jface.text.source.SourceViewer.doOperation(SourceViewer.java:937)
	at org.eclipse.ui.texteditor.TextOperationAction$1.run(TextOperationAction.java:128)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:70)
	at org.eclipse.ui.texteditor.TextOperationAction.run(TextOperationAction.java:126)
	at org.eclipse.jface.action.Action.runWithEvent(Action.java:498)
	at org.eclipse.ui.commands.ActionHandler.execute(ActionHandler.java:185)
	at org.eclipse.ui.internal.handlers.LegacyHandlerWrapper.execute(LegacyHandlerWrapper.java:109)
	at org.eclipse.core.commands.Command.executeWithChecks(Command.java:476)
	at org.eclipse.core.commands.ParameterizedCommand.executeWithChecks(ParameterizedCommand.java:508)
	at org.eclipse.ui.internal.handlers.HandlerService.executeCommand(HandlerService.java:169)
	at org.eclipse.ui.internal.keys.WorkbenchKeyboard.executeCommand(WorkbenchKeyboard.java:468)
	at org.eclipse.ui.internal.keys.WorkbenchKeyboard.press(WorkbenchKeyboard.java:786)
	at org.eclipse.ui.internal.keys.WorkbenchKeyboard.processKeyEvent(WorkbenchKeyboard.java:885)
	at org.eclipse.ui.internal.keys.WorkbenchKeyboard.filterKeySequenceBindings(WorkbenchKeyboard.java:567)
	at org.eclipse.ui.internal.keys.WorkbenchKeyboard.access$3(WorkbenchKeyboard.java:508)
	at org.eclipse.ui.internal.keys.WorkbenchKeyboard$KeyDownFilter.handleEvent(WorkbenchKeyboard.java:123)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Display.filterEvent(Display.java:1262)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1052)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1077)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1062)
	at org.eclipse.swt.widgets.Widget.sendKeyEvent(Widget.java:1104)
	at org.eclipse.swt.widgets.Widget.sendKeyEvent(Widget.java:1100)
	at org.eclipse.swt.widgets.Widget.wmKeyDown(Widget.java:1809)
	at org.eclipse.swt.widgets.Control.WM_KEYDOWN(Control.java:4892)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4560)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:341)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:4985)
	at org.eclipse.swt.internal.win32.OS.CallWindowProcW(Native Method)
	at org.eclipse.swt.internal.win32.OS.CallWindowProc(OS.java:2427)
	at org.eclipse.swt.internal.BidiUtil.windowProc(BidiUtil.java:639)
	at org.eclipse.swt.internal.win32.OS.DispatchMessageW(Native Method)
	at org.eclipse.swt.internal.win32.OS.DispatchMessage(OS.java:2533)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3752)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2701)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2665)
	at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2499)
	at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:679)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:668)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:123)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:352)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:601)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:624)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:579)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1431)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1407)
Comment 9 Deepak Azad CLA 2011-10-20 08:34:27 EDT 
Created attachment 205620 [details]
fix+tests

This patch should be good to go.

(In reply to comment #8)
> > - Improve the additional proposal info, or do not show anything. Not sure what
> > is the best approach here.
> Showing nothing is not a good idea. It should at least give a hint what it
> does. A preview is not needed.
I reused the code from CUCorrectionProposal to show a nice looking proposal info for 'Create field...' and 'Remove key' quick assists. 
 
> 1) Oops: doesn't look like it works when I choose custom names for the class
> and 
>    properties file.
As discussed on ST, will open a bug for this. (See also comment 2)
 
> 2) Quick Assists (e.g. rename) should not save the editor if it is dirty. We
>    also don't do this when renaming a Java member.
This was an old problem in NLSAccessorFieldRenameParticipant. 
 
> 3) An IAE (not sure how to reproduce):
Fixed.

The tests use a o.e.osgi jar stub (for NLS class), this jar is not included in the patch.
Comment 10 Dani Megert CLA 2011-10-21 04:14:41 EDT 
The additional info puts the "..." after the last entry if there is no last line delimiter.


> The tests use a o.e.osgi jar stub (for NLS class), this jar is not included in
> the patch.
Please attach the JAR, so that I can look at it and verify that the tests pass. Also, please extract the stuff which has warnings into a method and deprecate it, so that the CU does not have warnings.


> > 1) Oops: doesn't look like it works when I choose custom names for the class
> > and 
> >    properties file.
> As discussed on ST, will open a bug for this. (See also comment 2)
Please open the bug and let it depend on bug 361535.
Comment 11 Dani Megert CLA 2011-10-21 04:18:37 EDT 
(In reply to comment #10)
Forgot to say: otherwise it looks good and can be committed.
Comment 12 Deepak Azad CLA 2011-10-21 05:00:25 EDT 
Patch + jar committed to master - 08d4c4c84b31f4e626a33a1c55bc38d5a9625e1f

The patch had a small bug - the quick assist can also show up if Ctrl+1 is pressed with the caret on an empty line between 2 keys. Fix + tests for this are included in the commit.

(In reply to comment #10)
> The additional info puts the "..." after the last entry if there is no last
> line delimiter.
I am taking a look at this.

> Also, please extract the stuff which has warnings into a method and deprecate
> it, so that the CU does not have warnings.
Done.
 
> > > 1) Oops: doesn't look like it works when I choose custom names for the class
> > > and 
> > >    properties file.
> > As discussed on ST, will open a bug for this. (See also comment 2)
> Please open the bug and let it depend on bug 361535.
I opened bug 361535 for this problem only. Why do we need another bug?
Comment 13 Dani Megert CLA 2011-10-21 05:22:22 EDT 
(In reply to comment #12)
> Patch + jar committed to master - 08d4c4c84b31f4e626a33a1c55bc38d5a9625e1f
Thanks!

The stub should be smaller (just the class def I guess).
 
> > > > 1) Oops: doesn't look like it works when I choose custom names for the 
> > > > and 
> > > >    properties file.
> > > As discussed on ST, will open a bug for this. (See also comment 2)
> > Please open the bug and let it depend on bug 361535.
> I opened bug 361535 for this problem only. Why do we need another bug?
Then please change the summary. "Improve accuracy" doesn't clearly indicate that since currently it just does not work.
Comment 14 Deepak Azad CLA 2011-10-21 05:50:30 EDT 
Additional fix - 3afa999ca8c8a881ac8e71fb02bcd90c439fa644

(In reply to comment #13)
> The stub should be smaller (just the class def I guess).
Done, 5 kb now.

(In reply to comment #12)
> (In reply to comment #10)
> > The additional info puts the "..." after the last entry if there is no last
> > line delimiter.
> I am taking a look at this.
Fixed.
Comment 15 Dani Megert CLA 2011-10-21 09:25:37 EDT 
> (In reply to comment #13)
> > The stub should be smaller (just the class def I guess).
> Done, 5 kb now.
Look at how it looks now :-)
Comment 16 Raksha Vasisht CLA 2011-10-25 06:23:10 EDT 
Question: 

Any reason why we don't use the standard 'Rename in workspace' image and shorcut 
for the proposal the properties file?
Also, the text in addl info box shows 'Start the Rename refactoring' not 'Start Rename refactoring'
Comment 17 Deepak Azad CLA 2011-10-25 06:34:05 EDT 
(In reply to comment #16)
> Any reason why we don't use the standard 'Rename in workspace' image 
The 'standard' image indicates that refactoring will be started in 'linked' mode, which is not the case here.

> and shorcut for the proposal the properties file?
This could probably be done.
Comment 18 Raksha Vasisht CLA 2011-10-25 06:52:30 EDT 
Remove Key(s) -> addl info is not very clear that it is modifying 2 different files. We use '...'  to show modifications in different parts of the same file for other proposals. I'm not sure if we also use it to show changes in 2 different files already, but I think we probably should indicate it,  ex: mention the file names, in that case.
Comment 19 Dani Megert CLA 2011-10-25 06:56:36 EDT 
(In reply to comment #16 and comment #18)

Raksha, please file separate bugs for issues you think should be tracked. We won't use this bug as bin for all further changes to this feature.
Comment 20 Raksha Vasisht CLA 2011-10-25 08:33:41 EDT 
(In reply to comment #19)
> (In reply to comment #16 and comment #18)
> 
> Raksha, please file separate bugs for issues you think should be tracked. We
> won't use this bug as bin for all further changes to this feature.

Filed bug 361916.
Comment 21 Raksha Vasisht CLA 2011-10-25 08:55:21 EDT 
Verified for Juno M3 with 4.2 build I20111021-1625. 

Also see bug 361920.
Comment 22 Dani Megert CLA 2011-11-01 03:34:37 EDT 
Grant, please verify it in Juno M3.
Comment 23 Dani Megert CLA 2011-12-12 06:28:09 EST 
(In reply to comment #22)
> Grant, please verify it in Juno M3.

Ping!
Comment 24 vciontu CLA 2011-12-14 09:59:04 EST 
(In reply to comment #23)
> (In reply to comment #22)
> > Grant, please verify it in Juno M3.
> Ping!

Hello!

I have verified it in Juno M3 and 3.8M4.
I have opened bug 366707. 
Besides this, the feature looks good and useful.

Thank-you.
Comment 25 Dani Megert CLA 2011-12-14 10:11:10 EST 
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Grant, please verify it in Juno M3.
> > Ping!
> 
> Hello!
> 
> I have verified it in Juno M3 and 3.8M4.
> I have opened bug 366707. 
> Besides this, the feature looks good and useful.
> 
> Thank-you.

Thanks for the feedback Val.