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

Fix Object inspector message for OSX #2318

Merged
merged 3 commits into from
Apr 11, 2015
Merged

Conversation

blink1073
Copy link
Contributor

OS X uses Cmd+I as the default keyboard shortcut for the Object Inspector. This updates the help string to avoid confusion.

Fixes #2317

@goanpeca
Copy link
Member

Is the Ctrl+I or Cmd+I a global thing that will most likely remain unchanged? or is there someway to link the actual text that gets displayed to the shortcuts defined in Spyder. It would be more clean (and future proof)

Also, since there seems to be a lot of places around where we check for the OS, maybe it would be good idea to create some helper functions in spyderlib/utils:

is_win()

is_linux()

is_mac()

And adapt the code around spyder to make use of that (and of course add this to the docs 😸 )

@blink1073
Copy link
Contributor Author

The config is available as:

In [9]: from spyderlib.config import CONF

In [10]: CONF.get('shortcuts', 'editor/inspect current object')
Out[10]: u'Ctrl+I'

Unfortunately it gives me the default. I'm not sure of a better way. I do like the idea of the helper functions.

@goanpeca
Copy link
Member

Well config.py has it defined as:

'editor/inspect current object': 'Ctrl+I', so it will always return that,

I do not see any

'editor/inspect current object': 'Cmd+I' if is_mac else 'Ctrl+I'

is this shortcut being altered somewhere else?

@goanpeca
Copy link
Member

ahh now I get it... this might require some tweaking to make it more appropriate I guess.

The spyderlib\plugins\shortcuts.py is where the things get adapted.

@goanpeca
Copy link
Member

So maybe we can import from there?

@blink1073
Copy link
Contributor Author

Right, it may be that if I stick it in the live code, it will grab the right version. I'll give in a shot.

@blink1073
Copy link
Contributor Author

Nope, it did not work...

@goanpeca
Copy link
Member

Well, I think we need a few things...

  1. Document in config.py that shortcuts are arranged depending on the OS on runtime in spyderlib\plugins\shortcuts and include some help on that
                 Qt.ControlModifier: "Ctrl", Qt.AltModifier: "Alt",
                 Qt.MetaModifier: "Meta"}
    if sys.platform == 'darwin':
        MODIFIERNAMES = {Qt.NoModifier: "", Qt.ShiftModifier: "Shift",
                         Qt.ControlModifier: "Cmd", Qt.AltModifier: "Alt",
                         Qt.MetaModifier: "Ctrl"}
    elif sys.platform == 'win32':
        MODIFIERNAMES = {Qt.NoModifier: "", Qt.ShiftModifier: "Shift",
                         Qt.ControlModifier: "Ctrl", Qt.AltModifier: "Alt",
                         Qt.MetaModifier: "Win"}
    else:
        MODIFIERNAMES = {Qt.NoModifier: "", Qt.ShiftModifier: "Shift",
                         Qt.ControlModifier: "Ctrl", Qt.AltModifier: "Alt",
                         Qt.MetaModifier: "Meta"}
  1. We need a helper function that would retrieve the correct shortcut on runtime based on the previous info.

OR

Maybe we should define these transformations directly inside the config.py so instead of using strings directly... we should use maybe a tuple of modifiers plus the key?

So instead of writing:

'editor/last edit location': "Ctrl+Alt+Shift+Left", we would do

'editor/last edit location': (Ctrl, Alt, Shift, Left)?

@goanpeca
Copy link
Member

I guess the helper function would be the easiest to do.

@goanpeca
Copy link
Member

But now that I check... the config.py is not consistent... some of the shortcuts use CTRL variable

# Ctrl key
CTRL = "Meta" if sys.platform == 'darwin' else "Ctrl"
...

'editor/code completion': CTRL+'+Space'`  

This is too messy!

@ccordoba12
Copy link
Member

@goanpeca. it seems you don't understand how Qt works with shortcuts. Ctrl is mapped to Cmd automatically in Mac. So everything that in the other OSes is Ctrl + something else, in Mac works with Cmd. That's something standard and that people working on all platforms accept without problems (maybe @blink1073 is new in Mac? ;-)

I added the line you referred too

# Ctrl key
CTRL = "Meta" if sys.platform == 'darwin' else "Ctrl"

so that code completion in the Editor didn't get mapped to Cmd+space because that shortcut is taken by Spotlight.

Of course, there are things like the message in the Object Inspector that pass overlooked (because that's text), but most of the time things work without problems.

@ccordoba12 ccordoba12 added this to the v3.0 milestone Apr 11, 2015
@@ -749,6 +749,8 @@ def show_intro_message(self):
tutorial = _("tutorial")
intro_message = intro_message % ("<b>Ctrl+I</b>", "<br><br>",
Copy link
Member

Choose a reason for hiding this comment

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

Why not add a block before this one with something like

if sys.platform == 'darwin':
    shortcut = "<b>Cmd+I</b>"
else:
    shortcut = "<b>Ctrl+I</b>"

and then change this line like this

             intro_message = intro_message % (shortcut, "<br><br>", "<i>"+prefs+"</i>")

I think this approach is much cleaner than the proposed one.

@blink1073
Copy link
Contributor Author

@ccordoba12, I agree that the default on OSX is the Cmd key, but the Ctrl key is still used and the text is misleading.

@ccordoba12
Copy link
Member

@goanpeca, there are few places where we check for the operating system type, and in those places we use the standard library (which is the standard way of doing it! :-) I don't understand what's the problem with that, really ;-)

We don't need our own functions for that! It's like saying that we need to define our own functions to open files :-)

The modifier names are changed by Qt internally everywhere (in menus, in tooltips, etc), except when they are text. Please run Spyder on a Mac to see it yourself :-)

@ccordoba12
Copy link
Member

@blink1073, sure, I totally agree with you. I just overlooked that the shortcut was not pointing to Cmd+I on Mac.

What do you think about my suggestion?

@ccordoba12
Copy link
Member

Please move this discussion to your own PR. We are just polluting @blink1073 PR with things that doesn't have to do with his contribution.

@blink1073
Copy link
Contributor Author

How's that look @ccordoba12?

@ccordoba12 ccordoba12 changed the title Fix inspector message for OSX. Fixes #2317 Fix inspector message for OSX Apr 11, 2015
@ccordoba12
Copy link
Member

Much better, thanks for doing the change. Merging :-)

@ccordoba12 ccordoba12 changed the title Fix inspector message for OSX Fix Object inspector message for OSX Apr 11, 2015
ccordoba12 added a commit that referenced this pull request Apr 11, 2015
Fix Object inspector message for OSX
@ccordoba12 ccordoba12 merged commit 80b1c8b into spyder-ide:master Apr 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object Inspector Text on Mac OS X is Misleading
3 participants