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

ctrl-backspace shortcut breaks word delete #453

Closed
KayEss opened this issue May 5, 2015 · 18 comments
Closed

ctrl-backspace shortcut breaks word delete #453

KayEss opened this issue May 5, 2015 · 18 comments

Comments

@KayEss
Copy link

KayEss commented May 5, 2015

Normally the ctrl-backspace does a single word delete of the word behind the input cursor. The 'delete untracked files' short cut that uses this key combination breaks this standard editing functionality.

@nocnokneo
Copy link

For me, Ctrl-Backspace still does single-word-delete when the cursor is active in the message summary or description fields. The move-file-to-trash shortcut is only used when a selected file in the Status widget is has active focus. Maybe it's a platform difference? I'm using Linux/GNOME 3.

@KayEss
Copy link
Author

KayEss commented Jul 8, 2015

I'm Linux KDE, plasma 5. It still doesn't work for me. ctrl-del does what I'd expect though.

@davvid
Copy link
Member

davvid commented Jul 11, 2015

On xfce4 (debian/sid) and I observe this same behavior. At first I also thought that the shortcut should only be active when the status widget was active, but I guess I was mistaken when writing the code. The shortcut contexts might need to be tweaked to get the behavior that was originally intended, but that's a higher impact change.

Maybe Qt changed behavior in there, somewhere? A simple fix would be to switch to Alt+Backspace and Alt+Shift+Backspace. What do you all think?

An alternative idea would be a larger refactoring to make all the shortcuts configurable. Despite the hard-codedness of it, which I usually dislike in code, I enjoy the regularity and consistency that in forces upon the UI by not being configurable. Thus, I can't think of a strong reason to make it configurable when we can just tweak it once.

I guess that's a minor version bump at least since we're breaking user's muscle memory (but only once, I hope!). Technically, someone could argue for a major version bump, but I'll save that for the qt5 port.

My commit that tweaks the hotkeys may close this issue, but I'm still curious about the shortcut context stuff in case anyone has any insights.

@davvid davvid closed this as completed in 4ff23a3 Jul 11, 2015
davvid added a commit that referenced this issue Jul 11, 2015
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid
Copy link
Member

davvid commented Jul 11, 2015

We've been using WidgetWithChildrenContext: http://doc.qt.io/qt-4.8/qt.html#ShortcutContext-enum

This thread mentions,

A shortcut should be triggered if it has a valid parent widget and this parent widget receives events (id est it is not hidden). In addition, if the shortcut context is Qt::WindowShortcut (default) the widget has to be active.

We're using dock widgets, so that might explain why the shortcuts propagate through -- they're all part of the same window.

@nocnokneo
Copy link

I'm also totally okay with hard-coded shortcuts -- as long as they follow
de facto standards as much as possible. For this reason I was never a big
fan of the Ctrl+Backspace shortcut. I think it should have been Ctrl+Delete

  • though I'm guessing the rationale was for laptop users who don't usually
    have an easily accessible Delete key. I think Alt+Backspace is a step in
    the wrong direction just to work around a bug that relatively few users
    experience.
    On Jul 11, 2015 6:30 PM, "David Aguilar" notifications@github.com wrote:

On xfce4 (debian/sid) and I observe this same behavior. At first I also
thought that the shortcut should only be active when the status widget was
active, but I guess I was mistaken when writing the code. The shortcut
contexts might need to be tweaked to get the behavior that was originally
intended, but that's a higher impact change.

Maybe Qt changed behavior in there, somewhere? A simple fix would be to
switch to Alt+Backspace and Alt+Shift+Backspace. What do you all think?

An alternative idea would be a larger refactoring to make all the
shortcuts configurable. Despite the hard-codedness of it, which I usually
dislike in code, I enjoy the regularity and consistency that in forces upon
the UI by not being configurable. Thus, I can't think of a strong reason to
make it configurable when we can just tweak it once.

I guess that's a minor version bump at least since we're breaking user's
muscle memory (but only once, I hope!). Technically, someone could argue
for a major version bump, but I'll save that for the qt5 port.

My commit that tweaks the hotkeys may close this issue, but I'm still
curious about the shortcut context stuff in case anyone has any insights.


