Details

Reviewers

hindenburg

Group Reviewers

Konsole

Commits

R319:581df2a23a9c: Improve the Keyboard tab and the KeyBindings editor

Summary

Change updateKeyBindingsList() to select the last edited keybindings scheme after closing the editor dialog
Add a tooltip showing the path of each keybinding scheme, this should lessen the confusion when the scheme description and the file name of the .keytab file are different
Add a button to the Keyboard tab to reset a scheme to its default values, this is only applicable for schemes that exist in both system-wide and user specific locations
Move a lot of the construction of the keybinding editor widgets from EditProfileDialog to KeyBindingEditor
Change the wording from "key bindings list" to "key bindings scheme" to differentiate between a keyboard translator "scheme" and the key bindings "list" on the Keyboard tab

KeyBindingEditor:

Make the KeyBindingEditor a QDialog and set Qt::WA_DeleteOnClose
Disallow saving a scheme with an empty description
Add a QLineEdit to show only the rows that have strings matching the text entered in the line edit box
Reimplement sizeHint() to make the dialog slightly smaller than the Edit Profile dialog, for better visibility of the entries

KeyboardTranslatorManager:

Make findTranslatorPath() const and public
Make allTranslators() return type const

Test Plan

Editing a key bindings scheme works and closing the dialog selects the edited scheme
Creating a new scheme works as before
"Defaults" button works for system-wide schemes if they were previously edited and saved under the user's home dir
Remove button is only enabled for schemes saved in a dir writable for the user

Diff Detail

Repository

R319 Konsole

Branch

keybindings-editor (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

ahmadsamir created this revision.May 19 2018, 12:45 PM

Restricted Application added a project: Konsole. · View Herald TranscriptMay 19 2018, 12:45 PM

Restricted Application added a subscriber: konsole-devel. · View Herald Transcript

ahmadsamir requested review of this revision.May 19 2018, 12:45 PM

Thanks for working on this.

You can remove the description which shouldn't be allowed - just like profile name can't be empty - I would not use a special kmessagewidget - just use something like setPlaceholderText(i18n("A name must be entered!")); and do the normal popup about missing description when Apply/OK
The 'remove' button is for any file that the user has permissions to delete (for example on one of my system I install konsole into /usr/local)

In D12981#266191, @hindenburg wrote:

[..]

You can remove the description which shouldn't be allowed - just like profile name can't be empty - I would not use a special kmessagewidget - just use something like setPlaceholderText(i18n("A name must be entered!")); and do the normal popup about missing description when Apply/OK

Done, I'll update the diff shortly.

The 'remove' button is for any file that the user has permissions to delete (for example on one of my system I install konsole into /usr/local)

The wording in the test plan isn't clear TBH; I meant the remove button is only enabled for schemes whose parent dir is writeable for the user.

Disallow saving a scheme with an empty description

Make it clearer in the Test Plan that the remove button is only enabled for schemes saved in a dir where the user has write permission

ahmadsamir added a subscriber: hindenburg.May 22 2018, 12:31 PM

doesn't build here

KeyBindingEditor.cpp:307:9: error: use of undeclared identifier 'KMessageBox'

KMessageBox::sorry(this, i18n("A key bindings scheme cannot be saved with an empty description."));

Add proper include

In D12981#266406, @hindenburg wrote:
doesn't build here

KeyBindingEditor.cpp:307:9: error: use of undeclared identifier 'KMessageBox'
KMessageBox::sorry(this, i18n("A key bindings scheme cannot be saved with an empty description."));

Sorry about that, I didn't properly sync the changes between my local build and my konsole git clone.

I notice you can overwrite files w/o any notice - create multiple new key bindings files and apply/ok. You don't need to work on that just bringing it up.

src/KeyBindingEditor.cpp
66	I used this for the profile name entry - think it is OK here as well? setPlaceholderText(i18nc("@label:textbox", "Enter descriptive label"));

Use the same placeholderText from the Edit Profile dialog General tab, more consistent wording across the application and saves the translators a bit of time

In D12981#266447, @hindenburg wrote:

I notice you can overwrite files w/o any notice - create multiple new key bindings files and apply/ok. You don't need to work on that just bringing it up.

TBH, I did have code to check the name of a scheme is valid before saving (that was the main reason for reimplementing accept()), but there were multiple issues:

A scheme can have a different file name and description
When saving a new scheme I checked that the file name and description didn't match any existing ones, respectively
I added quirks to accommodate the read-only schemes in /usr/share/konsole

Long story short, after I had to add more than 5-6 conditions just to achieve that (and still some stuff would make it barf), I just dropped that part of the patch. It seems to have worked as-is for so many years, probably because most users don't dabble with the keyboard translators much, and the ones that do are savvy enough not to muck it up. :)

KeyBindingEditor.cpp:66:97: error: expected ';' after expression

_ui->descriptionEdit->setPlaceholderText(i18nc("@label:textbox", "Enter descriptive label"))

hindenburg edited the test plan for this revision. (Show Details)May 22 2018, 3:14 PM

In D12981#266504, @hindenburg wrote:
KeyBindingEditor.cpp:66:97: error: expected ';' after expression
_ui->descriptionEdit->setPlaceholderText(i18nc("@label:textbox", "Enter descriptive label"))

Shameful, will fix.

Add missing semicolon

hindenburg accepted this revision.May 22 2018, 10:47 PM

This revision is now accepted and ready to land.May 22 2018, 10:47 PM

Closed by commit R319:581df2a23a9c: Improve the Keyboard tab and the KeyBindings editor (authored by ahmadsamir, committed by hindenburg). · Explain WhyMay 22 2018, 11:12 PM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			src/EditProfileDialog.h (12 lines)
M			src/EditProfileDialog.cpp (110 lines)
M			src/EditProfileDialog.ui (16 lines)
M			src/KeyBindingEditor.h (56 lines)
M			src/KeyBindingEditor.cpp (117 lines)
M			src/KeyBindingEditor.ui (16 lines)
M			src/KeyboardTranslatorManager.h (29 lines)
M			src/KeyboardTranslatorManager.cpp (17 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	34474	5128781		May 19 2018, 12:45 PM	★	★
Diff 2	34639	674de97	Disallow saving a scheme with an empty description	May 22 2018, 12:30 PM	★	★
Diff 3	34643	674de97	Add proper include	May 22 2018, 1:12 PM	★	★
Diff 4	34648	674de97	Use the same placeholderText from the Edit Profile dialog General tab, more…	May 22 2018, 2:14 PM	★	★
Diff 5	34657	674de97	Add missing semicolon	May 22 2018, 4:34 PM	★	★
Diff 6	34674	c4baeb5	R319:581df2a23a9c736b23b0b6d1782c19eadbcbafe4	May 22 2018, 11:11 PM	★	★

Improve the Keyboard tab and the KeyBindings editor
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 34657

src/EditProfileDialog.h

src/EditProfileDialog.cpp

src/EditProfileDialog.ui

src/KeyBindingEditor.h

src/KeyBindingEditor.cpp

src/KeyBindingEditor.ui

src/KeyboardTranslatorManager.h

src/KeyboardTranslatorManager.cpp

Improve the Keyboard tab and the KeyBindings editorClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 34657

src/EditProfileDialog.h

src/EditProfileDialog.cpp

src/EditProfileDialog.ui

src/KeyBindingEditor.h

src/KeyBindingEditor.cpp

src/KeyBindingEditor.ui

src/KeyboardTranslatorManager.h

src/KeyboardTranslatorManager.cpp

Improve the Keyboard tab and the KeyBindings editor
ClosedPublic
Actions

Revision Contents
Changeset List