[Pcb-bugs] [ pcb-Patches-2971620 ] Consolidate hid action parsing.

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Patches item #2971620, was opened at 2010-03-16 15:59
Message generated for change (Comment added) made by jaredcasper
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=538813&aid=2971620&group_id=73743

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: lesstif GUI
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Jared Casper (jaredcasper)
Assigned to: Nobody/Anonymous (nobody)
Summary: Consolidate hid action parsing.

Initial Comment:
Teach the common hid_parse_actions how to handle lesstif style simple
actions without parenthesis ("action arg1 arg2").

Added extra error handling to common hid_actionv to match
lesstif_call_action.

Remove lesstif_call_action and command_parse in favor of common
hid_actionv and hid_parse_actions.

----------------------------------------------------------------------

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 12:41

Message:
So to be clear, yes, the lesstif hid currently has a special parsing
function, it is called "command_parse" in src/hid/lesstif/main.c:630.  One
thing this patch does is fold the functionality that was in that function
into hid_parse_actions, and removes it.

----------------------------------------------------------------------

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 12:33

Message:
Currently, typing ":s test" in the GTK hid prints "unknown action `s test'"
on stdout, because hid_parse_actions doesn't understand that style of
command.  The CommandSaveLayout in command.c creates an action name "s"
that is an alias for the save action, but has nothing to do with how it is
called.  The command input that ':' brings up is parsed by
hid_parse_actions in action.c to make the argv/argc pair which is passed to
the action function (i.e. CommandsaveLayout if the action is 's').  

In the lesstif hid, there was a check to see if there were an '('s in the
string and if so call hid_parse_actions, otherwise, simply split at
whitespace and pass that to the action, allowing such things as ":s
filename".  There was nothing of the sort in the GTK hid.

----------------------------------------------------------------------

Comment By: Peter Clifton (petercjclifton)
Date: 2010-03-17 12:24

Message:
I'm not certain about that.. GTK Hid supports those too, and your
implication was that the lesstif HID had a special parsing function.

I think the ":s filename" stuff comes not from action.c, but the
confusingly similar "command.c"
Search for "CommandSaveLayout" for example.

----------------------------------------------------------------------

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 12:14

Message:
>From how the manual reads (I'll update that section and include it in this
patch), it sounds like the extra syntax is used for vi style commands...
i.e. type ":s filename" to save.  We would lose that functionality.   I'd
be fine with ditching it, but don't use the lesstif hid so am not used to
using those style commands.

----------------------------------------------------------------------

Comment By: Peter Clifton (petercjclifton)
Date: 2010-03-17 11:28

Message:
Why do we even need two different action syntax anyway?

Can't we just say actions have () around the parameters, and be done with
it?

----------------------------------------------------------------------

Comment By: DJ Delorie (djdelorie)
Date: 2010-03-17 10:59

Message:
That sounds reasonable.

----------------------------------------------------------------------

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 09:52

Message:
So we should explicitely forbid registering actions with spaces in the
name?

I'll work up a patch based on that.

----------------------------------------------------------------------

Comment By: DJ Delorie (djdelorie)
Date: 2010-03-17 09:36

Message:
I think the rule should be: The first [space,paren] you find determines the
parsing type.  Action names never have spaces in them, so if you see
Action<space> you use the space-parsing method, if you see Action<paren>
you use the paren-parsing method.  I suppose if you see
Action<space><paren> you could use the paren-parsing method, but blindly
scanning the whole string is wrong.

----------------------------------------------------------------------

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 09:25

Message:
Thanks for taking a look and for the comments!

Arg, I thought I had fixed the white space errors, sorry about that.  I
thought about removing the second parameter to hid_parse_actions, but
figured it wasn't harming anything and might be helpful in the future, but
if you think it isn't helpful then I can remove that as well.

The idea was that if the '(' character appears anywhere in the action
string, you can't use the "no parenthesis" syntax, which is how the current
lesstif hid works.  Allowing parenthesis in this case would mean requiring
actions to not have spaces in them to distinguish between "foo bar(1)" with
action name "foo bar" with argument "1" and action name "foo" with argument
"bar(1)".  Currently you can do both with "foo bar(1)" for the first and
foo("bar(1)") for the second.

I did think about checking for the case with an open parenthesis in quotes
as your comments suggests, but that seemed like a lot of work to do each
action call for a very uncommon case, especially since you could just use
the parenthesis syntax for that case.

Any more thoughts on this?

----------------------------------------------------------------------

Comment By: Patrick Bernaud (patrickbernaud)
Date: 2010-03-17 02:59

Message:
There is a problem in hid_parse_actions() in the way it is searching for
opening parenthesis as the start of parameter list.

For example, if the user type the following command: 
Select ElementByName, "(" 
what the hid_parse_actions() extracts as the command name is then:
Select ElementByName, "
(and of course parameters are also wrong).

----------------------------------------------------------------------

Comment By: Patrick Bernaud (patrickbernaud)
Date: 2010-03-17 02:15

Message:
What about now getting ride of the second parameter of hid_parse_actions()?

Also git is reporting two whitespace errors: on line 82 and 147.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=538813&aid=2971620&group_id=73743