Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #185348 +++ TerminalConnectorInfo already takes care of those methods that make sense on a TerminalConnector before it is initialized. The extra indirection through TerminalConnectorProxy is unnecessary and just confusing. The ITerminalConnectorInfo.getConnector() call should already return the real connector. Only like this, clients which know the actual connector implementation can perform methods on it which would otherwise be hidden due to the proxy - see bug #185348 comment 6.
Created attachment 94252 [details] Patch returning the real connector instead of Proxy when possible I'm wondering whether it might be sufficient to just return the "real" connector once it is initialized, or return the proxy otherwise? See attached patch.
In fact the TerminalConnectorInfo could also return "null" for getConnector() as long as it's not yet initialized. That would ensure that no TerminalConnectorProxy instances ever leak to the outside, making life simper - and essentially making the TerminalConnectorProxy class unnecessary.
Created attachment 94317 [details] A patch to make the Proxy adaptable Another solution is to make ITerminalConnectorInfo IAdaptable. The proxy does some clever things to delay the creation of the connector. Removing the proxy would mean that the delay logic would be pushed to the client code. For example, ITerminalConnector.load is called before the connector is created. This is cached inside the proxy. ITerminalConnector.load is called when the view gets restored in TerminalView.setupControl (the information comes from the view state). For the terminal view to delay the load of ITerminalConnector.load, it would have to know who and when the terminal connector is created. One way to know this would be to add creation listener to the terminal connector. But the terminal connector would then have to support such a listener. And I am sure other users of the Terminal would have difficulties implementing this logic. I think Using the adapter machanism would solve the ptoblem....
(In reply to comment #3) > Another solution is to make ITerminalConnectorInfo IAdaptable.... Another solution is to make TerminalConnectorProxy IAdaptable.... > I think Using the adapter machanism would solve the ptoblem.... I think Using the adapter mechanism would solve the problem....
Hmm, the load could be moved to ITerminalConnectorInfo..... The ITerminalConnectorInfo would then have the same role as the proxy.... But I think it is good practice to "firewall" classes that come from extensions and not hand them directly to clients. The "firewall" (in our case the TerminalConnectorProxy) can hide implementation problems of the ITerminalConnector from users. The adaptable mechanism provides a way to get to the real thing. But in all other cases it is much better to deal with the proxy....
Created attachment 94807 [details] Some refactoring
The attached patch contains the following changes: - ITerminalConnectorInfo is gone - ITerminalConnector is now the client interface - the implmentation is in org.eclipse.tm.internal.terminal.connector.TerminalConnector (was TerminalConnectorExtension.TerminalConnectorProxy) - TerminalConnector is adaptable and can be adapted to the TerminalConnectorImpl - the terminalConnector extension now requires extensions to implement TerminalConnectorImpl (was ITerminalConnector before) - a test added for TerminalConnector
Created attachment 94858 [details] Renaming ext.point to terminalConnectors + Adding some Javadocs Thanks for the patch, I truly think that this makes the API more clean, usable and understandable. I still have a couple of questions: 1. I think we should rename the extension point to "terminalConnectors" since multiple <connector> markups are possible per extension. Also, this makes the extension more in line with the rest of Eclipse. See attached patch, which also adds some Javadocs. Looks good? 2. What about moving TerminalConnectorImpl into org.eclipse.tm.internal.terminal.provisional.api.provider/ package to separate Provider API from user API? I'm also not yet 100% happy with the name ("TerminalConnectorImpl") but can't think of anything better right now. 3. It looks like the TerminalConnector.Factory should be (provisional) provider API, in order to support different connector instantiation mechanisms? 4. Javadoc @since tags are missing for new API, API Tooling should help creating these (somehow since we're all having "internal" packages) Migration Docs: --------------- * For Users: - Use ITerminalConnector wherever you used to use ITerminalConnectorInfo - If you have an ITerminalConnector object "connector", you can now do connector.getAdapter(SshConnector.class); in order to get a concrete instance of connector type, e.g. for modifying its settings. Note that this imposes a bit of a security hole for stored passwords since anybody can read them now! We might consider migrating to Equinox Secure Storage in the future (see bug 225320) - Just by replacing TerminalConnectorExtension with a different mechanism for enumerating and instantiating connectors, the Terminal Framework can now also be used in an environment where the Eclipse Extension Registry is not available e.g. very resource constrained Equinox/OSGi environments. * For Implementers of custom Terminal Connectors (Extenders): - extend TerminalConnectorImpl rather than implementing ITerminalConnector - rename your extension to "terminalConnectors"
(In reply to comment #8) Patch committed, Javadocs added with all tags, TerminalConnectorImpl moved to new tm.internal.terminal.provisional.api.provider package, Copyright dates updated. I'm still not sure if TerminalConnectorImpl is a good name, but can't think of anything better. Perhaps we should go back to TerminalConnector --> TerminalConnectorProxy TerminalConnectorImpl --> TerminalConnector but it's also not quite correct so I guess we'd better stick with what we have now. Migration Docs still apply as per comment #8.
