Bug 186640 - [api] Add IRSESystemType.testProperty() method for checking contributed properties
Summary: [api] Add IRSESystemType.testProperty() method for checking contributed prope...
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 1.0.1   Edit
Hardware: All All
: P1 enhancement (vote)
Target Milestone: 2.0   Edit
Assignee: Martin Oberhuber CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 150498 170918
  Show dependency tree
 
Reported: 2007-05-11 15:21 EDT by Martin Oberhuber CLA
Modified: 2008-08-13 13:18 EDT (History)
0 users

See Also:
mober.at+eclipse: pmc_approved+
uwe.st: review+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2007-05-11 15:21:42 EDT
Very much RSE code still uses the deprecated IRSESystemType.getName() method just in order to check whether it is running locally, or running on Windows.

This should be replaced by convenience methods:
  IRSESystemType.isLocal()
  IRSESystemType.isWindows()

These are, basically, just an example for how Properties of system types should be used in the future in order to perform certain actions. For instance:
  boolean isCaseSensitive=IRSESystemType.getProperty("isCaseSensitive", false);

Testing for Properties like this allows getting rid of static checks for the system type name, and will make RSE much more dynamic in the future -- especially when Property checks as mentioned above are performed against an ISubSystem with fallbacks:

ISubSystem -> ISubSystemConfiguration -> IConnectorService -> IRSESystemType

Thanks to these fallbacks, providers can override the return value of such properties at any level, and actual service implementation code can act correctly according to the detected property.
Comment 1 Martin Oberhuber CLA 2007-05-11 15:22:55 EDT
At the same time, RSESystemType should be promoted from "internal" to an API type in order to allow extenders provide dynamic system types by deriving from RSESystemType, even if we add more such property checks to the IRSESystemType interface in the future.
Comment 2 Martin Oberhuber CLA 2007-05-11 16:28:05 EDT
Change completed. Code got a lot easier thanks to this change.
Checkin comment was:
[186640] Add IRSESystemTyep.isLocal(), isWindows(), getProperty(String, boolean)
Comment 3 Uwe Stieber CLA 2007-05-12 05:49:42 EDT
Martin, can you please clarify what IRSESystemType.isLocal() and IRSESystemType.isWindows() are testing for?

If IRSESystemType.isLocal() returns true if the system if the local system type and false in all other cases, I cannot vote for this kind of API. The correct replacement for the IRSESystemType.getName().equals(...) way of testing would be to use the IRSESystemType.getId().equals(...). The necessary id's to know are defined as public constants.

Instead of adding more convenience methods to central interfaces, we should keep them as small as possible. Methods, which can return true only for a very specific instance, are not API. 
Comment 4 Martin Oberhuber CLA 2007-05-14 05:11:06 EDT
The idea of this change is to avoid having to check for a particular id of a system type, but rather check for properties of system types as described in the plan item on bug #170918. It's not about the id of "the local type" but the property of "a local type".

isLocal() is not the very best example, but imagine the case where an extender wants to register his own system type for e.g. "Windows CE" kinds of systems. He wants the existing FileServiceSubsystem to work, and knows that this new system types acts just as other "Windows" systems. Therefore, he defines a new system type with a new ID and sets the "isWindows" Property.

Conceptionally, the methods mentioned are just shortcuts as follows:
  isLocal() --> testProperty(IRSESystemType.PROPERTY_IS_LOCAL, true)
  isWindows() --> testProperty(IRSESystemType.PROPERTY_IS_WINDOWS, true)
Because in existing code, these two property tests are by far the ones used most often, I wanted to have special methods for them. Also, in order to maintain backward compatibility, these methods do not just check the property but also check against the formerly well-known ID's; plus, isWindows checks the os.name so it is smarter than a plain property check only.

By testing for Properties, we can 
 * write subsystem code that checks for properties of a system type, or
   active system connection (the latter being more dynamical)
 * thus make the subsystem code better re-usable for newly defined system types
   that are "similar" (i.e. have a particular property).

I've updated Javadoc in IRSESystemType accordingly and hope you can vote + for the change now. Summing up, the changes are:

1. Added IRSESystemType#testProperty(String key, boolean expectedValue)
         IRSESystemType#isLocal()
         IRSESystemType#isWindows()
         IRSESystemType#PROPERTY_IS_CASE_SENSITIVE
         IRSESystemType#PROPERTY_IS_LOCAL, PROPERTY_IS_WINDOWS

2. Created 
         AbstractRSESystemType
   In order to be able and add similar property checks in the future without
   breaking API. Clients defining their own SystemType for the dynamic
   systemTypeProviders extension poitn are expected to extend 
   AbstractRSESystemType instead of just implementing IRSESystemType.
   breaking API, created

Migration Docs:
---------------
If IRSESystemType was implemented before, extend AbstractRSESystemType instead.
Comment 5 Martin Oberhuber CLA 2008-08-13 13:18:23 EDT
[target cleanup] 2.0 M7 was the original target milestone for this bug