knowledge icon indicating copy to clipboard operation
knowledge copied to clipboard

[16.0][MIG] attachment_preview: Migration to 16.0

Open houzefa-abba opened this issue 1 year ago • 9 comments

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: Screenshot at 2023-09-29 15-13-28

Inline preview of a PDF: Screenshot at 2023-09-29 15-14-48

Inline preview of an ODT (which I navigated to using left arrow at the top of the viewer): Screenshot at 2023-09-29 15-15-09

Binary field with button to open in new tab: Screenshot at 2023-09-29 15-15-57

houzefa-abba avatar Sep 29 '23 08:09 houzefa-abba

Hi this PR is ready, please review

@vancouver29 @HolgerNahrstaedt @holgern I am tagging you as you worked on https://github.com/OCA/knowledge/pull/363 (15.0 mig)

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

houzefa-abba avatar Sep 29 '23 13:09 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?

len-foss avatar Oct 09 '23 15:10 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?

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.

houzefa-abba avatar Oct 09 '23 16:10 houzefa-abba

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

Screenshot from 2023-10-10 08-47-12

astirpe avatar Oct 10 '23 06:10 astirpe

@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.

len-foss avatar Oct 10 '23 07:10 len-foss

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

Screenshot from 2023-10-10 08-47-12

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 avatar Oct 10 '23 14:10 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

houzefa-abba avatar Oct 12 '23 13:10 houzefa-abba

/ocabot migration attachment_preview

hbrunn avatar Oct 17 '23 17:10 hbrunn

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.

github-actions[bot] avatar Mar 24 '24 12:03 github-actions[bot]