Added: keyboard shortcuts for save and save profile#350
Added: keyboard shortcuts for save and save profile#350vadi2 merged 16 commits intoMudlet:developmentfrom
Conversation
Dice mudlet
Added: keyboard shortcuts for save and save profile
Revert "Added: keyboard shortcuts for save and save profile"
Revert "Dice mudlet"
|
|
||
| QAction * saveAction = new QAction( QIcon( QStringLiteral( ":/icons/document-save-as.png" ) ), tr("Save Item"), this); | ||
| //saveAction->setShortcut(tr("Ctrl+S")); | ||
| saveAction->setShortcut(tr("Ctrl+S")); |
There was a problem hiding this comment.
After reviewing the Qt Documentation for the QKeySequence class under the heading Keyboard Layout Issues:
Although some developers might resort to fully specifying all the modifiers they use on their keyboards to activate a shortcut, this will also result in unexpected behavior for users of different keyboard layouts.
For example, a developer using a British keyboard may decide to specify "Ctrl+Shift+=" as the key sequence in order to create a shortcut that coincidentally behaves in the same way asCtrl plus. However, the=key needs to be accessed using theShiftkey on Norwegian keyboard, making the required shortcut effectivelyCtrl Shift Shift =(an impossible key combination).As a result, both human-readable strings and hard-coded key codes can both be problematic to use when specifying a key sequence that can be used on a variety of different keyboard layouts. Only the use of standard shortcuts guarantees that the user will be able to use the shortcuts that the developer intended.
Despite this, we can address this issue by ensuring that human-readable strings are used, making it possible for translations of key sequences to be made for users of different languages. This approach will be successful for users whose keyboards have the most typical layout for the language they are using.
I was going to suggest we use saveAction->setShortcut( QKeySequence::Save ) instead. It will get mapped to Ctrl+S currently for the en-US case (and other languages in the future where that is appropriate) and to national other values as needed.
However, this might make the use further down of Ctrl_Shift+S problematic, as THAT shortcut normally means a "SaveAs" action but it is being used as "SaveToDisk" whereas there is NOT currently a shortcut for "SaveAsToDisk"...
How about:
saveAction->setShortcut(tr("Alt+S")) for this one
and:
profileSaveAction->setShortcut( QKeySequence::Save ) i.e. Ctrl-S by default
saveProfileAsAction->setShortcut( QKeySequence::SaveAs ) i.e. Ctrl-Shift-S by default
for the other ones further down?
Also it might be worth noting that Heiko previously commented out the Ctrl-S binding in 96bb8d2 on 2009-03-14 17:40:17 with the comment "Fix: couldnt bind control+s keys" after re-purposing it from the showSearchAreaAction in 22e0c3b 2009-03-07 13:43:20 - it might be worthwhile establishing what broke back then to see if it is still broken...!
There was a problem hiding this comment.
Well, the problem I guess he would be mentioning would be that having Ctrl+s getting captured by the window as a shortcut prevents you from setting up a new Macro using that key combo. It does not prevent you from running any existing Macros with that key combo however. Not sure how that could be fixed, ultimately(can you disable shortcuts like that while attempting to capture the key-combo someone is setting a macro to work with?).
As far as the shortcut keys go, I guess alt+s would work as well, although it seems a little counter-intuitive to me since I've always thought of ctrl+s as more of saving your immediate work, rather than saving an entire project. I don't think SaveAs seems like something that should be getting enough use to even warrant a shortcut key, although eventually I'd like to see Mudlet set up so that navigating it is very easily done without the use of a mouse at all, which would require some additional changes that I'm pondering.
There was a problem hiding this comment.
I suspect it might be a matter of the applicability of the shortcut i.e. the QShortcut::context property - according to the Qt Documentation:
By default, this property is set to Qt::WindowShortcut.
and it says:
For a
QEvent::Shortcutevent to occur, the shortcut's key sequence must be entered by the user in a context where the shortcut is active. The possible contexts are these:
- Qt::WidgetShortcut = 0: The shortcut is active when its parent widget has focus.
- Qt::WidgetWithChildrenShortcut = 3: The shortcut is active when its parent widget, or any of its children has focus. Children which are top-level widgets, except pop-ups, are not affected by this shortcut context.
- Qt::WindowShortcut = 1: The shortcut is active when its parent widget is a logical subwidget of the active top-level window.
- Qt::ApplicationShortcut = 2: The shortcut is active when one of the applications windows are active.
I wonder whether we need to use the non-default Qt::WidgetShortcut...
|
Where are we at with this - good to be tested? |
|
Completely functional. Only real remaining thing to be done with it is potentially modifying the actual shortcut keys that are being used. I've got it using CTRL+S and CTRL+SHIFT+S currently, was thinking about switching it to QKeySequence::Save and QKeySequence::SaveAs which SHOULD be the same for those using standard qwerty keyboards and layouts, but which would be appropriately replaced with the standard shortcuts for whatever languages/layouts people use. I want to note that adding these shortcuts will NOT prevent CTRL+S and CTRL+SHIFT+S from being used for macros that already have those combos set up, but you won't be able to set macros to those key combos anymore, because there is no code in place to have it disable those shortcuts when you're waiting for the user to input the desired key combo on a macro(after you hit the Grab New Key button). (This should mean that it keeps to y'all's design principle of not breaking existing things?) |
|
That sounds OK to me. Will test tonight.
…On Wed, 7 Dec 2016 11:55 am dicene, ***@***.***> wrote:
Completely functional.
Only real remaining thing to be done with it is potentially modifying the
actual shortcut keys that are being used. I've got it using CTRL+S and
CTRL+SHIFT+S currently, was thinking about switching it to
QKeySequence::Save and QKeySequence::SaveAs which SHOULD be the same for
those using standard qwerty keyboards and layouts, but which would be
appropriately replaced with the standard shortcuts for whatever
languages/layouts people use.
I want to note that adding these shortcuts will NOT prevent CTRL+S and
CTRL+SHIFT+S from being used for macros that already have those combos set
up, but you won't be able to set macros to those key combos anymore,
because there is no code in place to have it disable those shortcuts when
you're waiting for the user to input the desired key combo on a macro(after
you hit the Grab New Key button).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#350 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGxjEwcj0heE9tEihcifHtcjDaz8lvnks5rFhIOgaJpZM4KzQ5f>
.
|
|
Broke some stuff, then I unbroke it. This PR should be clean and good to go if we want to stick with CTRL+S and CTRL+SHIFT+S. Was wanting to go with generic shortcuts, but according to http://doc.qt.io/qt-4.8/qkeysequence.html#standard-shortcuts the generic SaveAs doesn't have a working sequence for Windows? In the future I could try and have it check for QKeySequence defaults first then go with the hard-coded shortcuts as backups, but I'd say it's easiest just to ship it as is. If I'm not mistaken, this won't be the first shortcut hard-coded in anyway(looking at you, Disconnect and Reconnect), so if someone gets around to making them all use QKeySequences, Save and Save Profile can be done at that time. |
…trlSToSaveScripts
|
@SlySven what do you reckon? |
|
Yeah, even the up to date (we're not using Qt4.x anymore) documentation for |
|
So, depending on what everyone thinks would be most future proof, I could add in a way to toggle between using QKeySequence's or hard-coded shortcuts via something in the menu? Might even be able to set up something to let you change the key combos dynamically in the menu, similar to how it works with Macros. This would likely entail a lot of code changes, since I'd have to add in a new key-capturing setup in the preferences window, as well making some changes that could allow it to disconnect existing shortcuts and reconnect them with new key shortcuts when you SAVE the preferences. Also, I guess I could switch out the hard-coded codes with QKeySequences and see if it works with Windows even though it doesn't have a documented combo for SaveAs? You never know, they could have added the shortcut in and just forgotten it from the table. 😕 |
|
OK. Try and see if the key sequence works on Windows now. If it doesn't,
I'll need to wrap my head around what exactly will we be losing.
…On Tue, 13 Dec 2016 5:06 am dicene, ***@***.***> wrote:
So, depending on what everyone thinks would be most future proof, I could
add in a way to toggle between using QKeySequence's or hard-coded shortcuts
via something in the menu? Might even be able to set up something to let
you change the key combos dynamically in the menu, similar to how it works
with Macros. This would likely entail a lot of code changes, since I'd have
to add in a new key-capturing setup in the preferences window, as well
making some changes that could allow it to disconnect existing shortcuts
and reconnect them with new key shortcuts when you SAVE the preferences.
Also, I guess I could switch out the hard-coded codes with QKeySequences
and see if it works with Windows even though it doesn't have a documented
combo for SaveAs? You never know, they could have added the shortcut in and
just forgotten it from the table. 😕
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#350 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGxjNgMa0HzMBc4ywwTFML_STBt-_3vks5rHZszgaJpZM4KzQ5f>
.
|
|
Can confirm that in Win10 QKeySequence::SaveAs isn't working at all. |
…trlSToSaveScripts
|
Played around a little to see if it was at all easy to change a shortcut assigned to an action after the triggerEditor has already been created. Some gaps in my understanding of C++ and in the way scope works are stopping me from making it work. Honestly, this doesn't need to be a profile specific setting in Mudlet. I think it'd be safe as application/installation specific, so if there's somewhere that basic settings are being stored(wherever you guys are storing Mudlet's x/y window position and width/height), you could probably drop it in there and maybe let users customize a few built in shortcuts from there, that way they're also easy to implement, since you'd just read them from variables stored in the Mudlet object instead of dealing with trying to allow it to be dynamically changed after the dlgTriggerEditor is created. There isn't really a reason why you should actually want to support dynamically changing them anyway, so I think a more out-of-the-way approach is reasonable? |
|
Yeah, I'm fine with that solution. As you mentioned before, it looks like if you setup the Ctrl+S keybind before this change, the keybind will still work when you're focused on the main display - and the keybind will still work when you're focused on the editor. So if you have it disable the keybind when you're capturing a new combination, wouldn't we have the best of both worlds? |
…trlSToSaveScripts
Modified the functions that run to allow you to set Macros so that they now temporarily disable the Ctrl+S and Ctrl+Shift+S shortcuts until you cancel with Escape or input a key
|
Welp, dug down into the code a little(for like, 3 hours) and worked out a solution that disables the "Save Item" and "Save Profile" shortcuts when you're waiting to capture a new key(+modifiers) for a macro(includes handling the case for using the escape key to disable the capturing). Seems to be functioning right. Now that I know how to make changes to the shortcuts properly, I think I could probably set up something to let you set them via either a config file(the one that stores x, y, width, height), or from preferences(although this might be a little harder). Want me to put in some time trying to figure that out before pushing this change, or is it safe to ship now with the expectation that I'll get something configurable put together later on? |
|
We can add it later.
…On Fri, 16 Dec 2016 6:49 pm dicene, ***@***.***> wrote:
Welp, dug down into the code a little(for like, 3 hours) and worked out a
solution that disables the "Save Item" and "Save Profile" shortcuts when
you're waiting to capture a new key(+modifiers) for a macro(includes
handling the case for using the escape key to disable the capturing). Seems
to be functioning right.
Now that I know how to make changes to the shortcuts properly, I think I
could probably set up something to let you set them via either a config
file(the one that stores x, y, width, height), or from preferences(although
this might be a little harder). Want me to put in some time trying to
figure that out before pushing this change, or is it safe to ship now with
the expectation that I'll get something configurable put together later on?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#350 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGxjKDDOqurkyjz01BpXNk_K3G7_rQBks5rIlCHgaJpZM4KzQ5f>
.
|
|
Should be good to go then, after you get a chance to test it. 👍 |
|
Merging as @SlySven isn't against it in #350 (comment) |
The following two shortcuts were added to the trigger-editing window
CTRL-S: Saves the current trigger/alias/script that you're working on.
CTRL-SHIFT-S: Saves the profile