From: SourceForge.net <no...@so...> - 2010-03-20 17:33:14
|
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-20 10:33 Message: This will fix the compilation bug reported in geda-user, and will conflict if someone pushes a patch for that before this. Any chance of getting it pushed before then? :) ---------------------------------------------------------------------- Comment By: Patrick Bernaud (patrickbernaud) Date: 2010-03-19 04:25 Message: Looks good to me. ---------------------------------------------------------------------- Comment By: Jared Casper (jaredcasper) Date: 2010-03-18 23:45 Message: Okay, two more patches added. First one doesn't check the action name upon registration and includes the spelling fix in the doc. Second one checks the action name for a space and a '(' which are the only two which should cause hid_parse_actionstring to croak, I think. :) Thanks for your patience. :) ---------------------------------------------------------------------- Comment By: Patrick Bernaud (patrickbernaud) Date: 2010-03-18 11:09 Message: You are right, this is not a bug. Sorry. ---------------------------------------------------------------------- Comment By: Jared Casper (jaredcasper) Date: 2010-03-18 10: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 04: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-17 23: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 13: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 12: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 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 |