Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shortcuts #3710

Merged
merged 6 commits into from Jul 20, 2020
Merged

Shortcuts #3710

merged 6 commits into from Jul 20, 2020

Conversation

mwganson
Copy link
Contributor

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written
  • Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.19 Changelog Forum Thread.


mwganson and others added 2 commits July 17, 2020 16:41
….getCommandShortcut() and Gui.getCommandInfo() where user has changed shortcut via customize dialog, but these 2 commands returned previous default information
#endif
std::string shortcutTxt = action->shortcut().toString().toStdString();

#if PY_MAJOR_VERSION >= 3
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that all pre-processor directives in this project are placed at column 1, so it should probably stay this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks for reviewing. I was thinking I had accidentally moved them during editing. Not sure if people are still building for python 2 anyway at this point. In retrospect we should have made all the #if's read #if python 2 instead of #if python 3 as that would simplify removing them when it comes time to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, our test matrix in Travis does build for Python 2; so until we completely deactivate this test, we should still maintain Python 2 compatibility. And about removing Python 2 support, I'm not sure how that will be handled. I guess we will passively ignore Python 2 rather than actively remove Python 2 code.


#if PY_MAJOR_VERSION >= 3
PyObject* strMenuTxt = PyUnicode_FromString(menuTxt ? menuTxt : "");
PyObject* strMenuTxt = PyUnicode_FromString(menuTxt ? menuTxt : "");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a tab character. It should be spaces.


#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 ? shortcutTxt : "");
PyObject* strShortcutTxt = PyUnicode_FromString(!shortcutTxt.empty() ? shortcutTxt.c_str() : "")
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the final semi-colon ; is missing here. The unit test with Python 3 fails because of it.

@mwganson
Copy link
Contributor Author

Some shortcuts are unable to be set, but after loading all of my installed workbenches and testing setting shortcuts for all commands it succeeds about 84.5% of the time. Randomly, I tried to set the shortcuts for some of the failed commands using the customize dialog, which also fails for these same commands (assign button is always grayed out). In the terminal window I get a warning message that these commands are not yet implemented. From what I can tell with limited testing if you can't set the shortcut in the python console with Gui.setCommandShortcut() you can't do it from the customize dialog either.

I used this macro to do the tests:

from PySide import QtGui

keys = Gui.listWorkbenches().keys()
for k in keys:
    Gui.activateWorkbench(k)
    QtGui.QApplication.processEvents()

cmds = Gui.listCommands()
for c in cmds:
    s = Gui.getCommandShortcut(c)
    if len(s)==0:
        s = "None"
    Msg(c + "-->" + s + "\n")

for c in cmds:
    #s = Gui.getCommandInfo(c)
    s = Gui.getCommandShortcut(c)
    if len(s)==0:
        s = "None"
    Msg(c + "-->" + str(s) + "\n")

Msg('Testing...\n')
successes = 0
fails = 0
for c in cmds:
    old = Gui.getCommandShortcut(c)
    new = "Ctrl+Alt+Shift+T"
    Gui.setCommandShortcut(c,new)
    QtGui.QApplication.processEvents()
    Msg('command: '+c+'-->' + Gui.getCommandShortcut(c) + '\n')
    if Gui.getCommandShortcut(c) != new:
        Err('Unable to set shortcut for command: '+str(c)+'\n')
        fails += 1
    else:
        successes += 1
    Gui.setCommandShortcut(c,old)
    QtGui.QApplication.processEvents()
    Msg('command: '+c+'-->' + Gui.getCommandShortcut(c) + '\n')
Msg('Shortcut set successes = '+str(successes)+', fails = '+str(fails)+', rate = '+str(float(successes/(successes+fails)))+'\n')

shortcut_test_output.txt

@yorikvanhavre
Copy link
Member

Excellent addition anyway! The commands that fail to get a shortcut indeed seem like another problem.

@yorikvanhavre yorikvanhavre merged commit 8565736 into FreeCAD:master Jul 20, 2020
@mwganson mwganson deleted the shortcuts branch July 20, 2020 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants