These patches will add a link tot he keyboard shortcuts dropdown in the advanced cataloging editor to a page where the mappings can be redefined
Created attachment 79408 [details] [review] Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work
Created attachment 79409 [details] [review] Bug 21411: DO NOT PUSH - Schema changes
Hi Nick, nice enhancement, just few quick comments: 1) you should also provide sql installer data - for new installs, 2) there is missing file tools/adveditorshortcuts.pl 3) I am not sure if it should be part of tools module... 4) in file adveditorshortcuts.tt you are referencing the adveditorshortcuts.pl as reviews/adveditorshortcuts.pl - in breadcrumbs 5) could you please use bootstrap for layout of edit shortcuts page?
Created attachment 85365 [details] [review] Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work
Created attachment 85366 [details] [review] Bug 21411: DO NOT PUSH - Schema changes
Created attachment 85397 [details] [review] Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work
Created attachment 85398 [details] [review] Bug 21411: DO NOT PUSH - Schema changes
Created attachment 86012 [details] [review] Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work
Created attachment 86013 [details] [review] Bug 21411: DO NOT PUSH - Schema changes
Created attachment 86014 [details] [review] Bug 21411: (follow-up) Missing filters and unused variable POD warnings from QA tool are false positives (basic functionsn POD not needed)
Created attachment 86018 [details] [review] Bug 21411 - Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Created attachment 86019 [details] [review] Bug 21411: DO NOT PUSH - Schema changes Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Created attachment 86020 [details] [review] Bug 21411: (follow-up) Missing filters and unused variable POD warnings from QA tool are false positives (basic functionsn POD not needed) Signed-off-by: Liz Rea <liz@catalyst.net.nz>
I've got a bad git email address hiding out somewhere in my git config, so sorry if those look like my old email address at Catalyst. I'm still signing off and if you're inclined you can change them to wizzyrea@gmail.com. I don't want to clutter the bug by fixing it but I'll fix it before I do my next one. Cheers, Liz
Hi Nick, having a first look here: 1) FAIL Koha/KeyboardShortcuts.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'object_class' 2) No tests? 3) Translation issues a) Ctrl is not labelled Ctrl on German keyboards, it's Strg - so people might not understand. Currently I can fix this in translation, but with the new system that will not work. We could add another column for a 'spelled out' mapping or we could add some replacing in the template that can be translated. Not sure here. (Not quite blocker... but meh) b) The new file is missing from the translated installers. (blocker) I'd like to suggest a different approach here. As you can't invent your own keyboard shortcuts, I'd deal with this like with the permissions. Have one SQL file with the mandatory values on top level (not in language directories) and deal with the description translation in the templates. 4) I am wondering if tools is the right place for this. I think I would put it with Sound alerts and column configuration in Administration. 5) If we leave it in tools: It's missing from tools sidebar and start page. If we move to admin, we'll also need sidebar and start page. As the navigation of tools shows on the tool page, I think that makes it currently a bit confusing. 6) Permissions: currently it doesn't require any permission: flagsrequired => {}, Do we want to leave it like this? As the keyboard shortcuts are global and not per user I can imagine libraries would not want anyone be able to reconfigure them. 7) "If combing multiple keys " - interesting idea :) 8) The button is a bit close to the table, could use a little template clean-up.
Created attachment 88465 [details] [review] Bug 21411: Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Created attachment 88466 [details] [review] Bug 21411: (follow-up) Missing filters and unused variable POD warnings from QA tool are false positives (basic functionsn POD not needed) Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Created attachment 88467 [details] [review] Bug 21411: Address QA concerns Move to admin Add a permission Remove descriptions from table Clean up template
Created attachment 88468 [details] [review] Bug 21411: DO NOT PUSH - SCHEMA CHANGES
Something is odd here :( kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove -v t/db_dependent/Koha/KeyboardShortcuts.t t/db_dependent/Koha/KeyboardShortcuts.t .. 1..4 ok 1 - use Koha::KeyboardShortcuts; DBIx::Class::Row::store_column(): No such column 'shortcut_desc' on Koha::Schema::Result::KeyboardShortcut at /home/vagrant/kohaclone/Koha/Object.pm line 75 # Looks like your test exited with 255 just after 1. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 3/4 subtests Test Summary Report ------------------- t/db_dependent/Koha/KeyboardShortcuts.t (Wstat: 65280 Tests: 1 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 4 tests but ran 1. Files=1, Tests=1, 2 wallclock secs ( 0.03 usr 0.01 sys + 1.50 cusr 0.37 csys = 1.91 CPU) Result: FAIL I've run updatedatabase, dbic, restart_all etc. :( Functionality wise this seems ok so far, can you please have a look?
Created attachment 89119 [details] [review] Bug 21411: (QA follow-up) fix tests This patch removes erroneous shortcut_desc from the tests, which was left in accidentally.
Suggested follow-ups: Hide hide the Keyboard settings when Rancor is not activated, similar like other settings are not visible (Plugins, Search engine configs) without the pref.
Some small stuff: 1) As we moved the sql out of hte en installer, this needs to go too: installer/data/mysql/en/mandatory/keyboard_shortcuts.txt 2) Do you really want to give liblime copyright? +-- Default keyboard shortcuts +-- for Koha. +-- +-- Copyright (C) 2007 LiblimeA +-- Copyright 2018 Koha Development Team 3) Needs permission check: +<a id="redefine_shortcuts" href="/cgi-bin/koha/admin/adveditorshortcuts.pl">Redefine shortcuts</a> 4) Hide hide the Keyboard settings when Rancor is not activated, similar like other settings are not visible (Plugins, Search engine configs) without the pref. - Admin menu - Admin home page
Created attachment 89179 [details] [review] Bug 21411: (follow-up) Clean up for QA
Created attachment 89416 [details] [review] Bug 21411: Advanced cataloging editor - rancor - Allow configuration of Keyboard This patch allows for redfining the keyboard shortcuts used in rancor. For now these are set globally (per instance) Ctrl-S is a codemirror setting and cannot be altered To test: 1 - Apply patches 2 - update database 3 - Load the advanced cataloging editor 4 - Click the "Keyboard shortcuts" button 5 - Ensure the list looks correct 6 - Note that 'Ctrl-S' note sit cannot be remapped 7 - Note the 'Redefine shortcuts' link at the top of the menu 8 - Confirm all the listed shortcuts work as expected 9 - Click the 'Redefine' link 10 - Ensure current mappings load correctly 11 - Read explanation and verify it makes sense 12 - Remap some functions 13 - Return to the cataloging editor 14 - Confirm your mappings work Signed-off-by: Liz Rea <liz@catalyst.net.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89417 [details] [review] Bug 21411: (follow-up) Missing filters and unused variable POD warnings from QA tool are false positives (basic functionsn POD not needed) Signed-off-by: Liz Rea <liz@catalyst.net.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89418 [details] [review] Bug 21411: Address QA concerns Move to admin Add a permission Remove descriptions from table Clean up template Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89419 [details] [review] Bug 21411: (QA follow-up) fix tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89420 [details] [review] Bug 21411: (follow-up) Clean up for QA Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89421 [details] [review] Bug 21411: (QA follow-up) Fix permission checks on admin page and menu Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I realized I had missed the follow-up, sorry!
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series.