Reply to this email directly or view it on GitHub
#453 (comment).

@davvid
Copy link
Member

davvid commented Jul 12, 2015

yes, the reason for Backspace over Delete was for Mac OSX (and possibly other) laptops where Delete is not available.

that said, we still have the right click context menu, so I can certainly be persuaded. another possibility is to do a platform check for osx.

on osx, ctrl-backspace doesn't do the delete word thing, so there's no shortcut being clobbered there.

what do you think about this plan?: use ctrl-delete, and use ctrl-backspace on osx only.

we haven't released anything yet, so it's OK for us to change it now during the prerelease time.

cheers

@KayEss
Copy link
Author

KayEss commented Jul 13, 2015

My feeling would be that both ctrl-backspace (delete word to the left) and ctrl-del (delete word to the right) are already so well established that I don't think it's a good idea to use either, at least on non-Mac platforms. I also don't think it'd be a great idea to have the same shortcut do different things based purely on focus.

Some software (e.g. Chrome) uses ctrl-shift-backspace to delete a paragraph, but the shortcut doesn't seem anywhere near as well established -- it doesn't do anything in cola on my platform (Linux/KDE).

So for me, +1 on changing it to ctrl-shift-backspace.

@davvid
Copy link
Member

davvid commented Jul 14, 2015

I can agree with that. Part of the problem was that I was too eager to give these shortcut keys. Another part of the problem is that there are actually two sets of shortcuts.. if the user has SendToTrash installed then we provide shortcuts for both "Delete" and "Send To Trash".

Thinking it through, one shortcut is sufficient, and I agree that we should do ctrl-shift-backspace. Thanks for the clarity.

davvid added a commit that referenced this issue Jul 14, 2015
We were previously assigning "Ctrl+Backspace" to "Delete".  This was
later adjusted to use "Alt" instead of "Ctrl", but that was a step
backwards.

Originally, when `send2trash` was installed, we would assign the
"Ctrl+Backspace" hotkey to "MoveToTrash", and "Ctrl+Shift+Backspace" to
"Delete".

This is problematic for several reasons.  First off, we should not
clobber the prevalent "Ctrl+Backspace" hotkey, as it is used to remove
the previous word in text edits.

Secondly, we don't need a shortcut for *both* "MoveToTrash" and "Delete"
when the `send2trash` module is installed.  Using a single shortcut is
sufficient.

Adjust the logic so that "Ctrl+Shift+Backspace" is used as the hotkey.
When `send2trash` is installed, only the "MoveToTrash" command will be
bound, otherwise the "Delete" command is bound to the hotkey.

Closes #453
Helped-by: Kirit Sælensminde <k@kirit.com>
Helped-by: Taylor Braun-Jones <taylor@braun-jones.org>
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid
Copy link
Member

davvid commented Jul 14, 2015

Let me know what y'all think. I adjusted it so that we only ever assign a single Shift+Ctrl+backspace hotkey. When send2trash is installed the hotkey is assigned to MoveToTrash, otherwise the hotkey is bound to Delete.

This seems like the nicest solution since there's no need for platform-specific checks, and it keeps things simple for laptop users. Thanks for your sugs, much appreciated.

@davvid
Copy link
Member

davvid commented Jul 16, 2015

Sorry to beat this horse to death. Apparently we can have our cake and eat it to. I just realized that the clobbering behavior was because I was adding the QAction at too broad a scope (the top-level widget) and that was the true source of our woes.

I'm actually going to go back to the original shortcut since there's no downside anymore, and it'll be well behaved on all platforms. 😊

davvid added a commit that referenced this issue Jul 16, 2015
Remove the redundant adding of actions to the main widget.  This is
needed to avoid ambiguous shortcut overloads with the Status widget.

This real benefit of this is that this avoids clobbering the
Ctrl+Backspace hotkey, which frees us to use any hotkeys we want in any
other widget.

This is especially important because the diff editor really should have
vim hotkeys.  Fix it once and for all.

Closes #453
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit that referenced this issue Jul 16, 2015
The *real* reason we were clobbering the Ctrl+Backspace editing hotkey
is that we were adding the action at too broad a scope.  That was fixed
in $SHA1, so now there's no reason we can't continue to use these
hotkeys.

