customizing multiple keys shortcut doesn't work

[Simon.pasquet]

Hi!
My name Simon, I'm new here. I'm an architecture student interrested in coding and I would like to help.
English is not my mother thong, feel free to correct myself on gramar and spelling I'm here to learn.

I would like to report a bug and may be work on it.
When you try to customize shortcuts with several keys in it, the second key erase the first.
It might be a bug because, wall in Arch's workbench has the "wa" shortcut.

I have looked in the code, and the function responsible of this is:
AccelLineEdit::keyPressEvent() in ./src/Gui/Widgets.cpp

Presently, I have fixed some aspect of the bug but it's not perfect.
I can customize the shortcut I want, adding "," before the key, but there isn't any validation on the string passed. For exemple the number of keys that can be used. (4 for Qtshortcut)
I have never used Qt and my nivel in c++ is quit low, I use to code in python.
For exemple, line 390-391, I dont understand why there are parenthesis around QString, nor what is ks(). is ks() a function?:

Code: Select all

QKeySequence ks(Qt::SHIFT+key);
txt += (QString)(ks)

I began to work on this bug to learn. So I report it if someone with better ability want to fix it, otherwise I can sumit what I've done when finished but it might be dirty.

the bug on Mantis
http://www.freecadweb.org/tracker/view.php?id=1694

OS: Ubuntu 14.04.1 LTS
Word size: 64-bit
Version: 0.15.3881 (Git)
Branch: master
Hash: 7665e34a3a844dc0e21aa86375ff9de1e632ec36
Python version: 2.7.6
Qt version: 4.8.6
Coin version: 4.0.0a
SoQt version: 1.6.0a
OCC version: 6.7.0

[wmayer]

Hi Simon,

thanks for looking into this issue!

For exemple, line 390-391, I dont understand why there are parenthesis around QString, nor what is ks(). is ks() a function?:

C++ has many stuff other languages do not have and thus it's sometimes hard to understand the code for non-C++ coders.

Code: Select all

QKeySequence ks(Qt::SHIFT+key);

So, in this case "ks" is an instance of "QKeySequence".

Code: Select all

txt += (QString)(ks)

In C++ you can implement special operator functions in a class interface which are used to do an automatic conversion. So, the relevant part of the class QKeySequence looks like this:

Code: Select all

class Q_GUI_EXPORT QKeySequence
{
public:
...
    operator QString() const;  // <<=== this is the above function
    operator QVariant() const;
    operator int() const;
...
};

So, this means you can create a QString representation of the QKeySequence. Actually it suffices to write:

Code: Select all

txt += (QString)ks; 

i.e. the parenthesis around "ks" are not needed. As you can see from the class interface you can also convert "ks" into an "int" or "QVariant" representation.

[yorik]

I think one good way to solve this would be:
- you press one key: it works like now
- you press a second key: the shortcut becomes a two-keys shortcut (add a comma)
- you press a third key: it switch back to one-key shortcut.

I don't think it's possible to have two-key shortcuts with modifier (Ctrl, shift), so one needs to decide how to treat that (only apply the modifier if we have only one key?)

[Simon.pasquet]

Thank you for your responses.

wmayer:
I understand the operator stuff. It's seem like an operator overloading but instead of overloading you create a new one. Although I don't understand the advantage over

Code: Select all

txt += ks.toQString()

. May be it's just syntactic sugar.

yorik:
Qt accept two keys (exactly four) with modifier. for example "Ctrl+D, Ctrl+G" work for me ("Ctrl+Q, Ctrl+Alt+A, Ctrl+Shift+S, Z" as well).

- you press one key: it works like now
- you press a second key: the shortcut becomes a two-keys shortcut (add a comma)

It's already done.

- you press a third key: it switch back to one-key shortcut.

