Review: Disapprove * What's the point of the name attribute? Seems used once for rendering, in which case why isn't it called `template` as it is everywhere else, and why is there a name of the (never rendered) Client? Also why is the name/template of the embedded client not "embedded client"?
* The rendering code in the session_bind callback (in #start) is idiotic: - why initialize $e to undefined when it's the value of a declared-but-uninitialized variable anyway? - why put the result of $(QWeb.render) in a variable, and *then* test if that variable exist and append it? How about just appending it directly? - what's with the fucked up try/catch, why is that even there? * The old EmbeddedClient added the `openerp` class to its root element, the new one seems not to, what gives? * Related, why does WebClient do that (and more) independently? Also both should have fixme notes: if/web we get declarative DOM root, these should be used instead of procedural setting * Does all the stuff left in Client#show_common really make sense for the embedded client? It suddenly gains a loading thingie, a notification system and a crash manager (missing half its usefulness)… ? * Why has show_application remained into Client even though none of its content makes sense for the embedded client (and it's not called from the embedded client at all)? And of course what sense does it make for bind_events to be shared when 2 out of 3 events in it are related to UI elements which make no sense in the embedded client? The idea seems good, but I don't think implementing it by moving the WebClient out of itself works, it just leads to a half-assed and schizophrenic superclass. It should be re-done by extracting the common bits out (of WebClient, mostly) with everything remaining in its respective client until and unless it makes sense to move it up the hierarchy. And the mobile Client class should be involved in that scheme, why does everybody hate him? -- https://code.launchpad.net/~openerp-dev/openerp-web/trunk-clients-chs/+merge/115382 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openerp-web/trunk-clients-chs. _______________________________________________ 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
