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

Bug 361535

Summary: [nls tooling] Messages.java is not found from messages.properties in all cases.
Product: [Eclipse Project] JDT Reporter: Deepak Azad <deepakazad>
Component: TextAssignee: Deepak Azad <deepakazad>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P2 CC: daniel_megert, deepakazad
Version: 3.8Flags: daniel_megert: review-
daniel_megert: review+
Target Milestone: 3.8 M6   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
fix+test
none
fix+test none

Description Deepak Azad CLA 2011-10-20 08:45:58 EDT 
From bug 358384 comment 2.

> 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.)
 
Possible solution - when we open the editor we start a job that finds the corresponding message class in the background.
Comment 1 Deepak Azad CLA 2011-12-05 03:27:18 EST 
Unfortunately I was unable to finish work on this.
Comment 2 Deepak Azad CLA 2012-01-23 05:20:30 EST 
Created attachment 209900 [details]
fix+test

The patch should be good to go in terms of functionality. However, Dani can you please review the change in PropertiesFileEditor.doSetInput(IEditorInput), I am not 100% sure if this is the right place to invoke the new job.
Comment 3 Deepak Azad CLA 2012-01-23 07:46:17 EST 
(In reply to comment #2)
> Created attachment 209900 [details] [diff]
> fix+test

I tested this patch with o.e.jdt.ui source, more specifically with 
/org.eclipse.jdt.ui/ui refactoring/org/eclipse/jdt/internal/ui/refactoring/refactoringui.properties (The corresponding accessor class is RefactoringMessages.java). In this case 54 compilation units, in the same package, are inspected to be a match for refactoringui.properties. The patch works without any issues.
=> It should be safe to release this for M5.
Comment 4 Dani Megert CLA 2012-01-23 11:41:48 EST 
(In reply to comment #2)
> Created attachment 209900 [details] [diff]
> fix+test
> 
> The patch should be good to go in terms of functionality. However, Dani can you
> please review the change in PropertiesFileEditor.doSetInput(IEditorInput), I am
> not 100% sure if this is the right place to invoke the new job.

The location is good.
Comment 5 Dani Megert CLA 2012-01-23 11:42:06 EST 
(In reply to comment #3)
> (In reply to comment #2)
> > Created attachment 209900 [details] [diff] [diff]
> > fix+test
> 
> I tested this patch with o.e.jdt.ui source, more specifically with 
> /org.eclipse.jdt.ui/ui
> refactoring/org/eclipse/jdt/internal/ui/refactoring/refactoringui.properties
> (The corresponding accessor class is RefactoringMessages.java). In this case 54
> compilation units, in the same package, are inspected to be a match for
> refactoringui.properties. The patch works without any issues.
> => It should be safe to release this for M5.

I disagree ;-)

1. Create 'p.properties' inside 'src'
   ==> CCE.
   People also use the Properties File editor outside package and even for files
   outside a Java project.

2. Jobs don't run sequentially. If the editor is reused and the first job takes
   very long, then it might overwrite the result of the second job.

3. The job runs / does work even if the editor got closed before the job started

4. Pending jobs aren't stopped when the editor is closed or the input changed.

Copyright dates also need an update.
Comment 6 Deepak Azad CLA 2012-01-23 12:02:03 EST 
(In reply to comment #5)
Alright, M6 it is.
Comment 7 Deepak Azad CLA 2012-01-26 08:02:58 EST 
Created attachment 210112 [details]
fix+test

This patch should fix all the problems mentioned in comment 5.
Comment 8 Dani Megert CLA 2012-02-01 05:23:08 EST 
(In reply to comment #7)
> Created attachment 210112 [details] [diff]
> fix+test
> 
> This patch should fix all the problems mentioned in comment 5.

Better, but still:

- A job is not a cheap resource. We should not schedule it if we already know
  that it's not computing something because the input is not an IFile.

- The job is now correctly cancelled but its implementation does not check
  the progress monitor for being cancelled: it simply computes everything as
  if nothing had happened.

- The sequence problem got fixed but there is still a window where 
  'fAccessorType' can be wrong. This window can be relatively big if the second
  job is not scheduled fast enough.
Comment 9 Deepak Azad CLA 2012-02-01 07:29:36 EST 
(In reply to comment #8)
Fixed in master - 3ebdf20161a85b3bd1ec911e75b24c1c9fbb7fa6

I think the fix should be good now. (I pushed the change to avoid the 'create patch apply patch' dance.)
Comment 10 Dani Megert CLA 2012-02-02 02:42:40 EST 
Sorry Deepak, but your code is still not safe. Take another look. Ping me if you don't see the hole.


Little issue: please fix the empty catch block.
Comment 11 Deepak Azad CLA 2012-02-07 12:25:45 EST 
38a9f9250543744fc04ae88b4dec86d9a2243b6b

That should do it.
Comment 12 Dani Megert CLA 2012-02-08 03:27:28 EST 
Looks good now.