[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
[dsdp-tm-dev] RE: FTP Listing Parser Extension Point
|
Hello Javier,
this is fabulous!
I like the code, it makes FTPService simpler by factoring
out the
decision what parser to use, and makes it more configurable
at
the same time.
When reviewing the code, I came across few questions
though:
-
The .exsd doesn't document whether lower priority values are preferred,
or higher ones are preferred. The code seems to indicate that lower ones have
precedence. Documentation should be similar to subsystemConfiguration.exsd:
"
This optional attribute
determines the order in which subsystems appear in the New Connection wizard
and RSE views. Integer values are allowed. Subsystems with lower number will
appear first in the RSE tree. Subsystems that do not define the priority
attribute will be appended last to the list of subsystems."
-
In FTPClientConfigFactory, you seem to be indexing parsers by their
"label" attribute:
if(label.equals(parser))
This is dangerous IMHO since we
don't have a guarantee that multiple contributed parsers don't happen to have
the same labels. Also, in the PropertySet, parsers should be saved with ID In
order to allow multiple different translations access the same shared
configuration. I think you should better try and use the parser's ID as a key
rather than the label. For a possible implementation of indexing by label, see
the Proxy concept
below.
-
In IFTPClientConfigFactory getKeySet()
Javadoc seems to be wrong (refers to "name" attribute but returns "label"
attribute). I think that for type safety, the API should better return a
String[] list rather than an untyped Set. Or, even better yet, you should
define a new class for those items of the extension point that are
configurable without activating the extension:
public class
FTPClientConfigProxy {
public String
getId();
public String
getLabel();
public int
getPriority();
public String
getSystemTypeRegex();
public String
getClassName();
public Bundle
getDeclaringBundle();
}
This is what most
code dealing with extension points in Eclipse does. It makes you a lot more
flexible -- given a particular ID, you can return the Proxy giving all the
info you need. You need to parse the extension registry into Proxies only
once. You can extend the Proxy in the future for adding more attributes easily
if you want -- for instance, a boolean attribute for
isSystemTypeRegexCaseSensitive; or, a String attribute for the "dir" command,
see below, or an icon, or... See class RSESystemType, for instance, it is such
a Proxy as well. (You need the declaringBundle for instanciating the class
lazily later, or for loading icons). At the same time, such a proxy makes it
easier to register extensions even outside Eclipse when the extension registry
is not
available.
-
Custom "dir" command: You are right that
the FTP client translates this into a LIST command, but for VxWorks it needs
to send "LIST -l" or the output is unusable. Perhaps other FTP servers are
similar in the way that some attributes of the directory only get visible with
certain options to the LIST command. Therefore, the parser should be
associated with the corresponding command to be sent (and especially the
options to be
sent).
Don't get me wrong, I really like the code as it is
now, and my comments are just to further optimize
it.
If you have any questions, I'll happily
answer.
Thanks,
--
Martin
Oberhuber
Wind River Systems, Inc.
Target Management Project Lead, DSDP
PMC Member
http://www.eclipse.org/dsdp/tm
Hi,
I have submitted an improved version of the FTP parser,
so now the contributed FTP parsers can contain a regular _expression_ and
priority number that should match the contents of the reply of the SYST FTP
command.
This improvement has been
tested with the following servers:
OpenVMS (VMS)
Filezilla
(UNIX)
Windows NT
(WinNT)
Verifying that the correct
parser was assigned using the regexp as well as when the correct parser was
specified on the FTP Settings dialog.
Regards,
Javier Montalvo OrĂºs
Development Tools
Symbian
Software Limited.
"Oberhuber, Martin"
<Martin.Oberhuber@xxxxxxxxxxxxx>
14/05/2007 14:13
|
To
| <javier.montalvoorus@xxxxxxxxxxx>
|
cc
| "Target Management developer
discussions" <dsdp-tm-dev@xxxxxxxxxxx>
|
Subject
| FTP Listing Parser Extension
Point |
|
Hello Javier,
I noticed that you fixed the issues (1) and
(2)
I raised for the FTP Listing Parser extension
point.
I'm
wondering if you have any plans to also work
on issue (3) -- allowing
contributed listing
parsers to take part in the autodetect?
What
I'd like to see is the following optional
additional attributes for the
extension point:
ftpSystemTypes =
".*VxWorks.*|.*VXWORKS.*"
specifying a regular _expression_ to match against
the result of the SYST command, such that the
parser would take part
in autodetect only when
it matches;
ftpDirCommand = "dir
-l"
an optional command string to send if the given
listing parser is
active. Perhaps also a third one:
priority = "100"
giving a
priority to know what parser should be
preferred if multiple parsers match
the systemTypes
pattern and are thus candidates for autodetect.
If
any of these optional attributes is not given,
the current behavior would
be the default.
Even if the current implementation doesn't care
for
these attributes in the first implementation,
I think I'd like to see these
specified.
What do you think? What are your
plans?
Thanks,
--
Martin Oberhuber
Wind River Systems,
Inc.
Target Management Project Lead, DSDP PMC
Member
http://www.eclipse.org/dsdp/tm
-----Original
Message-----
From:
dsdp-tm-dev-bounces@xxxxxxxxxxx
[mailto:dsdp-tm-dev-bounces@xxxxxxxxxxx] On
Behalf Of Oberhuber, Martin
Sent: Friday, May 04, 2007 4:00 PM
To:
javier.montalvoorus@xxxxxxxxxxx
Cc: Target Management developer
discussions
Subject: [dsdp-tm-dev] FTP Listing Parser Extension
Point
Hello Javier,
thanks for moving the FTP Listing Parser
extension point
from Services to the Subsystem. Seeing the code, I'm
actually more convinced than ever that this was the
right move -- the
dropdown and extension point handling
are now all nicely in one
plugin.
There is just few concerns I still have:
1. Avoid early
plugin activation for extensions.
2. Translatable label of the FTP Parser
extension.
3. Autodetect.
As I have mentioned before, the
AUTO special parser should
not be restricted to the preconfigured
parsers only. User
contributed parsers should also be
considered.
For doing that, I'd suggest that the extension has an
additional attribute "ftpSystemTypes" holding a regular
_expression_ to be matched against the result of the FTP
SYST command.
If multiple parsers match, they should be
tried one after the other.
An RSEDelegatingFtpListingParser
could do this work.
See my
previous E-Mail comments that I sent you below for reference.
Thanks,
--
Martin Oberhuber
Wind River Systems,
Inc.
Target Management Project Lead, DSDP PMC
Member
http://www.eclipse.org/dsdp/tm
________________________________
From: Oberhuber,
Martin
Sent: Friday, April 20, 2007 6:10 PM
To:
javier.montalvoorus@xxxxxxxxxxx
Cc: Target Management developer
discussions
Subject: New FTP Listing Parser Extension
Point
Hello Javier,
two more considerations:
*
We should avoid having
to activate plugins which contribute FTP
parsers, just in order to check
whether they are valid. Therefore, the
extension point should have another
attribute "systemNames" which should
contain a regular _expression_ that
needs to match the system name
reported by the remote side. If it matches,
AUTO would activate the
plugin, instantiate the parser, and try that parser
on that system.
Cheers,
Martin
Oberhuber, Martin
schrieb:
Hello
Javier,
I looked at
your patch and it looks good to me.
I have no time right now to test it with VxWorks, but
I'm
confident
that it will work fine. Your patch looks sizeable, so please
go
ahead
and
commit it.
What I would
like to see going forward, is the ability that the
"AUTO" parser type also tries the various
contributed parsers.
Right now, when I'm not mistaken, any extender-contributed
parser will only be used when
explicitly selected by the user.
This is not user-friendly. We should allow extenders
to
contribute
parsers, but the system should be able to select the contributed
parsers automatically
wherever possible. In the simplest case,
the parser contribution could have an
boolean
isValidFor(String systemType)
method that allows auto-selecting it based on the
systemType
returned
by the remote. In case multiple contributed parsers
return TRUE for isValidFor(), they could be
tried one after the
other while parsing, until the first one is successful.
Thanks,
--
Martin Oberhuber
Wind River Systems, Inc.
Target Management Project Lead, DSDP PMC Member
http://www.eclipse.org/dsdp/tm
<http://www.eclipse.org/dsdp/tm>
<http://www.eclipse.org/dsdp/tm>
--
Martin Oberhuber
Wind River Systems, Inc.
Target
Management Project Lead, DSDP PMC
Member
http://www.eclipse.org/dsdp/tm
_______________________________________________
dsdp-tm-dev
mailing
list
dsdp-tm-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/dsdp-tm-dev
**********************************************************************
Symbian Software Ltd is a company registered in England and Wales with
registered number 4190020 and registered office at 2-6 Boundary Row,
Southwark, London, SE1 8HP, UK. This message is intended only for use by the
named addressee and may contain privileged and/or confidential information. If
you are not the named addressee you should not disseminate, copy or take any
action in reliance on it. If you have received this message in error please
notify postmaster@xxxxxxxxxxx and delete the message and any attachments
accompanying it immediately. Neither Symbian nor any of its Affiliates accepts
liability for any corruption, interception, amendment, tampering or viruses
occurring to this message in transit or for any message sent by its employees
which is not in compliance with Symbian corporate policy.
**********************************************************************