Really-finally-closes: #453
Signed-off-by: David Aguilar <davvid@gmail.com>
@nocnokneo
Copy link

Excellent!
On Jul 15, 2015 11:58 PM, "David Aguilar" notifications@github.com wrote:

Sorry to beat this horse to death. Apparently we can have our cake and eat
it to. I just realized that the clobbering behavior was because I was
adding the QAction at too broad a scope (the top-level widget) and that was
the true source of our woes.

I'm actually going to go back to the original shortcut since there's no
downside anymore, and it'll be well behaved on all platforms. [image:
😊]


Reply to this email directly or view it on GitHub
#453 (comment).

@nocnokneo
Copy link

Okay, I'm going to beat the dead horse a little more...

I've been using change 387e2c8 for the last week and I really miss the old behavior. I now, for example, have to make sure that the status widget has active focus for the Ctrl+s shortcut to work. It's super annoying.

I vote for just going back to the idea of a single Ctrl+Shift+Backspace shortcut for move-file-to-trash and no shortcut for delete-file.

@davvid
Copy link
Member

davvid commented Jul 24, 2015

let's totally resurrect the handling for Ctrl+s. thanks for bringing that up. I'll add this in shortly, and in a nicer way than the original code.

davvid added a commit that referenced this issue Jul 24, 2015
Related-to: #453
Suggested-by: Taylor Braun-Jones <taylor@braun-jones.org>
Signed-off-by: David Aguilar <davvid@gmail.com>
@nocnokneo
Copy link

Thanks - 6c986cc fixed the use case where I hit Ctrl+S from the diff editor. Can we do the same for when the Commit widget has focus? The use case for me is I start typing a commit message for a file that is selected but not yet staged. Once I'm done typing, I just hit Ctrl+S to stage all changes in the file, then Ctrl+Enter. Maybe hit Ctrl+S more than once if the commit includes changes to more than one file that all happen to be listed consecutively in the Status widget.

@nocnokneo
Copy link

Actually, same thing for the Ctrl+E and Ctrl+D shortcuts when the Commit editor has focus. As long as they don't clash with any standard text editing shortcuts on Linux/Windows/Mac (I don't think so)

And the Ctrl+U shortcut from both the Diff widget and the Commit widget.

I don't mean to nit picky, it's just that I find myself using my mouse a lot more than I used to before these changes.

davvid added a commit that referenced this issue Aug 5, 2015
This will allow having common hotkeys in the commit message editor.

Related-to: #453

Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit that referenced this issue Aug 5, 2015
Make things more keyboard-friendly by adding "Launch Editor",
"Launch Difftool", "Stage / Unstage", and "Move Up / Down" hotkeys
to the commit message editor.

Related-to: #453
Suggested-by: Taylor Braun-Jones <taylor@braun-jones.org>
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid
Copy link
Member

davvid commented Aug 9, 2015

I also made it so that Ctrl+U does Revert inside the diff editor, but it does the diff-specific one where it honors the selection, or uses the current hunk. That was previously only accessible via right-click.

Things should be much more keyboard-friendly now. I'm always into making it more keyboard-focused.

@nocnokneo
Copy link

Awesome. Thank you!
On Aug 9, 2015 6:11 AM, "David Aguilar" notifications@github.com wrote:

I also made it so that Ctrl+U does Revert inside the diff editor, but it
does the diff-specific one where it honors the selection, or uses the
current hunk. That was previously only accessible via right-click.

Things should be much more keyboard-friendly now. I'm always into making
it more keyboard-focused.


Reply to this email directly or view it on GitHub
#453 (comment).

@KayEss
Copy link
Author

KayEss commented Aug 10, 2015

Could I ask for another short cut whilst you're looking at these things? I've been using the file browser as a to open and edi files. Could CTRL-R be made to refresh the file list? Right now I have to close and re-open the pane to see newly added files.

davvid referenced this issue Aug 17, 2015
Add the refresh hotkey to the file browser.

Suggested-in: #477
Suggested-by: Kirit Sælensminde <k@kirit.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
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

No branches or pull requests

3 participants