Odoo integration #134

Goaman · 2020-03-18T13:27:18Z

Corresponding Odoo branch: master-implement-jabberwock-nby

Original odoo branch commits for comment safeguarding purposes:
odoo-dev/odoo@ba26712
odoo-dev/odoo@05fefe2

Zynton

I do realize this is a WIP so don't take my comments as criticism, I just wrote whenever I did find things that raised questions.

Zynton · 2020-05-18T09:19:10Z

doc/odoo_integration_workflow.md

+```bash
+npm run build
+npm run build-odoo
+cp build/webpack/build/build-full-odoo.js <your_odoo_path>/addons/web_editor/static/lib/jabberwock/jabberwock.js


Why cp instead of mv?

Zynton · 2020-05-18T09:19:41Z

doc/odoo_integration_workflow.md

+```
+`odoo-integration.js` will load the script `build-full.js`.
+The default loaded script is `http://localhost:8080/build-full.js`.
+You might want to change the port if your development port is not "8080".


doc/odoo_integration_workflow.md

Zynton · 2020-05-18T09:22:48Z

examples/utils/jabberwocky.xml

Why did you do this in jabberwocky.xml instead of creating a new file?
Is this template basically a copy of some template in Odoo? If so which one? Would be nice to keep its name.
I'm not looking at the template in details as I'm assuming it's just a duplicate from Odoo. If you tell me it's not then I'll look at it more closely.

Zynton · 2020-05-18T13:04:31Z

packages/plugin-odoo-snippets/src/OdooSnippet.ts

