Community
Participate
Working Groups
It's always irked me that NLS class derivatives have a hardcoded reference to their package name, thus making them fragile in the case of package refactoring. This adds a utility method that lets CDT and CDT clients avoid that. It also updates instances in the core plugin for reference. Will commit post-Helios unless someone objects.
Created attachment 171580 [details] Solution
Doug, please review.
Looks OK to me, but then I'm not an i18n expert. Is Vivian watching this bug?
NLS.initializeMessages(clazz.getPackage().getName() + ".messages", clazz); //$NON-NLS-1$ violates the prevailing convention that the properties file has the same name as the corresponding NLS file. It should be changed to: NLS.initializeMessages(clazz.getName(), clazz);
(In reply to comment #4) > NLS.initializeMessages(clazz.getPackage().getName() + ".messages", clazz); > //$NON-NLS-1$ > > violates the prevailing convention that the properties file has the same name > as the corresponding NLS file. It should be changed to: > NLS.initializeMessages(clazz.getName(), clazz); I don't believe that will work. In 100% of the cases I've seen, the properties file starts with a lowercase 'm', while the class name is uppercase. I'm pretty sure the resource bundle name is case sensitive. The utility method also specifically states in the javadoc that it is not completely general purpose but it caters to the very common scenario of having a "messages.properties" where the NLS derivative is.
Is CDT appropriate place for this solution? Shouldn't you ask the platform for setting standardized solution in NLS class?
(In reply to comment #5) In cdt.core + cdt.ui there are 36 messages.properties files and 36 SomeClassMessages.properties files. I prefer the latter naming convention that is cleaner in my opinion.
(In reply to comment #7) > (In reply to comment #5) > In cdt.core + cdt.ui there are 36 messages.properties files and 36 > SomeClassMessages.properties files. I prefer the latter naming convention that > is cleaner in my opinion. And you are certainly entitled to that opinion. But the fact is that there are many others who think differently (like me), and who will continue using the former regardless of your opinion. We can require them to hardcode the package name or we can help them out with a utility method. In fact, why not add a varant utility method to cater to your preference? I have no problem with that.
(In reply to comment #6) > Is CDT appropriate place for this solution? Shouldn't you ask the platform for > setting standardized solution in NLS class? It is an *adequate* place for it, IMO. I'm not going to ask the platform to standardize something when they've obviously designed it to be flexible, and it's unlikely they're going to add a utility to handle a scenario that has become common in practice. I don't think it's inappropriate to house something like this in CDT. I don't see why we can't make things a little easier for ourselves...and if we know CDT clients can benefit, too, expose it to them as well. I was initially going to add this to an internal package, but thought CDT clients would appreciate it if we expose it to them. My position is that CDT need not restrict its usefulness to CDT-isms, as long as we're not encouraging a violation of platform guidelines, or leading clients down some arbitrary path. The convention of having a Messages.class co-located with a messages.properties is extremely pervasive.
BTW, how does ICU4J fit into this. This has been an Eclipse requirement forever and I can't tell whether we satisfy it or not. Does that force us into a solution for message bundles?
(In reply to comment #10) > BTW, how does ICU4J fit into this. This has been an Eclipse requirement forever > and I can't tell whether we satisfy it or not. Does that force us into a > solution for message bundles? I don't see any overlap between ICU4J and this bugzilla.
(In reply to comment #11) > (In reply to comment #10) > > BTW, how does ICU4J fit into this. This has been an Eclipse requirement forever > > and I can't tell whether we satisfy it or not. Does that force us into a > > solution for message bundles? > > I don't see any overlap between ICU4J and this bugzilla. OK. Some day I'll try to figure out what ICU4J is. But I enjoy the entertainment of not meeting artificially created requirements from non-contributors :). BTW, whatever you do, don't break open declaration from the String to the value in the properties file. Without that message bundles are brutal to understand.
(In reply to comment #8) I think we should follow http://www.eclipse.org/articles/Article-Internationalization/how2I18n.html. Platform plugins are in agreement with it, org.eclipse.ui and org.eclipse.jdt.ui don't contain any messages.properties files.
(In reply to comment #13) > (In reply to comment #8) > I think we should follow > http://www.eclipse.org/articles/Article-Internationalization/how2I18n.html. > Platform plugins are in agreement with it, org.eclipse.ui and > org.eclipse.jdt.ui don't contain any messages.properties files. Externalize strings wizard suggests messages.properties by default, that's where they are coming from.
(In reply to comment #13) > (In reply to comment #8) > I think we should follow > http://www.eclipse.org/articles/Article-Internationalization/how2I18n.html. > > Platform plugins are in agreement with it, org.eclipse.ui and > org.eclipse.jdt.ui don't contain any messages.properties files. Am I overlooking something? I don't see where this document explicitly states a preference for using a common base filename for the NLS class and the properties file. That may be the case in the examples provided, but it is not stated as a guideline. That some plugins in the platform prefer that approach is fine. If they thought it an important-enough guideline to follow, they should have added it to their document.
(In reply to comment #12) > BTW, whatever you do, don't break open declaration from the String to the value > in the properties file. Without that message bundles are brutal to understand. Huh? You've lost me here.
(In reply to comment #15) The article, in general, is not written as a rigid set of rules. It starts with: Summary This article is a roadmap for writing Eclipse plug-ins destined for the international market. We'll begin with a brief review of the motivations and technical challenges of internationalization, followed by step-by-step instructions of how to internationalize your Eclipse plug-in. The example is part of the step-by-step instructions, isn't it?
(In reply to comment #17) > (In reply to comment #15) > The article, in general, is not written as a rigid set of rules. It starts > with: > Summary > This article is a roadmap for writing Eclipse plug-ins destined for the > international market. We'll begin with a brief review of the motivations and > technical challenges of internationalization, followed by step-by-step > instructions of how to internationalize your Eclipse plug-in. > > The example is part of the step-by-step instructions, isn't it? That's a stretch. Sorry, Sergey. You're not going to convince me. (a) Andrew pointed out the externalization wizard follows the 'messages.properties" convention. (b) There is no explicit guideline and you're trying to dissect and spin an introductory paragraph to support your position that it is a guideline. (c) I like the consistency of having a same-named file house the string properties in a package, just as I like to use "Activator.java" for a plugin's activator. Other developers apparently do, too. (d) I've found 146 instances of "messages.properties" in the platform plugins. Again, I'm not trying to sell you on my preference; I see no further point in having you sell me on yours. I've added a utility to cater to a pervasive, long-standing practice--one not only in CDT but in the platform. I don't see a problem.
(In reply to comment #18) > I've added a utility to cater to a pervasive, > long-standing practice--one not only in CDT but in the platform. I don't see a > problem. If we say that both naming conventions are legitimate, them NLSUtil should not give preference to one over another.
(In reply to comment #19) > (In reply to comment #18) > > I've added a utility to cater to a pervasive, > > long-standing practice--one not only in CDT but in the platform. I don't see a > > problem. > > If we say that both naming conventions are legitimate, them NLSUtil should not > give preference to one over another. Which is why I offered to add a variant for the other convention. Both benefit from not having to hardcode the package name.
(In reply to comment #20) > Which is why I offered to add a variant for the other convention. Both benefit > from not having to hardcode the package name. I'm ok with that.
Another thing to throw into the mix, how are we doing with Babel? It's an opportunity to get contributions to translations.
Created attachment 171592 [details] Updated solution; supports both common practices The utility method now takes a parameter to support both common use cases (see discussion above). I couldn't think of decent names for individual methods so I added a parameter and kept it to a single method.
(In reply to comment #23) How about initializeClassMessages and initializePackageMessages?
(In reply to comment #24) > (In reply to comment #23) > > How about initializeClassMessages and initializePackageMessages? I find that confusing, as it implies two distinct types of message sets: a set for a class, and a set for a package. According to the guidelines, there should be a single properties file per package, and the class scope is burned into the string property name, e.g: MyClass_hello_world AnotherClass_good_night
(In reply to comment #16) > (In reply to comment #12) > > BTW, whatever you do, don't break open declaration from the String to the value > > in the properties file. Without that message bundles are brutal to understand. > Huh? You've lost me here. Yeah, that's a real problem. Check this out: 0. Find a reference to one of the variables in one of Message class, for example Messages.EclipseVariablesVariableSupplier_illegal_variable (EclipseVariablesVariableSupplier.java:86). 1. Hover the mouse pointer over it. Before the patch I see "message.properties" and "Illegal usage of Eclipse variable: {0}". After the patch it just shows that the type is "String". 2. Hold Ctrl and then hover mouse pointer. Before patch you get suggestion "Open in 'message.properties'". If you click on it - it navigates right to the definition in 'message.properties'. After the patch it does not. 3. In 'message.properties', position the cursor on EclipseVariablesVariableSupplier_illegal_variable. Press F3. OK, this one still works. So does Ctrl-[mouse click]. Non-working cases 1) and 2) are no-go. I strongly suggest to submit a patch to platform so we are guaranteed to keep the tooling support. You don't know if it is going to be rejected or accepted.
(In reply to comment #26) > (In reply to comment #16) > > (In reply to comment #12) > > > BTW, whatever you do, don't break open declaration from the String to the value > > > in the properties file. Without that message bundles are brutal to understand. > > Huh? You've lost me here. > Yeah, that's a real problem. Check this out: > 0. Find a reference to one of the variables in one of Message class, for > example Messages.EclipseVariablesVariableSupplier_illegal_variable > (EclipseVariablesVariableSupplier.java:86). > 1. Hover the mouse pointer over it. Before the patch I see "message.properties" > and "Illegal usage of Eclipse variable: {0}". After the patch it just shows > that the type is "String". > 2. Hold Ctrl and then hover mouse pointer. Before patch you get suggestion > "Open in 'message.properties'". If you click on it - it navigates right to the > definition in 'message.properties'. After the patch it does not. > 3. In 'message.properties', position the cursor on > EclipseVariablesVariableSupplier_illegal_variable. Press F3. OK, this one still > works. So does Ctrl-[mouse click]. > > Non-working cases 1) and 2) are no-go. I strongly suggest to submit a patch to > platform so we are guaranteed to keep the tooling support. You don't know if it > is going to be rejected or accepted. That's not good. I'd vote -1 if this is the case.
(In reply to comment #27) > > Yeah, that's a real problem. Check this out: > > [snip] Crap. There's some real voodoo magic going on there. Simply add private static final String BUNDLE_NAME= "org.eclipse.cdt.internal.core.cdtvariables.messages"; //$NON-NLS-1$ to the Messages.java file and the feature is restored...even though 'BUNDLE_NAME' is not used anywhere. This tells me the feature is relying on some pretty fragile, heuristic methods for getting to the literals in the properties file. I guess I shouldn't be surprised by that, but it sucks to see a tooling feature get in the way of a code improvement. In other words, the JDT guys have pretty much forced us to hardcode the package name in the java file (or lose these powerful capabilities). Just doesn't seem right, but I understand the difficulties of trying to determine something at build-time (edit, really) that can only truly be established at run-time. At this point, I'm just going to chuck this down the shoot. I've already wasted too much time (mine and Sergey's) on this.
(In reply to comment #28) The things are actually not that bad. To see for yourself open TraceControlView.java and select TraceControlView_action_Refresh_label. Notice that both 1) and 2) work. Navigate to TracepointsMessages.java and notice that the initialization code is pretty decent: NLS.initializeMessages(TracepointsMessages.class.getName(), TracepointsMessages.class);
(In reply to comment #29) > (In reply to comment #28) > The things are actually not that bad. To see for yourself open > TraceControlView.java and select TraceControlView_action_Refresh_label. Notice > that both 1) and 2) work. Navigate to TracepointsMessages.java and notice that > the initialization code is pretty decent: > NLS.initializeMessages(TracepointsMessages.class.getName(), > TracepointsMessages.class); Of course my vote was conditional and not an outright veto :).
(In reply to comment #29) > (In reply to comment #28) > The things are actually not that bad. To see for yourself open > TraceControlView.java and select TraceControlView_action_Refresh_label. Notice > that both 1) and 2) work. Navigate to TracepointsMessages.java and notice that > the initialization code is pretty decent: > NLS.initializeMessages(TracepointsMessages.class.getName(), > TracepointsMessages.class); You are focusing on your particular preference of naming the .properties file after the NLS class. In that case, yes, it looks like the JDT features (1) and (2) work because it is able to predict what WhateverClass.class.getName() will return (not too difficult). I opened this bug primarily to address the other scenario--where there's a messages.properties file. Again there are MANY such cases. In that case, I see no way around having to hardcode the resource bundle name. Thus, my patch is still unusable.
(In reply to comment #31) I thought the primary concern was a hardcoded package name. This problem does have a solution, at least for property files named after classes. If you don't like this solution, you can close the bug.
(In reply to comment #32) > (In reply to comment #31) > > I thought the primary concern was a hardcoded package name. This problem does > have a solution, at least for property files named after classes. If you don't > like this solution, you can close the bug. I think it's a great technique. I'm saying it only addresses one approach. I took a survey and that approach is by far less common than the specific one I was trying to address. The instances of using "messages.properties" vs using the NLS class name differ by about a factor of 2X. Plus, you've somehow managed to change the purpose of the bug report (formally by changing the abstract line). I initially created the bug report in order to introduce a utility class to let us avoid hardcoding the package name in *new code*. I never intended to go through and change existing classes to use the utility, except that I did so for one plugin in order to provide some reference usage. What you've done is come up with a *technique* to avoid harcoding the package in the less common cases. It doesn't involve a new utility class, so there is no patch to contribute, and thus there really is no need for a bug. If you want to go through and convert all the existing classes to use your technique, I guess you could do that with a bug. I'm not going to do that. Are you? If not, then again, I don't see a case for an open bugzilla. What would be better is if you email the cdt-dev list and say, hey, in the future, you can use this technique to avoid hardcoding the package name. I've closed the bug. If you are going to sign up to update all existing usages that can be resolved with your technique, feel free to reopen it.
