Community
Participate
Working Groups
In SystemViewResources, there are currently lots of Strings that are really specific to files, processes or shells respectively. As part of our efforts on downscaling / componentizing RSE, these should be moved into the files.ui / processes.ui / shells.ui plugins respectively. An (RCP) product based on RSE that does not want any of these subsystem implementations should not be burdened with the extra load of these resources. Some examples: RESID_PROPERTY_VIRTUALFILE_COMPRESSEDSIZE_LABEL RESID_PROPERTY_FILE_SIZE_VALUE ... Unfortunately, some of these constants are re-used in some dialogs; these need to be examined on a case by case basis.
TBD by M6 latest since NLS specific.
TBD before Feb.25 which is the first PII drop.
Created attachment 89413 [details] patch describing approach to migrating system messages out of systemmessages.xml I've attached a patch that is my start to providing messages without using systemmessages.xml. I've extended SystemMessage to provide SimpleSystemMessage. This new class can be constructed directly using IStatus severities and pre-formated strings for the level one and level two text. For the message component, subcomponent and number, I'm using hard-coded "RSE", "G" and "-" respectively. These are normally used for creating a unique ID for a system message that gets displayed as the title in the SystemMessageDialog. For the simple system message, this ID is not so useful. I'm considering using the plugin ID right now as part of the ID, however I'm not sure what to do about the dialog title. What do you think we should do here? In this patch, I've provided an implementation for the dstore connector service (i.e. moved message strings to a properties file and use SimpleSystemMessage with those strings). There are still several other plugins that this will need to be applied to but I want to get opinions on the approach before I proceed further.
(In reply to comment #3) > Created an attachment (id=89413) [details] > patch describing approach to migrating system messages out of > systemmessages.xml > > I've attached a patch that is my start to providing messages without using > systemmessages.xml. I've extended SystemMessage to provide > SimpleSystemMessage. This new class can be constructed directly using IStatus > severities and pre-formated strings for the level one and level two text. For > the message component, subcomponent and number, I'm using hard-coded "RSE", "G" > and "-" respectively. These are normally used for creating a unique ID for a > system message that gets displayed as the title in the SystemMessageDialog. > For the simple system message, this ID is not so useful. I'm considering using > the plugin ID right now as part of the ID, however I'm not sure what to do > about the dialog title. What do you think we should do here? If the message number is -1 then perhaps we should go with a generic title in the dialog based on the severity "Error", "Warning", ... > > In this patch, I've provided an implementation for the dstore connector service > (i.e. moved message strings to a properties file and use SimpleSystemMessage > with those strings). There are still several other plugins that this will need > to be applied to but I want to get opinions on the approach before I proceed > further. > I think we are supposed to be using the ICU MessageFormat class, not the one from java.text. I think "startsWith" also may pose a problem depending on the translated language - but perhaps not for the cases you are using it.
(In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=89413) [details] [details] > > patch describing approach to migrating system messages out of > > systemmessages.xml > > > > I've attached a patch that is my start to providing messages without using > > systemmessages.xml. I've extended SystemMessage to provide > > SimpleSystemMessage. This new class can be constructed directly using IStatus > > severities and pre-formated strings for the level one and level two text. For > > the message component, subcomponent and number, I'm using hard-coded "RSE", "G" > > and "-" respectively. These are normally used for creating a unique ID for a > > system message that gets displayed as the title in the SystemMessageDialog. > > For the simple system message, this ID is not so useful. I'm considering using > > the plugin ID right now as part of the ID, however I'm not sure what to do > > about the dialog title. What do you think we should do here? > If the message number is -1 then perhaps we should go with a generic title in > the dialog based on the severity "Error", "Warning", ... > > > > In this patch, I've provided an implementation for the dstore connector service > > (i.e. moved message strings to a properties file and use SimpleSystemMessage > > with those strings). There are still several other plugins that this will need > > to be applied to but I want to get opinions on the approach before I proceed > > further. > > > I think we are supposed to be using the ICU MessageFormat class, not the one > from java.text. I think "startsWith" also may pose a problem depending on the > translated language - but perhaps not for the cases you are using it. What class might this ICU MessageFormat be? I'm assuming this is something we can use in an Eclipse open source project.
ICU MessageFormat should only be used in UI plugins. Non-UI plugins should try to save resources by using NLS.bind() instead. To date, we aren't using ICU MessageFormat anywhere, only ICU DateFormat and NumberFormat. An example of those is in SystemViewRemoteFileAdapter. Note that the import of com.ibm.icu.* requires a change in the related MANIFEST.MF.
(In reply to comment #4) > (In reply to comment #3) > If the message number is -1 then perhaps we should go with a generic title in > the dialog based on the severity "Error", "Warning", ... The current message dialog uses the following form for it's title: <component><subcomponent><number><status>. For example: "RSEG1011E". This, as a title, doesn't mean too much to a user. Do you suggest that we just say "Error", "Warning" etc, or something more specific?
(In reply to comment #6) > ICU MessageFormat should only be used in UI plugins. > Non-UI plugins should try to save resources by using NLS.bind() instead. > To date, we aren't using ICU MessageFormat anywhere, only ICU DateFormat and > NumberFormat. An example of those is in SystemViewRemoteFileAdapter. Note that > the import of com.ibm.icu.* requires a change in the related MANIFEST.MF. So would you suggest using NLS.bind() for substitutions? How is it different from java.text.MessageFormat?
It is much simpler but faster. NLS.bind() supports a maximum of 2 arguments to substitute, and only String substitutions can be made. MessageFormat can also format numbers, dates, and reference other properties of the objects to be formatted. In general, I'd suggest using MessageFormat if you must and NLS.bind() if you can.
I don't really understand the use of systemmessages.xml at IBM, so please forgive if my comments are unqualified - still here are a few observations: * The basic idea of a unique message identifier like "RSEG1069" in the code, which is then translated into NLS Strings, is good. Some advantages: - customer support can give help to customers based on message ID's - context help based on message ID's - logfiles with message ID's but it also has some potential problems: - how to guarantee Uniqueness of a message ID in multiple products and extensions? * Assuming that IBM has somehow solved the uniqueness problem, the other issue is that all systemMessages are currently loaded from rse.ui although they should be more fine granular. Is there any reason why we cannot have a separate systemmessages.xml file in, say, files.ui / processes.ui etc ? * The other problem is that message file loading appears to be possible in UI plugins only, and always happens eagerly. Could the code be improved to also run in non-UI plugins and lazily? XML parsing itself should not be that much of a problem, since plugin.xml needs to be parsed as well anyways. * The final problem I'm seeing is that PDE tooling is not available for automatic externalization of Strings - that's one argument in favor of getting rid of systemmessages.xml in favor of NLS.bind() and friends. * Also, the template substitution code in SystemMessages (with %1 etc) seems to be old and duplicate code compared to what NLS.bind() and MessageFormat.format() already do for other parts of the Platform. So I don't think I can come up with a solution, but perhaps these observations help...
(In reply to comment #10) > I don't really understand the use of systemmessages.xml at IBM, so please > forgive if my comments are unqualified - still here are a few observations: > > * The basic idea of a unique message identifier like "RSEG1069" in the code, > which is then translated into NLS Strings, is good. Some advantages: > - customer support can give help to customers based on message ID's > - context help based on message ID's > - logfiles with message ID's > but it also has some potential problems: > - how to guarantee Uniqueness of a message ID in multiple products and > extensions? > > * Assuming that IBM has somehow solved the uniqueness problem, the other issue > is that all systemMessages are currently loaded from rse.ui although they > should be more fine granular. Is there any reason why we cannot have a > separate systemmessages.xml file in, say, files.ui / processes.ui etc ? The old RSE used by IBM made using of messages.xml in plugin extensions. Different plugins could supply their own message files reusing the existing message file infrastructure. This could be used in Open RSE, but I think it should be discouraged in favour of being consistent with the rest of eclipse. > > * The other problem is that message file loading appears to be possible in UI > plugins only, and always happens eagerly. Could the code be improved to also > run in non-UI plugins and lazily? > XML parsing itself should not be that much of a problem, since plugin.xml > needs to be parsed as well anyways. > > * The final problem I'm seeing is that PDE tooling is not available for > automatic externalization of Strings - that's one argument in favor of > getting rid of systemmessages.xml in favor of NLS.bind() and friends. > > * Also, the template substitution code in SystemMessages (with %1 etc) > seems to be old and duplicate code compared to what NLS.bind() and > MessageFormat.format() already do for other parts of the Platform. > > So I don't think I can come up with a solution, but perhaps these observations > help... > I agree with all these points which is why I think it would be good to phase this out.
The changes will include new API (for SimpleSystemMessage).
Created attachment 90102 [details] patch of mgiration of messages out from systemmessages I've gone further with this migrating so that most plugins other than org.eclipse.rse.ui have their message string independent of systemmessages.xml. I still haven't touched useractions or the tutorial. One thing that will have to be done as part of the cleanup here is getting rid of unused messages from ISystemMessages and the systemmessages.xml file. There are quite a lot of changes here so I'd like a bit of feedback before I got ahead and commit.
The changes look very good. I especially like it that in those places, where really only a simple message text is needed (no message ID, no message details), you do not create a SystemMessage object but use the NLS string directly. Whereas in other cases, where API requires a SystemMessage or details is needed, you create a SimpleSystemMessage object. Here is some things I noticed, that might need to be considered: * It looks like in many cases, the message-ID is lost with the refactoring. It looks ok in most cases, but I'm not sure if this is OK in all cases. So I'm not sure whether you'd want to embed the ID somehow. * Before the change, some "Well Known Messages" were re-used by multiple plugins - e.g. MSG_OPERATION_CANCELLED was used by Processes, Files, DStore, even RemoteCmdOperation. You might want to consider keeping such messages to be re-used in the UI plugin and exposing them through official API - similar to Eclipse Platform ISharedImages. I'm not aware, though of any shared Texts currently exposed by Eclipse Platform, it looks like they also duplicate texts like "Copy" in multiple plugins. - Some of these "Shared Messages" might be worth moving into Core or Services, since they are used by multiple ConnectorServices or Subsystems: MSG_DISCONNECT_FAILED, MSG_CONNECT_FAILED for instance * Talking about API, the whole ISystemMessages interface used to be API, so anybody was free to re-use anything. It's a breaking API change to remove some (many) of these messages - just be aware of that. * Regarding translation of the migrated texts, where the placeholders used to be %1, %2 etc. these are now {0}, {1} etc. You might want to consider some tooling with your translation teams such that they can migrate already translated messages from previous releases rather than re-translating. * It looks like some minor bugs were introduced like typos -- I'd recommend running all changes through the Compare Tool in the Synchronize View and checking the diffs before checkin. For instance: - RESID_SEARCH_STRING_LABEL_TOOLTIP=to be searched # "String" was lost - file.Pending # space char missing - cached.Use # space char missing In general, it looks like your patch has more changes than necessary, and you lost space characters in the *.properties files. Review the *.properties files and avoid any changes that are not necessary. * FileResources.properties -- it looks like in the multi-line Strings, you are missing \n\ escapes at the line endings. Translation from XML to .properties might require escaping some characters. Look at the Warnings that PDE gives you. * PLUGIN_ID = "org.eclipse.rse.subsystems.files"; I think this is not correct, it is org.eclipse.rse.subsystems.files.core In general, thanks for your excellent work on this!
(In reply to comment #14) > The changes look very good. I especially like it that in those places, where > really only a simple message text is needed (no message ID, no message > details), you do not create a SystemMessage object but use the NLS string > directly. Whereas in other cases, where API requires a SystemMessage or details > is needed, you create a SimpleSystemMessage object. > Here is some things I noticed, that might need to be considered: > * It looks like in many cases, the message-ID is lost with the refactoring. > It looks ok in most cases, but I'm not sure if this is OK in all cases. > So I'm not sure whether you'd want to embed the ID somehow. > * Before the change, some "Well Known Messages" were re-used by multiple > plugins - e.g. MSG_OPERATION_CANCELLED was used by Processes, Files, DStore, > even RemoteCmdOperation. You might want to consider keeping such messages > to be re-used in the UI plugin and exposing them through official API - > similar to Eclipse Platform ISharedImages. I'm not aware, though of any > shared Texts currently exposed by Eclipse Platform, it looks like they > also duplicate texts like "Copy" in multiple plugins. > - Some of these "Shared Messages" might be worth moving into Core or > Services, since they are used by multiple ConnectorServices or Subsystems: > MSG_DISCONNECT_FAILED, MSG_CONNECT_FAILED for instance > * Talking about API, the whole ISystemMessages interface used to be API, so > anybody was free to re-use anything. It's a breaking API change to remove > some (many) of these messages - just be aware of that. Yeah, these connect, disconnect and operation messages occur quite regularly. Do you think it makes the most sense to give public access to the Strings or should it be less direct via preconfigured system messages or something? > * Regarding translation of the migrated texts, where the placeholders used > to be %1, %2 etc. these are now {0}, {1} etc. You might want to consider > some tooling with your translation teams such that they can migrate already > translated messages from previous releases rather than re-translating. > * It looks like some minor bugs were introduced like typos -- I'd recommend > running all changes through the Compare Tool in the Synchronize View > and checking the diffs before checkin. For instance: > - RESID_SEARCH_STRING_LABEL_TOOLTIP=to be searched # "String" was lost > - file.Pending # space char missing > - cached.Use # space char missing > In general, it looks like your patch has more changes than necessary, and > you lost space characters in the *.properties files. Review the *.properties > files and avoid any changes that are not necessary. > * FileResources.properties -- it looks like in the multi-line Strings, you > are missing \n\ escapes at the line endings. Translation from XML to > .properties might require escaping some characters. Look at the Warnings > that PDE gives you. > * PLUGIN_ID = "org.eclipse.rse.subsystems.files"; > I think this is not correct, it is org.eclipse.rse.subsystems.files.core Great catches! I'll make the corresponding fixes before I commit this stuff. I've now also done the move for SubSystem. I wonder if it would make sense to port that to rse.core while I'm at it or wait til this defect is done. I'm using RSECorePlugin.PLUGIN_ID for those messages but there are still references to rse.ui classes like GenericMessages. > In general, thanks for your excellent work on this!
(In reply to comment #15) > Yeah, these connect, disconnect and operation messages occur quite regularly. > Do you think it makes the most sense to give public access to the Strings or > should it be less direct via preconfigured system messages or something? I'm not sure what's the best way of sharing these things. There are multiple possible solutions: (a) When multiple plugins want to print the same Strings, I think it makes sense to check whether they actually perform the same (or similar) operations such that we could extract those similar operations into common framework functionality. Then, only the framework would want to print the String. (b) If it's not too many messages and there is no common pattern, just leave them duplicated. (c) Expose the Strings by message ID (d) Expose the plain Strings I think I'd rather not go the message id path (c) but the others might make sense. > I've now also done the move for SubSystem. I wonder if it would make sense to > port that to rse.core while I'm at it or wait til this defect is done. Better keep the bugs / work on it separate. Porting SubSystem to core requires a lot more than just the message id's. What you can do, if you want, is move the message Strings into RSECoreMessages.java but keep accessing them from the UI plugin for now -- this will make the move easier later on.
Created attachment 90400 [details] List of API changes for this bug Attached file lists all the API changes made for this bug. The following items currently remain to be done: (1) What to do with SystemMessage("RSE","F","9999",'E',e.getMessage(), ...) (2) Duplication of messages Duplicate Messages -2a: e.g. SystemProcessesResources: MSG_OPERATION_FAILED, MSG_OPERATION_CANCELED, MSG_EXPAND_FAILED, MSG_VALIDATE_FILEFILTERSTRING_NOTUNIQU; -2b: e.g. ShellStrings: MSG_CONNECT_FAILED -2c: e.g. TelnetConnectorResources: MSG_EXCEPTION_OCCURRED, MSG_COMM_AUTH_FAILED, MSG_COMM_AUTH_FAILED_DETAILS, MSG_CONNECT_CANCELED, MSG_DISCONNECT_FAILED, MSG_DISCONNECT_CANCELED --> These are used so often, they should perhaps remain API in some way. In general, finding common API methods, or common Exception classes for common functionality should be preferred over just re-using Strings. --> SystemProcessFilterStringEditPane should perhaps use a generic UniqueFilterStringValidator rather than doing the validation itself (and copying the message) --> For MSG_OPERATION_CANCELED: Why can't the OperationCanceledException include that text such that it's used in one place only? --> For MSG_CONNECT_FAILED: Why can' implicitConnect() already throw a proper SystemMessageException rather than just returning true/false --> would bring the message to 1 place only (3) Help Contexts: The help contexts for rse.files.ui should not be defined in rse.ui, but rse.files.ui should define its own contexts instead. Then, the helpprefix also needs to be changed. See SystemNewFileWizardMainPage: SystemWidgetHelpers.setCompositeHelp(composite_prompts, RSEUIPlugin.HELPPREFIX+ISystemFileConstants.NEW_FILE_WIZARD); ---> Search for SystemWidgetHelpers.setCompositeHelp and work on those which are not calling from rse.ui Also SystemNewFolderWizardMainPage
(In reply to comment #17) > Created an attachment (id=90400) [details] > List of API changes for this bug > Attached file lists all the API changes made for this bug. > The following items currently remain to be done: > (1) What to do with SystemMessage("RSE","F","9999",'E',e.getMessage(), ...) What was the rational behind the "9999"? I don't see a corresponding message - was this just a way to fake a message? Could we instead use: new SimpleSystemMessage(Activator.PLUGIN_ID,IStatus.ERROR,e.getMessage(),e); ?
Regarding the help contexts, I think I'd prefer a separate defect for that, as this one is already pretty huge as is.
Ok, can you file the help context bug please. I fixed PII errors after the migration from SystemMessages substitution to NLS / MessageFormat Substitution: In fact the ' character typically needs to be quoted by duplicating it, or no substitutions will be made in it: [216252] Fix PII errors after moving from SystemMessages substitution to MessageFormat substitution scheme
I noticed a few Compile Errors in clients creating SystemMessages by Constructor (Like TCF). I originally wanted to get rid of these errors by converting the IndicatorException into a subclass of RuntimeException - this would have fixed the errors. But then I noticed that the errors affect source code compatibility only, but not binary compatibility - and a similar change had been documented for JDT when moving from Eclipse 3.2 to Eclipse 3.3. Here is the expected description of that API change: 1. API contract changes to SystemMessage What is affected: Clients that call a SystemMessage Constructor. Description: In RSE 2.0, the constructor SystemMessage(String, String, String, char, String, String) threw a org.eclipse.rse.services.clientserver.messages.IndicatorException. This is no longer the case in RSE 3.0! Note that this change has no impact on binary compatibility. Action required: Clients calling a SystemMessage constructor will have to fix possible 'Unreachable catch block for IndicatorException' compiler errors.
I put a few more changes in so that the dstore and local services don't rely on a message provider anymore. Instead, they construct their own SimpleSystemMessages. I'd like to close this defect, and use separate defects to track any outstanding issues. Any problem with that?
These changes are some more breaking API changes, which particularly require reving up the org.eclipse.rse.services bundle version to 3.0: Removing public SystemMessage IService#getMessage(String messageID); public SystemMessage AbstractFileService#getMessage(String messageID); public SystemMessage LocalFileService#getMessage(String messageID); DStoreFileService(IDataStoreProvider, ISystemFileTypes, ISystemMessageProvider) public AbstractDStoreService#_msgProvider; AbstractDStoreService(IDataStoreProvider, ISystemMessageProvider); Are all breaking API changes for clients which either construct a file service, or extend a file service in a way needing messages. I'm OK with these changes, I'm also OK with marking this bug closed and dealing with other issues in separate bugs. For this specific bug, the change of removing ISystemMessageProvider is not yet complete, following should also be removed: Field LocalFileService#_msgProvider Constructor LocalFileService (ISystemFileTypes, ISystemMessageProvider); Field LocalProcessService#_msgProvider Constructor LocalProcessService (ISystemMessageProvider); I'm not sure whether interface ISystemMessageProvider should also be removed.
Created attachment 90708 [details] Updated list of API Changes
Filed bug 220309 for moving some GenericMessages from UI to Core
I've removed ISystemMessageProvider since it's not neeeded anymore. I'll close this defect and followup with the new related defects.
Following up with bug 220547 for adding an ID to SimpleSystemMessage
Just found ACTION_CONTENT_ASSIST declared in rse.ui but used in rse.shells.ui --> please move.
(In reply to comment #28) > Just found ACTION_CONTENT_ASSIST declared in rse.ui but used in rse.shells.ui > --> please move. Okay, I've moved that over.
I decided to change IndicatorException into a RuntimeException in order to avoid error messages in builds against the earlier API; but at the same time also to mark it deprecated, in order to give people still catching it a warning that they shouldn't have to catch it.
I learned today that "cancelled" is also correct (british) English, whereas "canceled" is correct (american) English. See http://dict.leo.org for instance. Since both variants are correct, I see no use in breaking any existing clients just for such a simple rename refactoring. CDT also seems to be using both variants, and in Eclipse Platform Javadocs (e.g. IRunnableContext) I'm also seeing both variants. I'd thus like to back out the following changes again. Note that changes marked with an ! are particularly likely to break existing clients since these are used a lot; backing out this change therefore makes sense. ! Rename RemoteFileCanceledException -> RemoteFileCancelledException ! Rename IHostSearchConstants.CANCELED -> CANCELLED Rename IRemoteSearchConstants.CANCELED -> CANCELLED Rename ISystemProcessRemoteConstants.STATE_ZOS_CANCELED -> STATE_ZOS_CANCELLED Rename ISystemProcessRemoteConstants.STATE_ZOS_CANCELED_INDEX -> STATE_ZOS_CANCELLED_INDEX Rename ISystemMessages.MSG_EXPAND_CANCELED -> CANCELLED Rename ISystemMessages.MSG_OPERATION_CANCELED -> CANCELLED Rename ISystemMessages.MSG_LIST_CANCELED -> CANCELLED Note that corresponding @since tags need to be removed again in some places, since we're reverting an API change that thus no longer is an API change. Any objections?
I guess one potential objection is that "OperationCanceledException" is a very popular class so we might want to be consistent with org.eclipse.core.runtime. On the other hand, the JDK defines java.util.concurrent.CancellationException java.nio.channels.CancelledKeyException and Eclipse/RSE defines org.eclipse.rse.dstore.universal.miners.ICancellableHandler org.eclipse.ecf.filetransfer.UserCancelledException Then, again, there is com.sun.corba.si.impl.protocol.RequestCanceledException org.eclipse.swt.internal.mozilla.nsICancelable javax.print.CancelablePrintJob I'd say it looks like a draw -- and given that it's a draw I'm in favor of keeping APIs so as not to break any clients. Also, the double-l-variants appear in more API packages whereas there are only 2 single-l-variants in API packages: CancelablePrintJob and OperationCanceledException. Thoughts?
I prefer canceled to cancelled just because most tools think canceled is the correct spelling even though cancelled is an acceptable spelling. With M6 coming to a close, I'd rather not mess with this stuff on such short notice.
Well it's not that much of a mess since it's a simple rename refactoring. I can do it easily, so no worries about that - I expect about 15 minutes work for the 8 refactorings, plus removing @since tags, plus committing everything. These are no constants which are anywhere in text files so the automated refactoring should run smoothly. Also, the plain fact that more tools YOU are using happen to use an American English Dictionary is not enough justification for me. M6 timeframe is an issue, of course, but IMHO outweighed by the benefit of achieving better 2.x backward compatibility and thus less client hassle when adopting 3.0.
If we really want to standardize on "Canceled" with single l, there's few more places to fix: AbstractSystemViewAdapter#getCancelledMessageObject() AbstractSystemWizard#wasCancelled() AbstractSystemWizard#setWasCancelled() SystemPromptDialog#wasCancelled() SystemPromptDialog#wasCancelledAll() DataStoreResources#model_Cancellable ICancellableHandler ICancellableHandler#isCancelled() DownloadListener#isCancelled() DownloadListener#wasCancelled() DStore Protocol: _status.getValue().equals("cancelled")) //$NON-NLS-1$ DStoreStatusMonitor#setCancelled() DStoreStatusMonitor#wasCancelled() HostSearchResultSet#isCancelled() IHostSearchResultSet#isCancelled() ISystemDialogAction#wasCancelled() ISystemPasswordPromptDialog#wasCancelled() ISystemPromptDialog#wasCancelled() StandardCredentialsProvider#isCancelled() StatusMonitor#setCancelled() StatusMonitor#wasCancelled() SubSystem#showConnectCancelledMessage() SubSystem#showDisconnectCancelledMessage() SubSystem#showOperationCancelledMessage() SystemAbstractAPIProvider#getCancelledMessageObject() SystemBaseCopyAction#showOperationCancelledMessage() SystemBaseDialogAction#wasCancelled() ... I stop counting here, and these are just the API methods, not talking about "internal" classes. Even if I'd like standardizing on Canceled to make the spellcheckers happy, I think we're more consistent standardizing on "Cancelled" with double l... Sure, many of the instances listed above come from the same class hierarchy rooted at some object so they could be refactored in one step, but still it looks like (much) more work than reverting the changes already made. Comments?
In current RSE codebase, I get 436 non-comment-matches of "canceled" and 397 non-comment-matches of "cancell" --> looks like a draw again. --> Note that from the 436 "canceled", most of them come from calling IProgressMonitor#isCanceled()
Google says: cancelled: 40,500,000 hits canceled: 17,000,000 hits
Good point! JDT Search gives more exact numbers, I checked our full RSE codebase: for "*cancell*": - in Method Declarations: 44, some of them internal - in Field Declarations: 23, half of them internal - in Type Declarations: 1 (ICancellableHandler) for "*cancele*": - in Method Declarations: 11, mostly API -plus "*cancela*": 9, mostly API - in Field Declarations: 28, half of them internal, - these are mostly due to recent refactoring for this bug - in Type Declarations: 1 (RemoteFileCanceledException) I'm tending to restore "cancelled" everywhere, even though it hurts due to the Spellchecker and due to Eclipse OperationCanceledException
What I'm planning to do is revert the changes we made (some cancelled became canceled), such that we're not unnecessarily breaking any 2.0 APIs. I expect that with this change RSE will pretty much standardize on cancelled with double l, so I'll then inspect the remaining instances and probably standardize some more on double l though this is not very likely. We can all feed our spell checkers with a user dictionary that allows the double-l-version easily. If anybody is not happy with this approach, speak up now or be silent forever :-)
PS there's (at least) 3 instances which affect NLS Keys: Rename ISystemMessages.MSG_EXPAND_CANCELED -> CANCELLED Rename ISystemMessages.MSG_OPERATION_CANCELED -> CANCELLED Rename ISystemMessages.MSG_LIST_CANCELED -> CANCELLED Xuan, DaveD: Are these NLS keys frozen due to the PII drop as well? Or are just the Strings that they map to frozen? - I'll refactor these as well unless you tell me otherwise by 10am tomorrow Toronto time.
I just checked the NL schedule, and we still have a delta drop on on 4/21 (PII 4). They only expect very small volumn for that drop.
I reverted the following changes back to what it was like in 2.0.x: ------------------------------------------------------------------- ! Rename IHostSearchConstants.CANCELED -> CANCELLED Rename IRemoteSearchConstants.CANCELED -> CANCELLED Rename ISystemProcessRemoteConstants.STATE_ZOS_CANCELED -> STATE_ZOS_CANCELLED Rename ISystemProcessRemoteConstants.STATE_ZOS_CANCELED_INDEX -> STATE_ZOS_CANCELLED_INDEX Rename ISystemMessages.MSG_EXPAND_CANCELED -> CANCELLED Rename ISystemMessages.MSG_OPERATION_CANCELED -> CANCELLED Rename ISystemMessages.MSG_LIST_CANCELED -> CANCELLED Rename ICommonMessageIds.MSG_DISCONNECT_CANCELED -> CANCELLED Rename ICommonMessageIds.MSG_CONNECT_CANCELED -> CANCELLED Rename RESID_PROPERTY_PROCESS_TYPE_ZOS_CANCELED_VALUE Rename UniversalFileTransferUtility.RENAME_DIALOG_CANCELED Rename UniversalFileTransferUtility.RENAME_DIALOG_CANCELED_ALL Rename UniversalFileTransferUtility.RENAME_DIALOG_NOT_CANCELED I dit not revert the following, because it is too similar to OperationCanceledException so I find it much more usable like this: ---------------------------------------------------------------------- ! Rename RemoteFileCanceledException -> RemoteFileCancelledException The result of this reverting is pretty good: From 471 matches of "canceled" in the workspace (case-insensitive!) there are 257 alone due to "IProgressMonitor.isCanceled" and "OperationCanceledException" which we cannot change. Only 74 occurrences of "canceled" remain that we could theoretically change; but these are mostly local variables so I think that pretty much all our API is consistent (again). We do have some inconsistencies in the displayed messages, though (the NLS Strings) -- I didn't want to touch those; Xuan/DaveM what do you want to do with them? e.g. in systemmessages.xml: <Message ID="1099" Indicator="I"> <LevelOne>Expand cancelled. Try again</LevelOne> <LevelTwo></LevelTwo> </Message> <Message ID="1101" Indicator="I"> <LevelOne>List cancelled</LevelOne> <LevelTwo></LevelTwo> </Message> but <Message ID="1013" Indicator="E"> <LevelOne>Expand failed or canceled. Try again.</LevelOne> <LevelTwo>When expanding a node in the tree, and retrieving the resources, either an error occurred or you cancelled the operation. The expand is cancelled. </LevelTwo> </Message> <Message ID="1067" Indicator="E"> <LevelOne>Operation canceled.</LevelOne> <LevelTwo></LevelTwo> </Message> In CommonMessages.properties: - Several NLS Strings "Canceled"
Reverting Change committed: [216252] Revert CANCELED -> CANCELLED to restore 2.0.x compatibility
Some currently inconsistent API: -------------------------------- public StandardCredentialsProvider.isCanceled() -- was canceled in 2.0.x already. Related to Progress Monitor. protected AbstractSystemViewAdapter.canceledObject protected SystemAbstractAPIProvider.canceledObject -- was canceled in 2.0.x already. Eventually I'd like to get rid of this API RemoteFileCanceledException Some API that we cannot or should not change: --------------------------------------------- OperationCanceledException IProgressMonitor#isCanceled() / setCanceled() / checkCanceled() Several local variables related to progress monitor OperationCanceledException|monitor.setCancel|monitor.isCancel|RemoteFileCanceledException| canceled\S|checkCanceled Some more API or non-API that I did refactor: --------------------------------------------- [216252] Refactor some simple Canceled -> Cancelled -- LocalFileService#getCanceledException() ... and that's pretty much it! Pending the NLS fixes mentioned in the previous comment, we're all standardized on Cancelled except for really few places (definitely less than 60).
On 2nd thought, I did revert the RemoteFileCanceledException back to RemoteFileCancelledException like it was in 2.x -- no use breaking API unless necessary. I also adjusted ISystemOperationMonitor#isCancelled since this is a new API interface that was only added in 3.0 so we have the chance making it consistent.
After little more work, here are the Final Numbers: Some currently inconsistent API: -------------------------------- protected AbstractSystemViewAdapter.canceledObject protected SystemAbstractAPIProvider.canceledObject -- was canceled in 2.0.x already. Eventually I'd like to get rid of this API public RemoteUtil.checkCanceled() -- was canceled in 2.0.x already public Policy.checkCanceled() -- was canceled in 2.0.x already Some API that we cannot or should not change: --------------------------------------------- OperationCanceledException IProgressMonitor#isCanceled() / setCanceled() / checkCanceled() Using the following filter in text search for "canceled": OperationCanceledException|monitor.setCancel|monitor.isCancel I now get only 27 unfiltered Matches! - At least some of which are NLS. This is now tracked in bug 226784. Marking this bug fixed for 3.0M6.
Created attachment 95780 [details] Final List of API Changes for this bug.
Created attachment 96415 [details] Additional patch for 3.0M7 Attached additional API patch fixes the following: * AbstractSystemViewAdapter: - Renamed protected field canceledObject --> cancelledObject - Made private field filterString - Deprecated protected fields cancelledObject, errorObject - Added @noextend where already indicated by textual Javadoc - Fixed some Javadoc Typos * SystemAbstractAPIProvider: - Renamed protected field canceledObject --> cancelledObject - Deprecated protected fields cancelledObject, errorObject - Deprecated protected field sr - Added @noextend where already indicated by textual Javadoc
