Bug 135784 - need to adopt ICU4J APIs
Summary: need to adopt ICU4J APIs
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Runtime (show other bugs)
Version: 3.2   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 3.2 RC1   Edit
Assignee: platform-runtime-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 127876
  Show dependency tree
 
Reported: 2006-04-09 23:12 EDT by Karice McIntyre CLA
Modified: 2006-04-11 18:07 EDT (History)
3 users (show)

See Also:


Attachments
patch for org.eclipse.core.runtime.compatibility (6.72 KB, patch)
2006-04-09 23:14 EDT, Karice McIntyre CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Karice McIntyre CLA 2006-04-09 23:12:52 EDT
To complete the plan item to deliver ICU4J support in the Eclipse (see bug
127876) code must be migrated to use the com.ibm.icu.* classes in place of the
default java implementation.  This component is affected.  I will attach
patches with the changes required (only import changes at this point).  This
needs to be done for RC1.
Comment 1 Karice McIntyre CLA 2006-04-09 23:14:21 EDT
Created attachment 38109 [details]
patch for org.eclipse.core.runtime.compatibility

Are we allowed to reference ICU4J in this bundle?
Comment 2 Thomas Watson CLA 2006-04-10 10:01:28 EDT
There are concerns about adding such a large dependency to our core bundles.  Particularly for the eRCP project.  the ICU4J bundle is a whapping 3.2 meg bundle.  Is there any way this can be made optional?
Comment 3 Karice McIntyre CLA 2006-04-10 10:41:31 EDT
See 
http://wiki.eclipse.org/index.php/ICU4J#Replacement_Plug-in
Comment 4 Pascal Rapicault CLA 2006-04-10 11:47:31 EDT
Tom I don't see any problem with applying this patch as the dependency is added to  the runtime compatibility layer.
Comment 5 Thomas Watson CLA 2006-04-10 13:16:44 EDT
Fine with me.  I am more concerned about the core bundles adding this dependency.  I understand that there is a 100K icu.base bundle which can be used instead, this is good, but 100K can still be viewed as "expensive" in an embedded space.
Comment 6 Mark Rogalski CLA 2006-04-10 13:46:41 EDT
The ICU change was also made to org.eclipse.core.expressions which is part of eRCP. See defect 135774.
Comment 7 John Arthorne CLA 2006-04-11 16:01:07 EDT
Note that this patch only adds ICU usage in one place: org.eclipse.core.internal.plugins.Policy#bind.  I think this dependency could be avoided entirely by replacing the method's implementation with NLS#bind instead. I.e., as long as the reference to java.text.MessageFormat goes away, then runtime will not need ICU4J.
Comment 8 John Arthorne CLA 2006-04-11 16:13:15 EDT
I think in general for core we should only be inserting ICU4J dependency if it is actually needed.  After reviewing the patches for core resources and jobs I found there was no value in releasing it (see bug 135785).  I think the core.expressions dependency was also not needed.
Comment 9 Karice McIntyre CLA 2006-04-11 16:18:24 EDT
Why is the core.expressions patch not needed?  It uses ICUs NumberFormat.  
Comment 10 Karice McIntyre CLA 2006-04-11 16:19:53 EDT
If all the MessageFormat usages can go away by using the NLS class that's great.  Is that the case for all the usages of MessageFormat that you came across in the core resources and core runtime plugins?
Comment 11 DJ Houghton CLA 2006-04-11 16:20:48 EDT
Using NLS sounds good to me. Since the Policy class is internal we don't have to
worry about breaking anyone using complicated MessageFormat formats. (we don't
use them)

Ok to change? Any objections?

The core.expressions bundle is not owned by the runtime team.
Comment 12 John Arthorne CLA 2006-04-11 16:23:12 EDT
The reference to NumberFormat was only in LRUCache.toString, which is specified as for debugging purposes only.  I replaced:

buffer.append(NumberFormat.getInstance().format(fillingRatio()));

with:

buffer.append(fillingRatio());

Where fillingRatio() returns a double.
Comment 13 DJ Houghton CLA 2006-04-11 16:27:00 EDT
Ok, I talked this over with team members and we replaced the call to MessageFormat with one to NLS. (in the Policy.bind method)

So bug is fixed. (no dependancy on ICU4J)

Patch not applied.
Comment 14 Karice McIntyre CLA 2006-04-11 16:36:36 EDT
I will hunt down core.expressions and point to this bug.
Comment 15 Mark Rogalski CLA 2006-04-11 18:07:13 EDT
My thanks to all you for helping keep 100K out of the eRCP runtime. It's a significant amount and your responsiveness is greatly appreciated.