+        const selectedNode = params.context.range.selectedNodes().map(node => node.clone());
+        const nodeWithFormat: InlineNode = selectedNode.find(
+            node =>
+                node instanceof InlineNode &&


Never do instanceof for vNodes, we have is for that...

To be fair, we discussed with chm a few days ago and he told me that using instanceof would be orders of magnitude faster than using .is once the memory is merged. I'm not feeling confident about the usefulness of .is apart from being shorter to write so it's possible it might get the axe at some point.

Zynton · 2020-05-18T13:06:03Z

packages/plugin-odoo-snippets/src/OdooSnippet.ts

+    async getSelectedLink(params: CommandParams): Promise<string> {
+        const selectedNode = params.context.range.selectedNodes().map(node => node.clone());
+        const nodeWithFormat: InlineNode = selectedNode.find(
+            node =>
+                node instanceof InlineNode &&
+                node.formats.find(format => format instanceof LinkFormat),
+        ) as InlineNode;
+        const linkFormat: LinkFormat =
+            nodeWithFormat &&
+            (nodeWithFormat.formats.find(format => format instanceof LinkFormat) as LinkFormat);
+        return linkFormat ? linkFormat.url : '';
+    }


All this code is irrelevant now that modifiers were merged. Also there is already a function doing exactly this in Link.

Zynton · 2020-05-18T13:10:04Z

packages/plugin-odoo-snippets/src/OdooSnippet.ts

+    async getSelectedText(params: CommandParams): Promise<string> {
+        const selectedNode = params.context.range.selectedNodes().map(node => node.clone());
+        const container = new ContainerNode();
+        container.append(...selectedNode);
+        const renderer = this.editor.plugins.get(Renderer);
+        const renderedNode = await renderer.render<HTMLElement[]>(
+            HtmlDomRenderingEngine.id,
+            container,
+        );
+        // todo: what if there is more rendered nodes?
+        return renderedNode ? renderedNode[0].innerText : '';
+    }


Uh? Why do you go through the rendering for that?

getSelectedText(params: CommandParams): string { const targettedNodes = params.context.range.targettedNodes(); let text = ''; for (const node of targettedNodes) { if (!node.ancestor(ancestor => targettedNodes.includes(ancestor))) { text += node.textContent; } } return text; }

Zynton · 2020-05-18T13:11:02Z

packages/plugin-odoo-snippets/src/OdooSnippet.ts

+        let classes: string;
+        if (vNode instanceof InlineNode) {
+            classes = vNode.formats.map(format => format.attributes.class as string).join(' ');
+        } else {
+            classes = vNode.attributes.class as string;
+        }
        return new Set(classes.split(' '));
    }
    _setClasses(vNode: VNode, classes: Set<string>): void {
-        vNode.attributes.class = [...classes].join(' ');
+        const classesString = [...classes].join(' ');
+        if (vNode instanceof InlineNode) {
+            const lastAttribute = vNode.formats[0]?.attributes;
+            if (lastAttribute) {
+                lastAttribute.class = classesString;
+            } else {
+                vNode.formats.push(new Format());
+            }
+        } else {
+            vNode.attributes.class = classesString;
+        }


Irrelevant with ClassList.

packages/plugin-video/src/OdooVideoHtmlDomRenderer.ts

dmo-odoo · 2020-06-04T10:50:32Z

packages/plugin-char/src/Char.ts

@@ -32,7 +33,7 @@ export class Char<T extends JWPluginConfig = JWPluginConfig> extends JWPlugin<T>
            handler: this.insertText,
        },
        insertHtml: {
-            handler: this.insertHtml,
+            handler: this.insertHtml.bind(this),


No it's not. The idea (at least at first) was to be able to associate something like makeBold to applyFormat(BOLD). The point is the method name on the plugin does not necessarily match the command name.

dmo-odoo · 2020-06-04T10:51:18Z

packages/plugin-char/src/Char.ts

+        if (!domParser) {
+            // TODO: remove this when the editor can be instantiated on
+            // something else than DOM.
+            throw new Error(`No DOM parser installed.`);


I don't think I agree with this comment. I'm not sure, though.

packages/plugin-char/test/Char.test.ts

packages/plugin-dom-layout/src/DomLayout.ts

dmo-odoo · 2020-06-04T11:01:36Z

packages/plugin-html/src/HtmlNode.ts

+export class HtmlNode extends AtomicNode {
+    domNode: Node;
+    constructor(params: HtmlNodeParams) {
+        super();
+        this.domNode = params.domNode;
+    }
+}


I suppose it's to have a VNode in our tree that represents a dom Node that needs to specifically be the exact same dom Node at rendering rather than being re-generated, to keep the listeners etc etc. If that's the case then I guess the name of the class could better reflect this.

dmo-odoo · 2020-06-04T11:39:58Z

packages/plugin-toolbar/assets/Toolbar.xml

@@ -37,7 +37,7 @@
                            </select>
                        </t>
                        <t t-else="">
-                            <button t-attf-class="fas {{toolbarItem.class}} fa-fw"
+                            <button t-attf-class="fa {{toolbarItem.class}} fa-fw"


I guess Odoo does not use the same version of FontAwesome as the one @Zinston used ?

This is the previous version of FontAwesome indeed. I thought Odoo had upgraded to FA5 though...

packages/plugin-video/src/OdooVideoHtmlDomRenderer.ts

dmo-odoo · 2020-06-04T11:41:38Z

test/core/JWEditor.test.ts

+// import { expect } from 'chai';
+// import JWEditor from '../../packages/core/src/JWEditor';
+
+// function createElementAndAppend(): HTMLElement {
+//     const element = document.createElement('div');
+//     element.innerHTML = `
+//       <p>Hello world!</p>
+//     `;
+//     document.body.appendChild(element);
+//     return element;
+// }
+
+// describe('core', () => {
+//     describe('JWEditor', () => {
+//         let element;
+//         beforeEach(() => {
+//             element = createElementAndAppend();
+//         });
+//         afterEach(() => {
+//             element.remove();
+//         });
+//         it.skip('init and start without argument', () => {});
+//         it.skip('init and start with text as value', () => {});
+//         it.skip('init and start with html as value', () => {});
+//         it('init and start with node inside the dom', () => {
+//             const editor = new JWEditor(element);
+//             editor.start();
+//             expect(element.parentElement).to.equal(document.body);
+//             expect(editor.el.parentElement).to.equal(document.body);
+//             editor.stop();
+//             expect(editor.el.parentElement).to.be.null;
+
+//             // try again to check if multiples call generate inconsistencies
+//             editor.start();
+//             expect(element.parentElement).to.equal(document.body);
+//             expect(editor.el.parentElement).to.equal(document.body);
+//             editor.stop();
+//             expect(editor.el.parentElement).to.be.null;
+//         });
+//         it.skip('init and start with node outside the dom', () => {});
+//     });
+//     describe('getValue', () => {
+//         it('should properly get the value once edited', () => {
+//             const element = createElementAndAppend();
+//             const editor = new JWEditor(element);
+//             editor.start();
+//             // todo: simulate keyboard eventns
+//             expect(editor.getValue()).to.equal('<div><p>Hello world!</p></div>');
+//             editor.stop();
+//             element.remove();
+//         });
+//     });
+//     describe('setValue', () => {
+//         it('should properly set the value', () => {
+//             const element = createElementAndAppend();
+//             const element2 = document.createElement('p');
+//             element2.innerHTML = 'This is a paragraph';
+
+//             const editor = new JWEditor(element);
+//             editor.start();
+//             editor.setValue(element2);
+//             expect(editor.getValue()).to.equal(element2.outerHTML);
+//             // debugger;
+//             editor.stop();
+//             element.remove();
+//             element2.remove();
+//         });
+//     });
+// });


You mean delete ?

dmo-odoo · 2020-06-04T11:42:25Z

webpack-base.config.js

+                        content,
+                        'return JWEditor',
+                        '});',
+                    ].join('\n');


Did you look at how they did it in owl for that ?

dmo-odoo · 2020-06-04T11:43:11Z

webpack-base.config.js

+    'packages/build-odoo-integration/odoo-integration.ts',
+);
+
+const mainConfig = {


We should have moved the file first in order to read the actual diff.

packages/plugin-video/src/OdooVideo.ts

Zynton · 2020-06-04T15:11:36Z

packages/core/src/Dispatcher.ts


            await this._dispatchHooks(commandId, args);
        }
