Bug 180022

Summary: [PATCH] Add accelerator to "save image as..." action popup
Product: [Applications] konqueror Reporter: FiNeX <finex>
Component: khtmlAssignee: Konqueror Developers <konq-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: faure
Priority: NOR    
Version: SVN   
Target Milestone: ---   
Platform: Unlisted Binaries   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: accelerator patch

Description FiNeX 2009-01-08 13:59:05 UTC
Created attachment 30026 [details]
accelerator patch

Can I apply this simple patch to add an accelerator for "save image as..." action on right click on images in webpages?
Comment 1 David Faure 2009-01-08 14:10:17 UTC
I think we should rather use KAcceleratorManager to ensure all actions have an accel. I'll give it a try.
Comment 2 David Faure 2009-01-08 14:13:06 UTC
SVN commit 907616 by dfaure:

Assign accelerators to all menu entries automatically.
BUG: 180022
Please test for possible side effects before I backport :)


 M  +3 -0      konq_popupmenu.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=907616
Comment 3 FiNeX 2009-01-08 14:45:14 UTC
Your fix seems works right: it adds the accelerator on all entries and there are no conflicts. I've tried most of the actions using accelerators and them all works. Which other tests could I do for being sure it is does not introduce side effects?
Comment 4 David Faure 2009-01-08 14:55:25 UTC
I was thinking of whether it changed a previously hardcoded accelerator, so people who got into the habit of using it would be disoriented by this change.
But AFAIK this shouldn't happen, kacceleratormanager is supposed to honour hardcoded accels before assigning automatic ones.
Comment 5 FiNeX 2009-01-08 22:00:06 UTC
Before your patch, there were some conficts, for example "c" was used for both "copy text" and "copy link address". Now, the accelerator for "copy link address" has become "l" (select a link and right click on it) because of the new code. This behaviour looks good to me: I personaly prefer to learn a new accelerator rather than having the same for more actions.

:-)
Comment 6 David Faure 2009-01-08 22:06:15 UTC
That's right. OK, thanks for the testing - patch backported.