Skip to content

Commit

Permalink
Gui: [skip ci] move Python functions for commands to its own class
Browse files Browse the repository at this point in the history
  • Loading branch information
@wwmayer
wwmayer committed Aug 2, 2020
1 parent ed4876a commit 9b529bc
Show file tree
Hide file tree
Showing 6 changed files with 327 additions and 177 deletions.
2 changes: 2 additions & 0 deletions src/Gui/Application.cpp
View file
Expand Up @@ -125,6 +125,7 @@
#include "ViewProviderLink.h"
#include "LinkViewPy.h"
#include "AxisOriginPy.h"
#include "CommandPy.h"

#include "Language/Translator.h"
#include "TaskView/TaskView.h"
Expand Down Expand Up @@ -472,6 +473,7 @@ Application::Application(bool GUIenabled)

Base::Interpreter().addType(&LinkViewPy::Type,module,"LinkView");
Base::Interpreter().addType(&AxisOriginPy::Type,module,"AxisOrigin");
Base::Interpreter().addType(&CommandPy::Type,module, "Command");
}

Base::PyGILStateLocker lock;
Expand Down
6 changes: 0 additions & 6 deletions src/Gui/Application.h
View file
Expand Up @@ -258,12 +258,6 @@ class GuiExport Application

static PyObject* sRunCommand (PyObject *self,PyObject *args);
static PyObject* sAddCommand (PyObject *self,PyObject *args);
static PyObject* sGetCommandInfo (PyObject *self,PyObject *args);
static PyObject* sListCommands (PyObject *self,PyObject *args);
static PyObject* sGetCommandShortcut (PyObject *self,PyObject *args);
static PyObject* sSetCommandShortcut (PyObject *self,PyObject *args);
static PyObject* sIsCommandActive (PyObject *self,PyObject *args);
static PyObject* sUpdateCommands (PyObject *self,PyObject *args);

static PyObject* sHide (PyObject *self,PyObject *args); // deprecated
static PyObject* sShow (PyObject *self,PyObject *args); // deprecated
Expand Down
171 changes: 0 additions & 171 deletions src/Gui/ApplicationPy.cpp
View file
Expand Up @@ -139,24 +139,6 @@ PyMethodDef Application::Methods[] = {
{"runCommand", (PyCFunction) Application::sRunCommand, METH_VARARGS,
"runCommand(string) -> None\n\n"
"Run command with name"},
{"isCommandActive", (PyCFunction) Application::sIsCommandActive, METH_VARARGS,
"isCommandActive(string) -> Bool\n\n"
"Test if a command is active"},
{"listCommands", (PyCFunction) Application::sListCommands, METH_VARARGS,
"listCommands() -> list of strings\n\n"
"Returns a list of all commands known to FreeCAD."},
{"getCommandInfo", (PyCFunction) Application::sGetCommandInfo, METH_VARARGS,
"getCommandInfo(string) -> list of strings\n\n"
"Usage: menuText,tooltipText,whatsThisText,statustipText,pixmapText,shortcutText = getCommandInfo(string)"},
{"getCommandShortcut", (PyCFunction) Application::sGetCommandShortcut, METH_VARARGS,
"getCommandShortcut(string) -> string\n\n"
"Returns string representing shortcut key accelerator for command."},
{"setCommandShortcut", (PyCFunction) Application::sSetCommandShortcut, METH_VARARGS,
"setCommandShortcut(string,string) > bool\n\n"
"Sets shortcut for given command, returns bool True for success"},
{"updateCommands", (PyCFunction) Application::sUpdateCommands, METH_VARARGS,
"updateCommands\n\n"
"Update all command active status"},
{"SendMsgToActiveView", (PyCFunction) Application::sSendActiveView, METH_VARARGS,
"deprecated -- use class View"},
{"sendMsgToFocusView", (PyCFunction) Application::sSendFocusView, METH_VARARGS,
Expand Down Expand Up @@ -1258,159 +1240,6 @@ PyObject* Application::sRunCommand(PyObject * /*self*/, PyObject *args)
}
}

PyObject* Application::sIsCommandActive(PyObject * /*self*/, PyObject *args)
{
char* pName;
if (!PyArg_ParseTuple(args, "s", &pName))
return NULL;

Command* cmd = Application::Instance->commandManager().getCommandByName(pName);
if (!cmd) {
PyErr_Format(Base::BaseExceptionFreeCADError, "No such command '%s'", pName);
return 0;
}
PY_TRY {
return Py::new_reference_to(Py::Boolean(cmd->isActive()));
}PY_CATCH;
}