Now, to clear the shortcut, I press Backspace.
What I can do is : set the max length to two-keys and when you reach the max length, the shortcut switch back to one-key.
Like this, if one day we want three or four-keys shortcut, we will need only to change a constant.
I see three way to accomplish that :
- analyzing the text, counting the number of "," ;
- set a Class Variable that actually count the number of key that have been Pressed ;
- find if Qt offer a standard way to do that.

I find the same function (AccelLineEdit::AccelLineEdit) in: ./src/Tools/plugins/widget/customwidgets.cpp. what should i do with it? It doesn't seem to change anything when I modify it.

[wmayer]

I understand the operator stuff. It's seem like an operator overloading but instead of overloading you create a new one. Although I don't understand the advantage over

This is very old code, I think roughly 10 years now. I assume at this time the only way was to use the QString operator to get the short cut as string. Now when checking the docs it says that the QString operator is marked as obsolete and in new code it should be "toString(QKeySequence::NativeText)".

I find the same function (AccelLineEdit::AccelLineEdit) in: ./src/Tools/plugins/widget/customwidgets.cpp. what should i do with it? It doesn't seem to change anything when I modify it.

All widgets there are only dummy widgets. Their only purpose is to provide a graphical representation in order to be used inside QtDesigner.

[DevJohan]

Code: Select all

txt += (QString)ks;

This is really a C-style cast and IMO (not just my opinion) should very rarely if ever be used in C++code. In this case if there are ambiguities I would use

Code: Select all

txt += static_cast<QString>(ks);

or if this is dependent on a deprecated operator

Code: Select all

txt += ks.toQString();

. The reason C-style casts should be avoided is they cast just about anything without requiring checks if the cast is sensible. If the cast is sensible or well defined C-style casts do what you expect, if not you're down to the bare metal.

[yorik]

What I can do is : set the max length to two-keys and when you reach the max length, the shortcut switch back to one-key.
Like this, if one day we want three or four-keys shortcut, we will need only to change a constant.

Perfect! Or let the possibility to set 4 letters already, why not after all! (Not sure a 4-letter shortcut can still be called a shortcut )

[Simon.pasquet]

Hi!

The first bug is fixed but on the other side other bugs appears:
1) I have added the meta(windows/super) modifier. Maybe i should not have because it collapse with Gnome. For example I can't enter the shortcut "Meta+s" because it is intercepted first by Gnome;

2) when you press shift+alt in this order (or ctrl+shift+alt, shift+ctrl+alt still in this order) the "ៀ" string is printed.
but when I added the Meta modifier, the bug disappeared ( and it reappear if I disable the Meta modifier. );

3) when you press Altgr, "ៀ" is also printed. I think that's because this key is not a Qt::KeyboardModifier but a Qt::Key.
However this key doesn't seem to have a string representation like the other Qt::Key:
In this post (in french):
http://www.developpez.net/forums/d11619 ... trl-shift/
They propose to look in the definition of keyname[] in qkeysequence.cpp. Indeed AltGr is not defined there. They also suggest to intercept manualy the key. I don't feel able to do that .

To finish with AltGr, in the documentation of Qt::nameSpace we can read.

On Windows, when the KeyDown event for this key is sent, the Ctrl+Alt modifiers are also set.

unfortunately, the best way to fix that might be to forbid the use of this very special key.

2) et 3) are present in the official development build too. Should I create a new bug on Mantis?
and 3) might be a bug of Qt.



To conclude, perhaps it is time to share my work, because i don't know how many time it would take to fix the bug 2) and 3).

Nota: I don't have change yet the "(QString)ks;" to "Something.toString(QKeySequence::NativeText)"

[Simon.pasquet]

I have post a patch on mantis.
I used:
$git format-patch master --stdout > multipleKeysShortcuts.patch
I don't know if it's the right way to do it, nor the right place to post it.

the 2) 3) bug of my previous post are fixed and ks.toString() is used instead of (QString)(ks)

http://www.freecadweb.org/tracker/view.php?id=1694

[saso]

see viewforum.php?f=17