Community
Participate
Working Groups
Right now RemoteFileSubSystem provides the following API: /** * Returns <code>true</code> by default. Subclasses should override if they do not support encodings. * @see org.eclipse.rse.subsystems.files.core.subsystems.IRemoteFileSubSystem#supportsEncoding() * @since 2.0 */ public boolean supportsEncoding() { return true; } The problem is that we don't encourage extending the subsystem and prefer that such specific things are handled in the service layer. Right now, without extending the subsystem, this can't be changed. I think we need to have a corresponding IFileService.supportsEncoding() that will be called by the subsystem.
Created attachment 95564 [details] patch adding supportsEncoding() file service method This patch adds a supportsEncoding() method to IFileService. The abstractFileService implements it to return true by default but it can be extended to return false (if you don't want to display encodings in the file Info property page).
The remote file subsystem api is currently only used for the SystemFilePropertyPage to decide whether or not to display the encoding and hence provide the ability to change the encoding. supportsEncoding() isn't all that intuitive of a name for that though since, in this case, we're just using this to decide whether or not users should be able to see and change the encoding. Perhaps a better name for this would be supportsUserEncodingModification() or something like that. Any thoughts?
This is api so I'd like to get feeback before tomorrow.
Before I decide this, I'd like to understand why this method is there in the first place. Can you find out why we added it in 2.0? I agree that the Javadoc is very bad because we don't really get an idea what this is good for. At least there should be a bug number referenced such that we can look it up. Note that we also have supportsEncodingConversion() which apparently does something different. I'd love to see the Javadoc of supportsEncoding() updated regardless of whether we add the service equivalent or not. Can you do that right away? Regarding your question for review, my naive answer would be that the _file service_ always deals with binary data and it's the _subsystem_ which presents that binary data in a particular encoding, or supports encoding conversion. Therefore it's not dependent on the Service. At least that's how I would implement it. The FileServiceSubsystem that we have does support this, so it always returns true. It doesn't need support from the service to do it's job, so it's fine to not ask the service.
PS the Javadoc in IRemoteFileSubsystem is a little more verbose, but still quite confusing, at least for me.
(In reply to comment #4) > Before I decide this, I'd like to understand why this method is there in the > first place. Can you find out why we added it in 2.0? > I agree that the Javadoc is very bad because we don't really get an idea what > this is good for. At least there should be a bug number referenced such that we > can look it up. Note that we also have supportsEncodingConversion() which > apparently does something different. I'd love to see the Javadoc of > supportsEncoding() updated regardless of whether we add the service equivalent > or not. Can you do that right away? > Regarding your question for review, my naive answer would be that the _file > service_ always deals with binary data and it's the _subsystem_ which presents > that binary data in a particular encoding, or supports encoding conversion. > Therefore it's not dependent on the Service. At least that's how I would > implement it. > The FileServiceSubsystem that we have does support this, so it always returns > true. It doesn't need support from the service to do it's job, so it's fine to > not ask the service. The method was added by Kushal via this defect: bug 163820 [api] [163820] Allow encodings to be queried for IRemoteFile and implementations for encodings through a remote file encoding manager. The method was added so that the Info property page could decide whether or not to show the encoding field which allows a user to change the encoding. For the dstore file service, encodings are important when doing a text transfer (where the remote native encoding gets converted to the local encoding). This is particularly important for IBM's bidi support.
Doing an encoding conversion is really simple these days, so I really cannot see how a Service would ever not support encodings. What I'm not sure is if there could be any interesting variant of a File Subsystem that wouldn't want to show the encoding property for any reason. I faintly remember some user saying that he wanted to hide that field from RSE in his product, because it's meaningless to the kind of remotes that he's working with (some devices) and he just didn't want to confuse users. I think I'd be fine with any of the following: (a) remove the supportsEncoding() method (b) rename it to getShowEncodingControl(), and potentially add a setShowEncodingControl(boolean) method such that the subsystem can be configured from the outside (c) rename to isEncodingControlEnabled() along with setEncodingControlEnabled(boolean); I really don't see this as a property of the Service, but rather as a User Preference or Product Preference of a particular product that configures RSE for its needs.
(In reply to comment #7) > Doing an encoding conversion is really simple these days, so I really cannot > see how a Service would ever not support encodings. > What I'm not sure is if there could be any interesting variant of a File > Subsystem that wouldn't want to show the encoding property for any reason. I > faintly remember some user saying that he wanted to hide that field from RSE in > his product, because it's meaningless to the kind of remotes that he's working > with (some devices) and he just didn't want to confuse users. For Violaine's defect about the BIDI property pages, this may help provide a solution because her page provides it's own encoding widget that does the same thing. For IBM's I5/OS work (Xuan could confirm this) it also makes sense since it's preferrable to show CCSIDs in an I5/OS-specific property page rather than encodings on the Info property page (which could confuse such users). > I think I'd be fine with any of the following: > (a) remove the supportsEncoding() method > (b) rename it to getShowEncodingControl(), and potentially add a > setShowEncodingControl(boolean) method such that the subsystem can be > configured from the outside Only thing is a subsystem shouldn't be coupled to the ui so I'd prefer this wasn't a method about "showing controls". > (c) rename to isEncodingControlEnabled() along with > setEncodingControlEnabled(boolean); > I really don't see this as a property of the Service, but rather as a User > Preference or Product Preference of a particular product that configures RSE > for its needs. In the examples, I don't see them as user preferences or product preferences. These cases are specific to the system type (and service) that are being used. For non i5/os or non z/os cases, the regular settings still make sense.
(In reply to comment #8) > For Violaine's defect about the BIDI property pages, this may help provide a Good, that makes sense, now I understand! > Only thing is a subsystem shouldn't be coupled to the ui so I'd prefer this > wasn't a method about "showing controls". But your examples are about UI, aren't they? You're essentially configuring the subsystem such that it doesn't expose its own notion of what it think wants to do with encodings, but lets the extension (from Violaine or whomever) do its thing. Anywyas, what about these: supportsEncodingProperties() / setSupportsEncodingProperties() hasEncodingProperties() / setHasEncodingProperties() > In the examples, I don't see them as user preferences or product preferences. > These cases are specific to the system type (and service) that are being used. > For non i5/os or non z/os cases, the regular settings still make sense. Well if it's specific to the system type, that's fine isn't it? They will have their own SubSystemConfiguration for their kind of service, and in the factory method they do this: public ISubSystem createSubSystemInternal(IHost host) { MyConnectorService connectorService = (MyConnectorService)getConnectorService(host); ISubSystem subsys = new FileServiceSubSystem(host, connectorService, getFileService(host), getHostFileAdapter(), getSearchService(host)); subsys.setSupportsEncodingProperties(false); return subsys; } I still can't see how that would relate to the service. It's a UI / Configuration thing IMHO. But perhaps you can explain me why IFileService#supportsEncodings() would make sense in a concrete case?
(In reply to comment #9) > (In reply to comment #8) > > For Violaine's defect about the BIDI property pages, this may help provide a > Good, that makes sense, now I understand! > > Only thing is a subsystem shouldn't be coupled to the ui so I'd prefer this > > wasn't a method about "showing controls". > But your examples are about UI, aren't they? You're essentially configuring the > subsystem such that it doesn't expose its own notion of what it think wants to > do with encodings, but lets the extension (from Violaine or whomever) do its > thing. Anywyas, what about these: > supportsEncodingProperties() / setSupportsEncodingProperties() > hasEncodingProperties() / setHasEncodingProperties() > > In the examples, I don't see them as user preferences or product preferences. > > These cases are specific to the system type (and service) that are being used. > > For non i5/os or non z/os cases, the regular settings still make sense. > Well if it's specific to the system type, that's fine isn't it? They will have > their own SubSystemConfiguration for their kind of service, and in the factory > method they do this: > public ISubSystem createSubSystemInternal(IHost host) { > MyConnectorService connectorService = > (MyConnectorService)getConnectorService(host); > ISubSystem subsys = new FileServiceSubSystem(host, connectorService, > getFileService(host), getHostFileAdapter(), getSearchService(host)); > subsys.setSupportsEncodingProperties(false); > return subsys; > } Hmm, maybe it would be better for this to be done in the configuration: IRemoteFileSubSystemConfiguration.supportsEncodingProperties() Since a system specific service would have it's own configuration anyway, it probably makes sense to do that..and then we don't have to worry about calling setSupportsEncodingProperties(). What do you think?
This is regarding to comment 8: > For IBM's I5/OS work (Xuan could confirm this) it also makes sense > since it's preferrable to show CCSIDs in an I5/OS-specific property page rather > than encodings on the Info property page (which could confuse such users). Yes. For i5/OS IFS file system, we don't have encoding information. We only have CCSID info available. So we want to hide the encoding widget, and show CCSID in a i5/OS specific page.
(In reply to comment #10) > Since a system specific service would have it's own configuration anyway, it > probably makes sense to do that..and then we don't have to worry about calling > setSupportsEncodingProperties(). Well, yes, that seems to be an option. We'd probably want to have ISubSystemConfiguration#supportsEncodingProperties(IHost h) in order to be able and decide differently based on the host being passed in, or the SystemType that the config is registered against. Advantage of the config is that (malicious) users cannot change it at runtime. Disadvantage of the config is that it's probably less dynamic... if a Subsystem (or dynamically contributed Host) finds out at runtime that it cannot support encidngs for some reason it cannot change it in the config, but it can change it with SubSystem#setSupportsEncodingProperties(). I'm fine with both approaches. One other question is if we'll want to totally get rid of teh encoding control, or just disable it to still show some encoding info as an unmodifiable text.
Created attachment 95614 [details] patch adding IRemoteFileSubSystemConfiguration.supportsEncoding() I've changed this to use the configuration rather than the subsystem.
I think you can get rid of the method in ISubSystem rather than just deprecating it. Also, please try and improve the Javaoc. When I'm not mistaken, we have learned that the meaning of this setting is whether users can specify or change the encoding with which remote resources are to be presented. In case it returns false, the encoding either cannot be changed, or it is specified with different alternative methods. Right?
Created attachment 95617 [details] updated patch How's this?
Actually, I think that it's not a file service specific question whether encoding properties are supported or not, it's something applicable to any kind of subsystem. The basic thing here is, that every subsystem deals with resources on some kind, which are encoded in binary on the remote side. At some point, users get text Strings describing these resources in some way or the other. Now there are three possibilities: 1.) the encoding of these text Strings can be fixed and known to the subsystem (so users cannot change it -- they get correct Unicode Java Strings automatically and never care about any encoding); 2.) encoding of text Strings is customizable in standard manner, using Java Streams / decoding of byte[] on the client; RSE properties etc. 3.) encoding is customizable, but in a way that goes beyond RSE's capabilities. Case (2) is what we do now, and cases (1) and (3) require us to switch off RSE's encoding support for a particular subsystem. The point in (2) is, that RSE allows the user to specify encodings for resources; and services or subsystems are capable of encoding/transcoding/recoding byte[] streams to what the user specified. These byte streams can be file names, process names, a Shell stream etc... but in any case that en/de/re/transcoding happens at the client side, inside RSE. So, I propose adding ISubSystemConfiguration#supportsEncoding(IHost host) meaning that the subsystem allows the user changing the way how bytes are encoded to Strings for that particular subsystem - using the RSE standard Controls. I tried coming up with better names, e.g. supportsClientEncoding() supportsEncodingChanges() supportsCustomEncoding() supportsRSEEncoding() supportsEncodingOnClient() but I'm not really happy with any of these. One big advantage of having it in ISubSystemConfiguration is that in case NO subsystem under an IHost supportsEncoding(), the property page on the IHost level can also be removed! Consequently, I propose also adding an IHost / IRSESystemType Property, by which users can control switching RSE Encoding support off: IRSESystemType#PROPERTY_SUPPORTS_ENCODING = "supportsEncoding"; Now, we can do this in the default impl of SubSystem, to support switching off the property page on the IHost if either no subsystem supports encodings, or all subsystems take the default fallback from the system type and it's switched off on the system type: class SubSystem { public boolean supportsEncoding(IHost host) { boolean rv = true; //support encodings by default IRSESystemType sysType = host.getSystemType(); if (sysType.testProperty("supportsEncoding", false)) { //switched off on system type! return false; } } }; How cool is that?
Updated summary, previous value was: [api] IFileService should have a supportsEncoding() method
Created attachment 95653 [details] Proposed patch Patch with my suggested implementation + Javadoc
I'm so sure that this is the right way to go that I'm committing this straight away -- we can revert anything later today if you're unsure: [226574][api] Add ISubSystemConfiguration#supportsEncoding()
Created attachment 95655 [details] Additional patch getting rid of IRemoteFileSubSystem#supportsEncoding() I forgot to also attach the patch which gets rid of the old way of doing things. I think we should get rid of this method since, as Dave pointed out, in a world where everything is handled by services and their configurations, overriding SubSystem is really deprecated and we thus shouldn't have such config methods in there.
Additional patch committed, which makes this change [breaking] because IRemoteFileSubSystem#supportsEncoding() is removed. Migration Docs: --------------- Clients who used to call IRemoteFileSubSystem#supportsEncoding() now do this instead: subSys.getSubSystemConfiguration().supportsEncoding(subSys.getHost())
This looks good. Can we also change the host property page to not include the encoding fields when all subsystem configurations return false for supportsEncoding(IHost)?
Created attachment 95672 [details] patch to not show encoding controls in SystemConnectionPropertyPage when no subsystem config supports encodings Here's how we could take out encodings for hosts/subsystems that don't need them.
Yup, that's exactly what I had in mind. Go for it! Please check bugzilla whether there is actually a bug requesting this functionality for the host property page -- I faintly remember so. If you find it, you can either mark it as dup of this one, or you mark it as blocked by this one and commit your patch for the other one.
*** Bug 207180 has been marked as a duplicate of this bug. ***
