Bugzilla – Full Text Bug Listing
| Summary: | [console] Help in FrameworkCommandProvider | ||
|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Lazar Kirchev <l.kirchev> |
| Component: | Framework | Assignee: | Lazar Kirchev <l.kirchev> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | minor | ||
| Priority: | P3 | CC: | tjwatson |
| Version: | 3.7 | ||
| Target Milestone: | 3.7 M7 | ||
| Hardware: | PC | ||
| OS: | Windows Vista | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 340290, 340291, 340293, 340296 | ||
| Attachments: | |||
Created attachment 187403 [details]
Updated patch for help in FrameworkCommandProvider.
Update of the previously submitted patch.
Thanks Lazar! I am not sure on the "display" command. 1) The name implies a UI or text "display" to me. 2) It is only helpful for commands provided by the framework command provider. It would be good if we could make this command handle all other commands from other providers. (In reply to comment #2) > Thanks Lazar! > > I am not sure on the "display" command. > > 1) The name implies a UI or text "display" to me. I first was thinking to call it man or help, but if currently one writes one of these (as well as any other string, which is not a command name), all help for all commands of all command providers will be present. If I chose one of these (these are the most intuitive names), it would override the current behavior. This is directly related to the second point, > 2) It is only helpful for commands provided by the framework command provider. > It would be good if we could make this command handle all other commands from > other providers. I also was thinking if it is possible to provide this command for arbitrary command providers. But the only thing on which we can really count on regarding the help from an arbitrary provider is the getHelp() method, which returns the help for all its commands. Do you know any means to get only the help for a single command of an arbitrary command provider? I just could not think of such a way, that is why I added the command to the FrameworkCommandProvider - we have control over it. As for the name, I may change it to man or help. In this case I will have to make some changes in the way the FrameworkCommandInterpreter executes this command - if it has no parameters, then all help for all providers should be displayed. If it has parameter, then it should be able to detect if the command was successfully executed or not (e.g., it is called with the name of an nonexistent command as an argument). If the execution was not successful on all command providers, then help for all should be displayed. I can make this modification and update the patch. But this will not solve the problem that the command will be valid only for the FrameworkCommandProvider. (In reply to comment #3) Well, actually I can think of one way to provide this command for all command providers - the FrameworkCommandInterpreter may parse result of getHelp() from all command providers. But this may be rather tricky since there is no common format for the output of getHelp(). (In reply to comment #4) > (In reply to comment #3) > Well, actually I can think of one way to provide this command for all command > providers - the FrameworkCommandInterpreter may parse result of getHelp() from > all command providers. But this may be rather tricky since there is no common > format for the output of getHelp(). One way could be to add an optional param to the help command implementation, for example: help ss The help command implementation could then reflectively check all command providers for a special help method (help_ss()). Or check for a help(String command) method, I think this route may be a little dangerous since some command providers may already have a method help(String). Either way this would require some change to all command providers to provide the extra help. (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > Well, actually I can think of one way to provide this command for all command > > providers - the FrameworkCommandInterpreter may parse result of getHelp() from > > all command providers. But this may be rather tricky since there is no common > > format for the output of getHelp(). > > One way could be to add an optional param to the help command implementation, > for example: > > help ss > > The help command implementation could then reflectively check all command > providers for a special help method (help_ss()). Or check for a help(String > command) method, I think this route may be a little dangerous since some > command providers may already have a method help(String). Either way this > would require some change to all command providers to provide the extra help. What worries me here are the modifications to the command providers. In Equinox we have two - the EclipseCommandProvider and the FrameworkCommandProvider. It is ok to make changes in them. But what about the others? There is a command provider in DS, in P2,... While we still could make changes to these, there are custom command providers, which are outside any Eclipse project and which we cannot change. Then for these user command providers any logic for displaying help for a single command will not work. But still I think that if we could have the additional help functionality even only for out command providers, this will be better than the current situation, in which help for a particular command cannot be acquired. (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > Well, actually I can think of one way to provide this command for all command > > > providers - the FrameworkCommandInterpreter may parse result of getHelp() from > > > all command providers. But this may be rather tricky since there is no common > > > format for the output of getHelp(). > > > > One way could be to add an optional param to the help command implementation, > > for example: > > > > help ss > > > > The help command implementation could then reflectively check all command > > providers for a special help method (help_ss()). Or check for a help(String > > command) method, I think this route may be a little dangerous since some > > command providers may already have a method help(String). Either way this > > would require some change to all command providers to provide the extra help. > > What worries me here are the modifications to the command providers. In Equinox > we have two - the EclipseCommandProvider and the FrameworkCommandProvider. It > is ok to make changes in them. But what about the others? There is a command > provider in DS, in P2,... While we still could make changes to these, there are > custom command providers, which are outside any Eclipse project and which we > cannot change. Then for these user command providers any logic for displaying > help for a single command will not work. > > But still I think that if we could have the additional help functionality even > only for out command providers, this will be better than the current situation, > in which help for a particular command cannot be acquired. We could come up with a solution like I outlined in comment 5 and update the providers in equinox to make help better. For other commands the implementation could detect that the help_<command>() method is not found and simply display the complete help for all commands as before. Is that what you are suggesting? (In reply to comment #7) > We could come up with a solution like I outlined in comment 5 and update the > providers in equinox to make help better. For other commands the > implementation could detect that the help_<command>() method is not found and > simply display the complete help for all commands as before. Is that what you > are suggesting? Yes, I meant that. Only one thing - what do you mean by providers in equinox? Only in the framework bundle, or also in the compendium and p2 bundles? According to me, the more providers we improve, the better. (In reply to comment #8) > (In reply to comment #7) > > > We could come up with a solution like I outlined in comment 5 and update the > > providers in equinox to make help better. For other commands the > > implementation could detect that the help_<command>() method is not found and > > simply display the complete help for all commands as before. Is that what you > > are suggesting? > > Yes, I meant that. Only one thing - what do you mean by providers in equinox? > Only in the framework bundle, or also in the compendium and p2 bundles? > According to me, the more providers we improve, the better. Yes, I meant all providers in equinox (p2, compendium etc.). (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > > We could come up with a solution like I outlined in comment 5 and update the > > > providers in equinox to make help better. For other commands the > > > implementation could detect that the help_<command>() method is not found and > > > simply display the complete help for all commands as before. Is that what you > > > are suggesting? > > > > Yes, I meant that. Only one thing - what do you mean by providers in equinox? > > Only in the framework bundle, or also in the compendium and p2 bundles? > > According to me, the more providers we improve, the better. > > Yes, I meant all providers in equinox (p2, compendium etc.). Fine, I will provide a patch.
> > Yes, I meant all providers in equinox (p2, compendium etc.).
>
> Fine, I will provide a patch.
I found the following command providers in org.eclipse.equinox project:
FrameworkCommandProvider and EclipseCommandProvider in the framework,
ProvCommandProvider (in p2 console) and ConfigurationCommandProvider (in simpleconfigurator) in p2,
RegistryCommandProvider (in equinox.registry) in components,
AppCommands (in equinox.app) and SCRCommandProvider(in DS) - in the compendium
Do you know of any others? I think these are all, but still I may be missing something.
Created attachment 187966 [details]
Proposed patch for the services comand and its help
In this patch I provide the change regarding the services command, which I explained in the bug description - the more detailed help of the command, and the ability to pass the <fully qualified name> of the class as a shortcut for the filter (objectClass=<fully qualified name>). I separate this from the second part of the original patch, because they are not related.
In relation to displaying help for an individual command I was thinking about adding a traditional help command (with corresponding method public void _help()) in all of the Equinox command providers. Currently there is no such command in any of them. This command will display help only for the command, passed as parameter. I think this could be an alternative to adding a method help_<command_name>() or help(String) to the command providers and calling it reflectively. There may be a problem if a custom command provider has a _help command with different semantics, but we will have the same problem even if we add one of the two methods mentioned above. If the user enters help <some_string> we have to try to call _help on the custom command providers in addition to the Equinox command providers' help_<come_string> or help(<some_string>), and the effect will be the same. What do you think? (In reply to comment #13) > What do you think? Would the _help() method take a parameter to figure out what command to get help for? Do you call all _help methods until one returns non-null for the command you are asking help for? Using _help as the method name should be safe since the current implementation of the console will never search for an _help method because the "help" command is built-in and the command providers provide help text through getHelp() method. (In reply to comment #14) > (In reply to comment #13) > > What do you think? > > Would the _help() method take a parameter to figure out what command to get > help for? Do you call all _help methods until one returns non-null for the > command you are asking help for? Using _help as the method name should be safe > since the current implementation of the console will never search for an _help > method because the "help" command is built-in and the command providers provide > help text through getHelp() method. Currently there is no actual implementation of help - it displays help just because there is no matching command in any command provider with that name and in this case the FrameworkCommandInterpreter calls getHelp() to all command providers. The effect is the same if man is written on the command line, or any other string, which is not a name of a command. Actually, if currently a custom command provider provides a command with name help, the mechanism for giving help will not work. I was thinking about providing help as an ordinary command, with corresponding public void _help(CommandInterpreter) method in all Equinox command providers. The implementation of _help(CommandInterpreter) would output help message only if it can get another argument from the CommandInterpreter. I think the FrameworkCommandInterpreter should process the help command as any other command, with the difference that if it detects that help is called with an argument, it should call all _help methods (of all CommandProviders) - this in case that it happens two command providers to provide commands with the same name. This is currently possible and we cannot differentiate for which command exactly the user needs help. In the case when help is called with no arguments, the FrameworkCommandInterpreter should call all getHelp() methods, as it currently does. (In reply to comment #15) This approach sounds reasonable. Created attachment 190859 [details]
Proposed patch for adding help for a specific command
Sorry for the delay of this patch.
This patch adds the functionality to provide help for a specific command only. As discussed in the bug, it adds a method _help(CommandInterpreter) to the CommandProviders. This method gets as parameter a name of command and returns the help message for this command.
FrameworkCommandInterpreter is also updated to try all CommandProviders for help commands.
Currently FrameworkCommandProvider and EclipseCommandProviders are updated with the new _help method.
If this change is approved, I will propose similar patches for the other CommandProviders in Equinox.
Created attachment 190930 [details]
Proposed patch improvements
Thanks Lazar for the patch!
I reviewed the patch and made a few changes.
1) There was a use of String.contains method which is not available on foundation.
2) I tried to avoid duplication of all the help string construction code by make a single method that takes null for all help or a commandName for a specific command help.
3) I moved the help description for the "help <command>" to FrameworkCommandInterpreter just like for more and disconnect.
Let me know what you think. Thanks.
Thanks a lot Tom for the improvements! Without the duplications it looks better. I will open bugs in the other Equinox components which provide commands and will make patches to add the new help method. Created attachment 190976 [details]
A minor change to the proposed patch
I just saw that in EclipseCommandProvider I used equals() instead of equalsIgnoreCase() when comparing the command names. I fixed this.
Created attachment 190977 [details]
Proposed patch minor improvement
Just a very minor change regarding abbreviations for commands in FrameworkCommandProvider. Whole names and abbreviations were handled separately, which would cause both of them to appear when help for all commands is taken. This patch handles together whole names and abbreviations.
(In reply to comment #20) > Created attachment 190976 [details] > A minor change to the proposed patch > > I just saw that in EclipseCommandProvider I used equals() instead of > equalsIgnoreCase() when comparing the command names. I fixed this. I did not notice equalsIngoreCase was being used in FrameworkCommandProvider. I think equals is actually appropriate since I don't thing we process the command we execute by ignoring case. For example, you cannot run the "INSTALL" command, it has to be lower case "install" (In reply to comment #21) > Created attachment 190977 [details] > Proposed patch minor improvement > > Just a very minor change regarding abbreviations for commands in > FrameworkCommandProvider. Whole names and abbreviations were handled > separately, which would cause both of them to appear when help for all commands > is taken. This patch handles together whole names and abbreviations. I thought my original patch handled that by an if (all ...) check. I am beginning to think these abbreviated commands are a pretty bad idea. I kinda wish they were never present. (In reply to comment #22) > (In reply to comment #20) > > Created attachment 190976 [details] [details] > > A minor change to the proposed patch > > > > I just saw that in EclipseCommandProvider I used equals() instead of > > equalsIgnoreCase() when comparing the command names. I fixed this. > > I did not notice equalsIngoreCase was being used in FrameworkCommandProvider. > I think equals is actually appropriate since I don't thing we process the > command we execute by ignoring case. For example, you cannot run the "INSTALL" > command, it has to be lower case "install" I will provide another patch, where equals() is used instead of equalsIgnoreCase(). > > (In reply to comment #21) > > Created attachment 190977 [details] [details] > > Proposed patch minor improvement > > > > Just a very minor change regarding abbreviations for commands in > > FrameworkCommandProvider. Whole names and abbreviations were handled > > separately, which would cause both of them to appear when help for all commands > > is taken. This patch handles together whole names and abbreviations. > > I thought my original patch handled that by an if (all ...) check. I am > beginning to think these abbreviated commands are a pretty bad idea. I kinda > wish they were never present. Sorry, this was my mistake, the if (all ...) is correct. Do not consider this patch. Created attachment 190983 [details]
Proposed patch improvement
This patch changes the improved patch, proposed by Tom, so that FrameworkCommandProvider compares commands names with equals() instead of equalsIgnoreCase().
Should I open bugs in the other components to submit patches for the other command providers in Equinox, or I can attach the patches to this bug? (In reply to comment #24) > Created attachment 190983 [details] > Proposed patch improvement > > This patch changes the improved patch, proposed by Tom, so that > FrameworkCommandProvider compares commands names with equals() instead of > equalsIgnoreCase(). Go ahead and open new bugs. (In reply to comment #26) > (In reply to comment #24) > > Created attachment 190983 [details] [details] > > Proposed patch improvement > > > > This patch changes the improved patch, proposed by Tom, so that > > FrameworkCommandProvider compares commands names with equals() instead of > > equalsIgnoreCase(). > > Go ahead and open new bugs. Do you think it makes sense to hold all help massages in a CommandProvider in a map, keyed by the command name? This would simplify the code in the method private getHelp(String commandName). If the commandName is null, then all map entries will be iterated and appended to the result help buffer. If the commandName is not null, no checks will be necessary - only a lookup in the map. This will make the code in getHelp() more clear and straightforward. If you agree with this change, I will provide a changed patch for the FrameworkCommandProvider and EclipseCommandProvider, and will include this change in the patches for the other providers. (In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #24) > > > Created attachment 190983 [details] [details] [details] > > > Proposed patch improvement > > > > > > This patch changes the improved patch, proposed by Tom, so that > > > FrameworkCommandProvider compares commands names with equals() instead of > > > equalsIgnoreCase(). > > > > Go ahead and open new bugs. > > Do you think it makes sense to hold all help massages in a CommandProvider in a > map, keyed by the command name? This would simplify the code in the method > private getHelp(String commandName). If the commandName is null, then all map > entries will be iterated and appended to the result help buffer. If the > commandName is not null, no checks will be necessary - only a lookup in the > map. This will make the code in getHelp() more clear and straightforward. > > If you agree with this change, I will provide a changed patch for the > FrameworkCommandProvider and EclipseCommandProvider, and will include this > change in the patches for the other providers. That would be fine, if you want to tweak the implementation. But I suggest that the map only get created and populated on the first request for help content.
> That would be fine, if you want to tweak the implementation. But I suggest
> that the map only get created and populated on the first request for help
> content.
Yes, it sounds reasonable to init the map upon first request. I will make the necessary changes.
Created attachment 191389 [details]
Proposed improvement of the patch
This improvement stores the mapping from commands to help messages in a map to simplify the code for getting the help message.
The patch here also solves the problem described in bug 335814. Just a comment to be clear how exactly a command provider is enabled to provide help for an individual command: the command provider provides method Object _help(CommandInterpreter), which returns either the help message of the command, or false, if it does not provide a command with this name. Lazar, quick question on the services command improvements. I don't understand the following starting at line 634 in FrameworkCommandProvider
if (originalException != null &&
!filter.startsWith("(") &&
filter.indexOf(' ') > -1) {
^^^^^^^^^^^^^^^^^^^^^^^^
What is the purpose of this check?
Why do we care if it has a space?
try {
filter = "(" + Constants.OBJECTCLASS + "=" + filter + ")";
services = context.getServiceReferences((String) null, filter);
} catch (InvalidSyntaxException e) {
throw originalException;
}
}
Actually I messed up the check in my patch in comment 18. You used to have !filter.contains(' '). I reversed the logic. So why do we care that it does NOT contain a space? Is it because we assume a service interface class name cannot contain spaces? I'm not sure it is important to check that, can I remove the space check altogether?
(In reply to comment #33) > Lazar, quick question on the services command improvements. I don't understand > the following starting at line 634 in FrameworkCommandProvider > > > if (originalException != null && > !filter.startsWith("(") && > filter.indexOf(' ') > -1) { > ^^^^^^^^^^^^^^^^^^^^^^^^ > What is the purpose of this check? > Why do we care if it has a space? > > try { > filter = "(" + Constants.OBJECTCLASS + "=" + filter + ")"; > services = context.getServiceReferences((String) null, filter); > } catch (InvalidSyntaxException e) { > throw originalException; > } > } > > Actually I messed up the check in my patch in comment 18. You used to have > !filter.contains(' '). I reversed the logic. So why do we care that it does > NOT contain a space? Is it because we assume a service interface class name > cannot contain spaces? I'm not sure it is important to check that, can I > remove the space check altogether? Yes, it is because we assume that the interface class name cannot contain spaces. This heuristic is intended to cover only the case when the user has entered solely the interface class name - if there is a space we do not have this case and we should not try this heuristic at all. (In reply to comment #34) > > Yes, it is because we assume that the interface class name cannot contain > spaces. This heuristic is intended to cover only the case when the user has > entered solely the interface class name - if there is a space we do not have > this case and we should not try this heuristic at all. Thanks I released the patch to head and change the check to filter.indexOf(' ') < 0 Comment on attachment 191389 [details]
Proposed improvement of the patch
Thanks for the contribution Lazar!
Thanks! While I made the patches in the compendium bundles (app and ds) and in components (in the registry), I saw that they have J2SE 1.4 as an ExecutionEnvironment. But the framework uses 1.5 and uses functionality from 1.5. Probably the other projects should be updated? (In reply to comment #37) > Thanks! > > While I made the patches in the compendium bundles (app and ds) and in > components (in the registry), I saw that they have J2SE 1.4 as an > ExecutionEnvironment. But the framework uses 1.5 and uses functionality from > 1.5. Probably the other projects should be updated? Not at this time. The framework's source is 1.5, but we use jsr14 to compile it for running on 1.4 VMs so we still support 1.4 base VMs (e.g. Foundation). The other bundles should not move up to this type of configuration unless they have a need to. |
Created attachment 187302 [details] Proposed patch for help in FrameworkCommandProvider. I made some observations on the commands in the FrameworkCommandProvider: 1. help of the services command - I think it would be helpful if the help message of the command gives an example for a filter and a link to the RFC 1960, which species it. 2. it would be nice if there is a command which displays the help for a particular command - currently the only way to get help on a command is to list all commands help messages. I provide a patch for these two issues: 1. it adds to the help message of the services command two examples for filters - I have taken them from the javadoc of the Filter class - and also adds the URL of RFC 1960 if the user wants to check the syntax in more details. Also, I think that it is most probable that, if the user does not specify a filter, the user to specify directly the name of an interface for the service. So, if the passed filter argument causes an exception, I propose a small check to be made in the services command - if the argument does not start with a bracket and does not contain spaces, it may be a name of an interface and an object filter may be created with it. If with the so created filter again an exception is thrown, the method throws the first exception, caused by the original filter. 2. provides an additional command - display - for the second of the above issues.