<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Daniel,<div>I am pretty sure this test case used to pass. However, I did reproduce the failure you see.</div><div><br></div><div>As for fixing it, I am not sure passing "false" there would be the right thing. Doing so would make it impossible for users to replace their own scripted commands.</div><div>If you look at AddUserCommand(), it says:</div><div><br></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(0, 132, 0);"><span style="color: #000000"> </span>// do not allow replacement of internal commands</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> <span style="color: #bb2ca2">if</span> (<span style="color: #31595d">CommandExists</span>(name_cstr))</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> <span style="color: #bb2ca2">if</span> (can_replace == <span style="color: #bb2ca2">false</span>)</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> <span style="color: #bb2ca2">return</span> <span style="color: #bb2ca2">false</span>;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> <span style="color: #bb2ca2">if</span> (<span style="color: #4f8187">m_command_dict</span>[<span style="color: #3d1d81">name</span>]-><span style="color: #31595d">IsRemovable</span>() == <span style="color: #bb2ca2">false</span>)</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> <span style="color: #bb2ca2">return</span> <span style="color: #bb2ca2">false</span>;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> }</div></div><div><br></div><div>Regardless of can_replace, a command can explicitly mark itself as non-removable, and that is the way system commands are supposed to be marked.</div><div>If you look at CommandObjectFrame, it is a MultiwordCommand, and for these:</div><div><br></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(187, 44, 162);"><span style="color: #000000"> </span>virtual<span style="color: #000000"> </span>bool</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"> IsRemovable() <span style="color: #bb2ca2">const</span> { <span style="color: #bb2ca2">return</span> <span style="color: #4f8187">m_can_be_removed</span>; }</div></div><div><br></div><div>The *real* bug seems to be that m_can_be_removed is never initialized when the constructor runs.</div><div><br></div><div>My proposed fix (I have not prepared a patch for it because I am currently busy with a different task):</div><div>a) the constructor for CommandObjectMultiword should initialize m_can_be_removed - my guess is that the default value should be false (because that is also the default for CommandObject)</div><div>b) if necessary, individual multiword commands can change the value of the flag - or third-parties can call SetRemovable() - with the right value</div><div><br></div><div>As a side note, plugin multiword commands already get this right by calling SetRemovable(true) (SBCommandInterpeter.cpp lines 416 and 470)</div><div><br></div><div>I will gladly appreciate any feedback on this proposal.</div><div>Assuming there are no objections, Daniel, feel free to write a patch to this effect, or if you are already busy, let me know and I will do this ASAP.</div><div><br></div><div>Thanks,</div><div><div>
<div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div style="font-size: medium; orphans: 2; widows: 2; border-collapse: separate; border-spacing: 0px;"><span style="font-size: 12px; orphans: auto; widows: auto;">Enrico Granata</span><br style="font-size: 12px; orphans: auto; widows: auto;"><span style="font-size: 12px; orphans: auto; widows: auto;">✉ egranata@</span><font color="#ff2600" style="font-size: 12px; orphans: auto; widows: auto;"></font><span style="font-size: 12px; orphans: auto; widows: auto;">.com</span><br style="font-size: 12px; orphans: auto; widows: auto;"><span style="font-size: 12px; orphans: auto; widows: auto;">✆ 27683</span></div></div>
</div>
<br><div><div>On Feb 20, 2013, at 2:36 PM, "Malea, Daniel" <<a href="mailto:daniel.malea@intel.com">daniel.malea@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hi,<br><br>I noticed the TestCommandScript.py test case is failing one line 118, where it verifies that a user cannot redefine a command named 'frame'.<br><br>I believe the failure is due to CommandObjectCommands.cpp passing the wrong value (true) for the 'can_replace' parameter to CommandInterpreter::AddUserCommand… When this is set to 'true', which allows the test-case to re-define the 'frame' command to be something else.<br><br>Attached patch should fix it. Any concerns/comments, or can I commit this as-is?<br><br>I'm still not exactly sure how CommandObjectCommands works, and why there's two code paths that end up calling AddUserCommand..so I figured I'd run it by the list first.<br><br><br>Dan<br><br><br><span><fix_test_command_script.patch></span><br><br></blockquote></div><br></div></body></html>