PyObject* Application::sUpdateCommands(PyObject * /*self*/, PyObject *args)
{
if (!PyArg_ParseTuple(args, ""))
return NULL;

getMainWindow()->updateActions();
Py_Return;
}

PyObject* Application::sGetCommandShortcut(PyObject * /*self*/, PyObject *args)
{
char* pName;
if (!PyArg_ParseTuple(args, "s", &pName))
return NULL;

Command* cmd = Application::Instance->commandManager().getCommandByName(pName);
if (cmd) {

#if PY_MAJOR_VERSION >= 3
PyObject* str = PyUnicode_FromString(cmd->getAction() ? cmd->getAction()->shortcut().toString().toStdString().c_str() : "");
#else
PyObject* str = PyString_FromString(cmd->getAction() ? cmd->getAction()->shortcut().toString().toStdString().c_str() : "");
#endif
return str;
}
else {
PyErr_Format(Base::BaseExceptionFreeCADError, "No such command '%s'", pName);
return 0;
}
}

PyObject* Application::sSetCommandShortcut(PyObject * /*self*/, PyObject *args)
{
char* pName;
char* pShortcut;
if (!PyArg_ParseTuple(args, "ss", &pName, &pShortcut))
return NULL;

Command* cmd = Application::Instance->commandManager().getCommandByName(pName);
if (cmd) {
Action* action = cmd->getAction();
if (action){
QKeySequence shortcut = QString::fromLatin1(pShortcut);
QString nativeText = shortcut.toString(QKeySequence::NativeText);
action->setShortcut(nativeText);
bool success = action->shortcut() == nativeText;
/**
* avoid cluttering parameters unnecessarily by saving only
* when new shortcut is not the default shortcut
* remove spaces to handle cases such as shortcut = "C,L" or "C, L"
*/
QString default_shortcut = QString::fromLatin1(cmd->getAccel());
QString spc = QString::fromLatin1(" ");
ParameterGrp::handle hGrp = WindowParameter::getDefaultParameter()->GetGroup("Shortcut");
if (success && default_shortcut.remove(spc).toUpper() != nativeText.remove(spc).toUpper()){
hGrp->SetASCII(pName, pShortcut);
} else {
hGrp->RemoveASCII(pName);
}
return Py::new_reference_to(Py::Boolean(success));
} else {
return Py::new_reference_to(Py::Boolean(false));
}
}
else {
PyErr_Format(Base::BaseExceptionFreeCADError, "No such command '%s'", pName);
return NULL;
}
}


PyObject* Application::sGetCommandInfo(PyObject * /*self*/, PyObject *args)
{
char* pName;
if (!PyArg_ParseTuple(args, "s", &pName))
return NULL;
Command* cmd = Application::Instance->commandManager().getCommandByName(pName);
if (cmd) {
Action* action = cmd->getAction();
PyObject* pyList = PyList_New(6);
const char* menuTxt = cmd->getMenuText();
const char* tooltipTxt = cmd->getToolTipText();
const char* whatsThisTxt = cmd->getWhatsThis();
const char* statustipTxt = cmd->getStatusTip();
const char* pixMapTxt = cmd->getPixmap();
std::string shortcutTxt = "";
if(action)
shortcutTxt = action->shortcut().toString().toStdString();

#if PY_MAJOR_VERSION >= 3
PyObject* strMenuTxt = PyUnicode_FromString(menuTxt ? menuTxt : "");
PyObject* strTooltipTxt = PyUnicode_FromString(tooltipTxt ? tooltipTxt : "");
PyObject* strWhatsThisTxt = PyUnicode_FromString(whatsThisTxt ? whatsThisTxt : "");
PyObject* strStatustipTxt = PyUnicode_FromString(statustipTxt ? statustipTxt : "");
PyObject* strPixMapTxt = PyUnicode_FromString(pixMapTxt ? pixMapTxt : "");
PyObject* strShortcutTxt = PyUnicode_FromString(!shortcutTxt.empty() ? shortcutTxt.c_str() : "");
#else
PyObject* strMenuTxt = PyString_FromString(menuTxt ? menuTxt : "");
PyObject* strTooltipTxt = PyString_FromString(tooltipTxt ? tooltipTxt : "");
PyObject* strWhatsThisTxt = PyString_FromString(whatsThisTxt ? whatsThisTxt : "");
PyObject* strStatustipTxt = PyString_FromString(statustipTxt ? statustipTxt : "");
PyObject* strPixMapTxt = PyString_FromString(pixMapTxt ? pixMapTxt : "");
PyObject* strShortcutTxt = PyString_FromString(!shortcutTxt.empty() ? shortcutTxt.c_str() : "");
#endif
PyList_SetItem(pyList, 0, strMenuTxt);
PyList_SetItem(pyList, 1, strTooltipTxt);
PyList_SetItem(pyList, 2, strWhatsThisTxt);
PyList_SetItem(pyList, 3, strStatustipTxt);
PyList_SetItem(pyList, 4, strPixMapTxt);
PyList_SetItem(pyList, 5, strShortcutTxt);
return pyList;
}
else {
PyErr_Format(Base::BaseExceptionFreeCADError, "No such command '%s'", pName);
return 0;
}
}

