[16.0][MIG] attachment_preview: Migration to 16.0

[houzefa-abba]

I squashed a few commits according to https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests

This PR has 2 commits: 1 with the actual migration; 1 where I move code around without changing anything. I can squash them if you feel it's better

I made sure this plays well with web_responsive & especially web_chatter_position with its 3 chatter settings (responsive / bottom / sided)

I added a ROADMAP file with possible future improvements

Overall implementation remains the same as in previous versions:

• patch into attachment list to prepare prev/next buttons in the viewer
• patch into attachment cards to observe clicks on new buttons this
  module adds
• fetch attachment extensions
• add an iframe into the DOM tree next to the main form one, display
  viewer inside it
• add preview button within binary fields

Main changes in this migration:

• update imports/exports to proper JS modules - in particular, this
  fixes "service already defined" console messages we also get in 15.0
• rework FormRenderer injector, previous (legacy) one was no longer used
• patch attachment list / cards with new methods in the mail module
• fix preview button inclusion within binary fields
• fix previous/next viewer buttons

Screenshots from runboat

2 buttons in attachment cards to preview inline & open in new tab:

Inline preview of a PDF:

Inline preview of an ODT (which I navigated to using left arrow at the top of the viewer):

Binary field with button to open in new tab:

[houzefa-abba]

Hi this PR is ready, please review

@vancouver29 @HolgerNahrstaedt @holgern I am tagging you as you worked on #363 (15.0 mig)

@hbrunn I am tagging you as you bootstrapped this module, in case you're still around

[len-foss]

Thank you for migrating this!

Despite having no browser or server error, the preview doesn't seem to work in either FF or Chrome for anything but pdf files.
With other types that should be supported by viewerJS (doc, docx, ...), the preview is just empty, and opening in a new tab it spawns a "not supported filetype" popup.
I've tested on two device with the same results. Do you have any idea about what could be wrong?

[houzefa-abba]

Thank you for migrating this!

Despite having no browser or server error, the preview doesn't seem to work in either FF or Chrome for anything but pdf files. With other types that should be supported by viewerJS (doc, docx, ...), the preview is just empty, and opening in a new tab it spawns a "not supported filetype" popup. I've tested on two device with the same results. Do you have any idea about what could be wrong?

Hi @len-foss did you test on runboat? My screenshots in this issue are from runboat; the file with "character formatting" / "paragraph formatting" is an ODT file.

I'm using Chromium mainly, but I just tried with Firefox in an incognito window; no problem either.

At the moment, the only supported file extensions are those in attachment_preview/static/src/js/utils.esm.js; doc & docx are not included there.

In this PR I did not update the viewerjs lib; we could update it in a separate PR and add new supported filetypes in there.

[astirpe]

@houzefa-abba I tried in combination with web_responsive, there is a glitch:

[len-foss]

@houzefa-abba oh thanks, indeed it works fine with odt files, runboat or local. I agree doing the update in a separate PR is the proper way to do it, let's just hope it gets through quickly for once.

I confirm the behavior reported by @astirpe, although I wouldn't call it a glitch. It seems you could trivially fix it by setting the z-index of attachment_preview_widget to 199 instead of 999 -- the full-screen-dropdown has a z-index of 200, so that would go below. The 200 value is also used by the pdf viewer so it seems to confirm that idea.

[houzefa-abba]

@houzefa-abba I tried in combination with web_responsive, there is a glitch:

Good catch! Fixed with z-index change suggested by @len-foss.
I went with z-index=100 as most Odoo elements are between 1-10 and other ones such as pop-ups are >=200.

I also used some if (x) return; 1-liners which I noticed in your code 🙃

[houzefa-abba]

I added widget clean-up in my last change, otherwise I was getting multiple widgets in the DOM tree when navigating in and out of form views

[hbrunn]

/ocabot migration attachment_preview

[astirpe]

At the moment we have two different functionalities for the PDF preview on forms:

• standard Odoo PDF preview
• this module (attachment_preview)

Would be nice to have an option to disable this attachment preview on forms for this module (attachment_preview). This is a requirement for one of our customers. So the idea is that the admin could decide to configure the option (enable/disable). This way, when the option is disabled, the standard Odoo preview will be work in place of this module (attachment_preview).

Disabling this option can be easily achieved by returning res just after the _super(..).

        var res = this._super(...arguments);
        if (is_attachment_preview_disabled) return res;

[houzefa-abba]

Yea makes perfect sense. To take it even further, I did ponder whether this module could entirely re-use components added by Odoo to implement their preview feature...

I won't be able to get to this until at least next week.

[houssine78]

@houzefa-abba did you get time to get to this or you plan to do it in the near future ? thanks for this PR anyway !

[houzefa-abba]

Hi this is still in my TODO but it has moved further back 😅
Very unlikely I could get back to this PR until end of May

This per-form setting hinges between fix (imposed by odoo16 adding its own preview system) and evolution; perhaps we could add it as a separate evolution PR so this initial PR can get moving as is?

[houzefa-abba]

Hi I got back onto this topic for a bit

I just rebased on top of latest 16.0 and added a commit to disable Odoo's default attachment sidebar can show up in views with <div class="o_attachment_preview" />

However I kept pondering @astirpe's idea above but I can't see a way it fully makes sense:

• Global setting: would be useful to disable our viewer in forms in favor of odoo's own viewer when one still wants to use our viewer somewhere else, but it doesn't make much sense to me: such an odoo app would use 2 different viewers based on view type, potentially displaying the same documents in different ways depending on view type.
• Per-form setting (on the <form> tag): again, I can't see a use case, especially when we assume our viewer is similar (and in parts more powerful) than odoo's base one.

Could we think about it some more before confirming we need such a setting? In a separate PR possibly?

Thanks

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.