Reassign Alt+S to Scan
Needs ReviewPublic

Authored by fakefred on Apr 10 2020, 5:38 AM.

Details

Reviewers
ngraham
sars
Group Reviewers
KDE Applications
Summary

BUG 375938

Diff Detail

Repository
R483 Skanlite
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25007
Build 25025: arc lint + arc unit
fakefred requested review of this revision.Apr 10 2020, 5:38 AM
fakefred created this revision.

Some janitor work from a minor annoyance tracing back to 2017.

sars added a comment.Apr 12 2020, 9:23 AM

I'm not a big fan of the assigning of shortcuts with &, because it becomes a big hassle to keep track of what has been set where and translations makes things even more complicated...

I would prefer something like this for the settings button:
(void)KStandardAction::Preferences(this, SLOT(showSettingsDialog()), actionCollection());

and adding "Real" shortcuts to the buttons...

and then also add a tool-tip to the buttons

aacid added a subscriber: aacid.Apr 12 2020, 10:04 AM
In D28717#646355, @sars wrote:

I'm not a big fan of the assigning of shortcuts with &, because it becomes a big hassle to keep track of what has been set where and translations makes things even more complicated...

There's nothing complicated about it, KXMLGUI will just make sure you don't get conflicts (if possible), so you don't really need to keep track of anything. (unless you're not using kxmlgui in here)

sars added a comment.Apr 13 2020, 8:56 AM

I would like to eventually have the "ctrl-..." along sides the auto generated shortcuts. This is basically a workaround to gett a shortcut (hopefully "Alt-s") for the scan button in stead of no shortcut at the moment.

We could also add the '&' in libksane directly...

I have obtained the source of libksane and located the strings. Should I try binding Ctrl+C to the button? If I did, how do I include the fresh build of libksane in (also freshly built) skanlite?

@sars As you said, I assigned ctrl+p and ctrl+s to preview and scan respectively in libksane along with tooltips (i18n will need to catch up with this, I think). I used QShortcut; poke me if there's a better way. The snippet looks like this:

// ...
d->m_prevBtn->setToolTip(i18n("Scan Preview Image (Ctrl+P)"));
connect(d->m_prevBtn,   SIGNAL(clicked()), d, SLOT(startPreviewScan()));
// ...
d->m_scanBtn->setToolTip(i18n("Scan Final Image (Ctrl+S)"));
connect(d->m_scanBtn,   SIGNAL(clicked()), d, SLOT(startFinalScan()));
// ...

QShortcut *prevShortcut = new QShortcut(QKeySequence(QStringLiteral("Ctrl+P")), parent);
connect(prevShortcut,   SIGNAL(activated()), d, SLOT(startPreviewScan()));

QShortcut *scanShortcut = new QShortcut(QKeySequence(QStringLiteral("Ctrl+S")), parent);
connect(scanShortcut,   SIGNAL(activated()), d, SLOT(startFinalScan()));

I'll make a patch for libksane if approved.

Also, mind reviewing and landing D28672?

sars added a comment.Apr 26 2020, 12:09 PM

Yes please add that to libksane :)

I would set the shortcut directly to the button with setShortcut(...)

then the tool-tip could be added with something like:

button->setTooTip(i18n("foo %1", button->shortcut().toString());

Then you specify the shortcut only once and there is no risk that they get out of sync

I would set the shortcut directly to the button with setShortcut(...)

I don't think it's working. Here's my code:

d->m_prevBtn->setShortcut(QKeySequence(QStringLiteral("Ctrl+P")));
d->m_prevBtn->setToolTip(i18n("Scan Preview Image (%1)", d->m_prevBtn->shortcut().toString()));
d->m_scanBtn->setShortcut(QKeySequence(QStringLiteral("Ctrl+S")));
d->m_scanBtn->setToolTip(i18n("Scan Final Image (%1)", d->m_scanBtn->shortcut().toString()));

Then, I compiled it with kdesrc-build and tested it in skanlite: LD_LIBRARY_PATH=/home/fakefred/kde/usr/lib/libKF5Sane.so.5.55.0 ./skanlite -d test

The tooltips give me the right key sequences, but the shortcuts do not invoke either of them. Is there a problem in my code?

sars added a comment.Apr 27 2020, 6:01 AM

That should work, but I can confirm that it does not work in libksane at the moment.

I tried to do the same with a test application and there it works....

We/(I?) need to investigate what eats up the key sequence...

sars added a comment.May 3 2020, 7:37 PM

This seems to be the reason...
https://mail.kde.org/pipermail/plasma-devel/2016-May/052732.html

TLDR: KAcceleratorManager modifies the button shortcut...

sars added a comment.May 3 2020, 7:46 PM

It seems it could be quite hard to get this to work with only the QPushButton, so your idea with a separate QShortcut is a good workaround.

The only change from your original proposal would be to use myShortcut->key().toString() to get the tooltip :)

Thank you for your improvements so far :)

Thanks :D see you at libksane!