+        return result;


result type: any
Return type: Promise<void>
return result;
...

Zynton · 2020-06-04T15:12:49Z

packages/core/src/JWEditor.ts

@@ -277,7 +289,7 @@ export class JWEditor {
        commandName: C,
        params?: CommandParams<P, C>,
    ): Promise<void> {
-        await this.dispatcher.dispatch(commandName, params);
+        return await this.dispatcher.dispatch(commandName, params);


This goes with the comment above about the Dispatcher.

packages/plugin-odoo-snippets/src/OdooStrucutreXmlDomParser.ts

Zynton · 2020-06-04T15:56:28Z

webpack-base.config.js

+                    const content = await fs.promises.readFile(filename);
+                    const newContent = [
+                        "odoo.define('web_editor.jabberwock', function(require) {",
+                        "// 'use strict';",


Why the // ?

packages/core/src/Modifiers.ts

[FIX] OdooSnippet: apply classes to LinkFormat instead of CharNodes (and unbreakable divider)

dmo-odoo · 2020-06-11T12:48:28Z

I'm closing this PR in favor of #210.

Goaman changed the title ~~temp~~ Odoo integration Mar 18, 2020

dmo-odoo added the wip Currently being worked on label Mar 18, 2020

Goaman force-pushed the master-odoo-implement-nby branch from 3b615fa to 8231bba Compare April 2, 2020 17:35

Goaman force-pushed the master-odoo-implement-nby branch 2 times, most recently from 3d92fdf to 1022cdb Compare April 27, 2020 09:47

Goaman force-pushed the master-odoo-implement-nby branch 3 times, most recently from e4414f2 to 441457e Compare May 14, 2020 14:34

Zynton reviewed May 18, 2020

View reviewed changes

Goaman force-pushed the master-odoo-implement-nby branch from 441457e to 7515027 Compare May 27, 2020 10:25

Goaman force-pushed the master-odoo-implement-nby branch from 7515027 to 4b47b7d Compare June 4, 2020 07:20

dmo-odoo reviewed Jun 4, 2020

View reviewed changes