PyObject* Application::sListCommands(PyObject * /*self*/, PyObject *args)
{
if (!PyArg_ParseTuple(args, ""))
return NULL;

std::vector <Command*> cmds = Application::Instance->commandManager().getAllCommands();
PyObject* pyList = PyList_New(cmds.size());
int i=0;
for ( std::vector<Command*>::iterator it = cmds.begin(); it != cmds.end(); ++it ) {
#if PY_MAJOR_VERSION >= 3
PyObject* str = PyUnicode_FromString((*it)->getName());
#else
PyObject* str = PyString_FromString((*it)->getName());
#endif
PyList_SetItem(pyList, i++, str);
}
return pyList;
}

PyObject* Application::sDoCommand(PyObject * /*self*/, PyObject *args)
{
char *sCmd=0;
Expand Down
3 changes: 3 additions & 0 deletions src/Gui/CMakeLists.txt
View file
Expand Up @@ -229,6 +229,7 @@ generate_from_xml(SelectionObjectPy)
generate_from_xml(LinkViewPy)
generate_from_xml(ViewProviderLinkPy)
generate_from_xml(AxisOriginPy)
generate_from_xml(CommandPy)

generate_from_py(FreeCADGuiInit GuiInitScript.h)

Expand All @@ -243,6 +244,7 @@ SET(FreeCADGui_XML_SRCS
LinkViewPy.xml
ViewProviderLinkPy.xml
AxisOriginPy.xml
CommandPy.xml
)
SOURCE_GROUP("XML" FILES ${FreeCADApp_XML_SRCS})

Expand Down Expand Up @@ -498,6 +500,7 @@ SET(Command_CPP_SRCS
CommandView.cpp
CommandStructure.cpp
CommandLink.cpp
CommandPyImp.cpp
)
SET(Command_SRCS
${Command_CPP_SRCS}
Expand Down
81 changes: 81 additions & 0 deletions src/Gui/CommandPy.xml
View file
@@ -0,0 +1,81 @@
<?xml version="1.0" encoding="UTF-8"?>
<GenerateModel xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="generateMetaModel_Module.xsd">
<PythonExport
Father="PyObjectBase"
Name="CommandPy"
Twin="Command"
TwinPointer="Command"
Include="Gui/Command.h"
FatherInclude="Base/PyObjectBase.h"
Namespace="Gui"
FatherNamespace="Base">
<Documentation>
<Author Licence="LGPL" Name="Werner Mayer" EMail="wmayer[at]users.sourceforge.net" />
<UserDocu>FreeCAD Python wrapper of Command functions</UserDocu>
</Documentation>
<Methode Name="get" Static='true'>
<Documentation>
<UserDocu>get(string) -> Command

Get a given command by name or None if it doesn't exist.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="update" Static='true'>
<Documentation>
<UserDocu>update() -> None

Update active status of all commands.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="listAll" Static='true'>
<Documentation>
<UserDocu>listAll() -> list of strings

Returns the name of all commands.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="run">
<Documentation>
<UserDocu>run() -> None

Runs the given command.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="isActive" Const="true">
<Documentation>
<UserDocu>isActive() -> bool

Returns True if the command is active, False otherwise.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="getShortcut">
<Documentation>
<UserDocu>getShortcut() -> string

Returns string representing shortcut key accelerator for command.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="setShortcut">
<Documentation>
<UserDocu>setShortcut(string) -> bool

Sets shortcut for given command, returns bool True for success.
</UserDocu>
</Documentation>
</Methode>
<Methode Name="getInfo">
<Documentation>
<UserDocu>getInfo() -> list of strings

Usage: menuText, tooltipText, whatsThisText, statustipText, pixmapText, shortcutText.
</UserDocu>
</Documentation>
</Methode>
</PythonExport>
</GenerateModel>

12 comments on commit 9b529bc

@vocx-fc
Copy link
Contributor

@vocx-fc vocx-fc commented on 9b529bc Aug 3, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This moves the commands but doesn't reimplement them with the same name, isCommandActive, listCommands, getCommandInfo, getCommandShortcut, setCommandShortcut, updateCommands.

This breaks things in scripts because some tools check for the existence of other tools.

if "My_Tool" in Gui.listCommands():
    something()

Something like this is needed.

if not hasattr(Gui, "listCommands"):
    Gui.listCommands = Gui.Command.listAll

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 9b529bc Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the handling with commands isn't something that belongs to the FreeCADGui module directly but rather to a sub-module or a class.
Some years ago we had exactly two functions in FreeCADGui to deal with commands: addCommand() and runCommand() that are used in several places in Python code and that was OK.

But over the years more and more functions have been added: listCommand (in 2016), isCommandActive (in 2019), updateCommands (in 2019), getCommandShortcut (in April 2020), getCommandInfo (in April 2020), setCommandShortcut (in July 2020) and listCommandByShortcut was about to be added next. Now, none of these functions added since 2016 are used anywhere in FreeCAD code.
In the first place I wonder why more and more functions are added which nobody uses.
In the second place when a header file is touched that is included from many source files I end up in re-compiling big parts of the application. And if a PR cannot be directly merged but requires to be merged locally I end up in re-compiling the stuff twice.

I was thinking for a longer time to move command specific functions to another place. Now that in the recent months more functions have been added regularly I just did it and refactored the code.
Since the functions have been moved to the FreeCADGui.Command class there is no need to keep the superfluous word "Command" in the function names and this makes it more consistent to the C++ API.

The only function that eventually might be used in some external add-ons is listCommands but all the others are too new in order to be used there. So, do you know of a concrete add-on that is affected by the change?

@vocx-fc
Copy link
Contributor

@vocx-fc vocx-fc commented on 9b529bc Aug 3, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, do you know of a concrete add-on that is affected by the change?

The BIM Workbench uses listCommands it to see if a particular command exists, loaded by another workbench, so it can set up its toolbars appropriately, yorikvanhavre/BIM_Workbench#59. I think Dodo may use it as well. In general I think this listCommands is pretty useful to handle undefined commands.

About the other functions, you are right that I haven't seen them used, so maybe they are fine being moved. However, I suspect TheMarkster, who implemented the latest CommandShortcut functions, wanted to use them to better manage the shortcuts of each workbench, as it is currently being discussed in the forum, [Volunteers welcome] Refactoring the keyboard shortcuts.

Also, what is your opinion on other Command functions, like doCommand, doCommandGui? Do you also plan on moving them, or it's best to keep them in the main Gui namespace?

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 9b529bc Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BIM Workbench uses listCommands it to see if a particular command exists

OK, thanks. This can be easily fixed by adding this line to FreeCADGuiInit.py:

Gui.listCommands = Gui.Command.listAll

About the other functions, you are right that I haven't seen them used, so maybe they are fine being moved.

OK, I will drop a note there.

Also, what is your opinion on other Command functions, like doCommand, doCommandGui? Do you also plan on moving them, or it's best to keep them in the main Gui namespace?

I don't have a string option on them. If I decide to move them as well I for sure will add an alias as above.

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 9b529bc Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

76e7429

@realthunder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wwmayer Some of these Python API have existed for some time. Removing it will likely break some existing Python workbench (well, it breaks my asm3 workbench). Would it be nicer to leave them there, and put some deprecation warning maybe?

@realthunder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the alias thing. So maybe add other aliases as well?

@wwmayer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the alias thing. So maybe add other aliases as well?

Which aliases exactly do you miss?

@realthunder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which aliases exactly do you miss?

I am using isCommandActive(). But looks like it can't be aliased just like that, because Command.isActive() expect the input to be a Command instance.

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 9b529bc Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two ways to achieve it:

  1. We can implement a regular function
def isCommandActive(cmd):
    return Gui.Command.get(cmd).isActive()
  1. We can use a lambda expression which is similar to an alias:
isCommandActive = lambda cmd: Gui.Command.get(cmd).isActive()

# Example:
isCommandActive("Std_New") # --> returns True

@wwmayer
Copy link
Contributor Author

@wwmayer wwmayer commented on 9b529bc Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 393da0d

@realthunder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's neat. Thanks!

Please sign in to comment.