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

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #2971620, was opened at 2010-03-16 22:59
Message generated for change (Comment added) made by patrickbernaud
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: Patrick Bernaud (patrickbernaud)
Date: 2010-03-18 18:09

Message:
You are right, this is not a bug. Sorry.

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

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

Message:
What bug to reference?  This is just a code cleanup and feature addition
(originally prompted by a post from DJ to the mailing list if I remember
correctly, I guess I could reference that). 

I'll address the other concerns when I get a minute.  Thanks for your
comments.

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

Comment By: Patrick Bernaud (patrickbernaud)
Date: 2010-03-18 11:37

Message:
Please make sure the bug is referenced in the commit message.

In documentation I think you meant 'preserves', not 'preseves'.

Maybe move the display of action (when verbose mode is turned on) before
the get_coord() in hid_actionv().

Finally consider making a separate patch regarding the search for spaces
in action names. But note that spaces are only part of the problem: no
spaces but no parenthesis either for example. We actually have to define
what characters are really allowed in action names.

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

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-18 06:43

Message:
Okay, added a revised patch.   Changes from the last one:

- Checks for spaces in action names at register time, which removes
ambiguieties and allows the action parser to be more tolerant, allowing
things like 'Select ElementByName "("', which wouldn't have worked before. 
It uses the method DJ suggested to determine whether or not to use parens.

- There are now two functions, hid_parse_command which will take either
form and hid_parse_actions which only takes the paren-style.  they both
just call a new static function hid_parse_actionstring with a bool to
indicate whether or not parens are required or not.

- The extra argument to give a function that should be called for each
action was removed, hid_actionv is always called.  All calls to
hid_parse_action were updated or changed to hid_parse_command as
appropriate.

- Also remove command_parse from batch/batch.c which was a copy of
command_parse from lesstif/main.c.

- Updates the manual to be more clear about how to type in commands and
some notes about quoting.

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

Comment By: Peter Clifton (petercjclifton)
Date: 2010-03-17 20:00

Message:
"I don't think ".cmd" files should parse "command" style actions"

(I know the above sounds rather silly, but I think you know what I mean).

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

Comment By: Peter Clifton (petercjclifton)
Date: 2010-03-17 19:59

Message:
Ok, I was wrong - thanks for looking into it though!

I think it might be useful to retain the command / action distinction
though, I don't think ".cmd" files should parse "command" style actions
such as "w ..". I'm not in favour of liberalising the action syntax
accepted by hid_parse_actions.

Perhaps having a common "command_parse" function which takes user-input
from the GUI (either a command or action() string), and calls
hid_parse_action would be good.

I don't feel especially strongly on the issue though.

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

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 19: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 19: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 19: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 19: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 18: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 17:59

Message:
That sounds reasonable.

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

Comment By: Jared Casper (jaredcasper)
Date: 2010-03-17 16: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 16: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 16: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 09: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 09: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