Review: Disapprove I'd like you to reimplement this functionality from the current trunk, from scratch. This proposal is not acceptable code-wise:
* There are random leftover characters which as far as I can tell don't belong (base.xml line 229; views.js lines 135, 138 and 139) (do you not check your diffs before committing?) * A number of implicit globals, many of them not even incidental but used to store state * The `start` method of openerp.base.shortcut and openerp.base.shortcut_add do not respect the contract for `start` * These classes are named incorrectly * And I'd like you to explain why shortcut inherits from Menu and why shortcut_add inherits from from shortcut, as far as I can tell they have different methods, different behaviors and they don't call into their superclasses at all * Also why shortcuts are not part of the Header object, especially when a hook for shortcuts was added to the Header template * Direct `self.rpc` calls are frowned upon, especially if it's to RPC on Dataset URLs, that's what the dataset classes are for * Incorrect use of jquery methods, and searching multiple times (in a row) for the exact same selector, not to mention unscoped finds where all finding should be scoped to the current widget's root element unless otherwise needed/specified * Useless initializations (e.g. `unlink_id` search.js:379 is initialized to 0 and immediately reset to something else) * Useless temporary variables, which don't add any clarity to the code (since their naming is crummy) e.g. `value` search.js:364-367 * *Unused* local variables (`self`, search.js:362) * Template XML file is mis-indented (base.xml:172) * And the template is non-optimal: `t[@t-name=Shortcuts]` has a single child `ul` which could very well be the template root. `t[@t-foreach=shortcuts_pass]` likewise has a single `li` child which could very well bear `@t-foreach` and `@t-as`. Furthermore it has a `class` attribute which is, as far as I know, useless as it's going to be ignored. * Why add a table for the view title printout, why the additional td in the ViewManager template, what's the point of the `shortcut_image` template -- https://code.launchpad.net/~openerp-dev/openerp-web/shortcut-vme/+merge/69270 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openerp-web/shortcut-vme. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
