To comment on the following update, log in, then open the issue:
http://www.openoffice.org/issues/show_bug.cgi?id=47233
User er changed the following:
What |Old value |New value
================================================================================
Target milestone|OOo 2.0.4 |OOo 2.x
--------------------------------------------------------------------------------
------- Additional comments from [EMAIL PROTECTED] Tue Aug 1 11:50:36 -0700
2006 -------
Hi Muthu,
Sorry, after vacation again one thing on the to-do list that was buried
under a pile of other things..
So, first of all, yes, great you accomplished all the IDL/API thingies!
Now on to my comments, following the patch top-down:
0. The SISSL (Sun Industry Standards Source License) isn't in use
anymore, all source code is LGPL only now. Please use the license
headers found in other files.
1. The API lacks a locale handling, this was primarily the reason to
move the code to i18npool. I suggest to
- add a com::sun::star::lang::Locale parameter to
XOrdinalSuffix::getOrdinalSuffix()
- implement handling according to the Locale.Language (only "en" in
this case), returning an empty string if the handling is unknown to
the method.
2. Regarding the naming of class OrdinalSuffixImpl: it isn't necessary
to add an ...Impl suffix there, as for UNO component libraries only
the three administrative functions are exported anyway. So for better
readability use OrdinalSuffix instead.
3. The integer types used in the API implementation must be fixed-size
to compile on different platforms, use sal_Int32 instead of the plain
C type 'long'. Note that in the IDL definition the types used (long,
string, ...) are language independent abstract types, in contrast to
language dependent implementation.
4. In the makefile.mk the two lines
.INCLUDE : svpre.mk
.INCLUDE : sv.mk
are superfluous, in already existing makefiles a legacy leftover,
please remove.
5. const sal_Char cOrdinalSuffix[] = "com.sun.star.i18n.OrdinalSuffixImpl";
This string must represent the _service_ name if used with
supportsService() and getSupportedServiceNames(). The
getImplementationName() may return a different string, but usually
does not if there is only one implementation for a service. So please
change to
const sal_Char cOrdinalSuffix[] = "com.sun.star.i18n.OrdinalSuffix";
6. Great you also created the xml/OrdinalSuffix.xml file, not many
developers even find that directory ;-) However,
- <name>com.sun.star.i18n.OrdinalSuffixImpl</name>
should be the service name, as again the implementation name is the
service name here.
- <supported-service>com.sun.star.i18n.OrdinalSuffixImpl</supported-service>
this MUST be the service name instead.
- <project-build-dependency>tools</project-build-dependency>
this is a superfluous dependency, no i18npool component code shall
depend on module 'tools'. In fact I forgot to remove that from the
other files in that directory when I cleaned out 'tools' code from
the i18npool module. Thanks for reminding me :)
- <runtime-module-dependency>tools</runtime-module-dependency>
same here, module 'tools' isn't needed.
7. sc/source/core/data/table4.cxx
::cppu::defaultBootstrap_InitialComponentContext() ); //@todo get context
from calc if that has one
Not really ;-)
You don't need the XComponentContext interface because of the next
point:
8. Reference< XMultiServiceFactory >
xServiceManager(::comphelper::getProcessServiceFactory());
Please obtain the service manager from the document instead:
pDocument->GetServiceManager();
9. To be congruent with the API please change the 'long' types used in
sc/source/core/data/table4.cxx to sal_Int32 instead.
Well, I think that's it. Note that I just browsed your patch and didn't
try to compile nor run anything, so there may be more to it.
Anyway, thanks, and keep on!
Eike
---------------------------------------------------------------------
Please do not reply to this automatically generated notification from
Issue Tracker. Please log onto the website and enter your comments.
http://qa.openoffice.org/issue_handling/project_issues.html#notification
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
