Skip to content
This repository has been archived by the owner on Sep 15, 2024. It is now read-only.

Fix inline PDF preview in expensereport #148

Closed
wants to merge 2 commits into from
Closed

Conversation

rqi14
Copy link

@rqi14 rqi14 commented Apr 7, 2024

In expensereport, it requires imagick and ghostscript to convert pdf and show an inline preview. This pull request is to include the installation and configuration of these packages

@tuxgasy
Copy link
Owner

tuxgasy commented Apr 8, 2024

I use expense reports module. I can preview PDF inline (without download it). As in other documents (order, invoice...), this feature doesn't require imagick/ghostscript.

@rqi14
Copy link
Author

rqi14 commented Apr 9, 2024

I use expense reports module. I can preview PDF inline (without download it). As in other documents (order, invoice...), this feature doesn't require imagick/ghostscript.

There are two places where pdf inline preview is available. One is the list of uploaded files at the bottom of the page. In this one imagick is not needed for preview. The other one is within each row of the expense table. At the most right hand side there should be a clickable icon to preview the file associated with the row. In the dolibarr code, it uses imagick to convert the pdf to png and use the png as the icon. If it cannot do that, it skips the div where the preview link sits in.

@rqi14
Copy link
Author

rqi14 commented Apr 9, 2024

I use expense reports module. I can preview PDF inline (without download it). As in other documents (order, invoice...), this feature doesn't require imagick/ghostscript.

You can find the code to call imagick here https://github.com/Dolibarr/dolibarr/blob/56c198992dce8c29880e84ec6e142e697df6c72d/htdocs/expensereport/card.php#L2210

dol_convert_file requires imagick and ghostscript

@tuxgasy
Copy link
Owner

tuxgasy commented Apr 9, 2024

I use expense reports module. I can preview PDF inline (without download it). As in other documents (order, invoice...), this feature doesn't require imagick/ghostscript.

You can find the code to call imagick here https://github.com/Dolibarr/dolibarr/blob/56c198992dce8c29880e84ec6e142e697df6c72d/htdocs/expensereport/card.php#L2210

dol_convert_file requires imagick and ghostscript

Ok I have never used this part.

Copy link
Owner

@tuxgasy tuxgasy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this will make the image bigger. Maybe it's better to document it or add an example. Let's see.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you don't need to create a separate script. The xml path will be always the same, you can run sed command directly in Dockerfile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for futural compatibility. The configuration folder depends on the version of Imagick. For example, in the current docker container, it is /etc/ImageMagick-6

@rqi14
Copy link
Author

rqi14 commented Apr 10, 2024

I'm afraid this will make the image bigger. Maybe it's better to document it or add an example. Let's see.

Here is an example (screenshots). Previews available at the end of each row and they are clickable.
image
image

@tuxgasy
Copy link
Owner

tuxgasy commented Apr 10, 2024

I'm afraid this will make the image bigger. Maybe it's better to document it or add an example. Let's see.

Here is an example (screenshots). Previews available at the end of each row and they are clickable. image image

I'm talking about the docker image size. It's already very big. I prefer not to increase it further.

@rqi14
Copy link
Author

rqi14 commented Apr 11, 2024

I'm afraid this will make the image bigger. Maybe it's better to document it or add an example. Let's see.

Here is an example (screenshots). Previews available at the end of each row and they are clickable. image image

I'm talking about the docker image size. It's already very big. I prefer not to increase it further.
It does increase the size of the image by 200MB ish.

dolibarr-dolibarr                          latest               e7e6edc449ac   3 days ago      978MB
tuxgasy/dolibarr                           latest               e84790d5bec8   2 weeks ago     774MB

It is true that the size gets bigger. But it is necessary for dolibarr to function normally. Alternatively, if you don't wish to increase the image, maybe it can be turned into a separate Dockerfile that users could build based on your image to have this functionality working.

@rqi14
Copy link
Author

rqi14 commented Apr 11, 2024

I'm afraid this will make the image bigger. Maybe it's better to document it or add an example. Let's see.

Here is an example (screenshots). Previews available at the end of each row and they are clickable. image image

I'm talking about the docker image size. It's already very big. I prefer not to increase it further.

Here is an example of the Dockerfile

FROM tuxgasy/dolibarr:latest
RUN apt-get update && apt-get install -y libmagickwand-dev ghostscript  --no-install-recommends
RUN pecl install imagick
RUN docker-php-ext-enable imagick
RUN rm -rf /var/lib/apt/lists/*
COPY update-imagemagick-policy.sh /usr/local/bin/update-imagemagick-policy.sh
RUN chmod +x /usr/local/bin/update-imagemagick-policy.sh
RUN /usr/local/bin/update-imagemagick-policy.sh

@tuxgasy
Copy link
Owner

tuxgasy commented Apr 11, 2024

You add an example in examples folder.

@Vaadasch
Copy link
Contributor

Hi @rqi14
I wonder about the usefulness of these tiny thumbnails from documents that need to be opened to be able to read anything.
Adding 200MB just for that does not seems wise for me

@rqi14
Copy link
Author

rqi14 commented Apr 25, 2024

Hi @rqi14 I wonder about the usefulness of these tiny thumbnails from documents that need to be opened to be able to read anything. Adding 200MB just for that does not seems wise for me

You can click on these thumbnails and previews will show. It is convenient to check each line has correct receipt attached.

@Vaadasch
Copy link
Contributor

Vaadasch commented Apr 25, 2024

I think it would be better to change the Dolibarr code.
You said that "If it cannot do that, it skips the div where the preview link sits in."
It might be possible to create the div with a 📷 icon without the need to generate a png that isn't readable anyway ? Will that be sufficient ?

@rqi14
Copy link
Author

rqi14 commented Apr 26, 2024

I think it would be better to change the Dolibarr code. You said that "If it cannot do that, it skips the div where the preview link sits in." It might be possible to create the div with a 📷 icon without the need to generate a png that isn't readable anyway ? Will that be sufficient ?

I agree the thumbnail here is weird. The preview functionality is available without the thumbnail for the linked files. Your suggestion could be implemented. I am not sure what kind of fix it would be, as technically it is not a bug. It is something I cannot really do because I am not too familiar with php and html. I can sure read it a little bit but not code such a complex page.

@Vaadasch
Copy link
Contributor

Vaadasch commented Apr 26, 2024

You can find the code to call imagick here https://github.com/Dolibarr/dolibarr/blob/56c198992dce8c29880e84ec6e142e697df6c72d/htdocs/expensereport/card.php#L2210

Two lines before this line, did you try to set MAIN_DISABLE_PDF_THUMBS to 1 to see what it does ?

Edit : it does nothing more, i've created a PR.

@rqi14
Copy link
Author

rqi14 commented Apr 26, 2024

You can find the code to call imagick here https://github.com/Dolibarr/dolibarr/blob/56c198992dce8c29880e84ec6e142e697df6c72d/htdocs/expensereport/card.php#L2210

Two lines before this line, did you try to set MAIN_DISABLE_PDF_THUMBS to 1 to see what it does ?

Edit : it does nothing more, i've created a PR.

Tested. The preview won't work with this option set to 1

@Vaadasch
Copy link
Contributor

On this line : https://github.com/Dolibarr/dolibarr/blob/0ceacfdc6a40dfbe84317d9477c37a84dbdfe8f9/htdocs/expensereport/card.php#L2234

Change

		// If the preview file is found
		if (file_exists($fileimage)) {
			$thumbshown = 1;
			$urlforhref = getAdvancedPreviewUrl($modulepart, $relativepath.'/'.$fileinfo['filename'].'.'.strtolower($fileinfo['extension']), 1, '&entity='.(empty($object->entity) ? $conf->entity : $object->entity));
			print '<a href="'.$urlforhref['url'].'" class="'.$urlforhref['css'].'" target="'.$urlforhref['target'].'" mime="'.$urlforhref['mime'].'">';
			print '<img height="'.$heightforphotref.'" class="photo photowithmargin photowithborder" src="'.DOL_URL_ROOT.'/viewimage.php?modulepart=apercu'.$modulepart.'&amp;file='.urlencode($relativepathimage).'">';
			print '</a>';
		}
	}
}

if (!$thumbshown) {
	print img_mime($ecmfilesstatic->filename);
}

By :

		$urlforhref = getAdvancedPreviewUrl($modulepart, $relativepath.'/'.$fileinfo['filename'].'.'.strtolower($fileinfo['extension']), 1, '&entity='.(empty($object->entity) ? $conf->entity : $object->entity));
		print '<a href="'.$urlforhref['url'].'" class="'.$urlforhref['css'].'" target="'.$urlforhref['target'].'" mime="'.$urlforhref['mime'].'">';
		// If the preview file is found we display the thumb
		if (file_exists($fileimage)) {
			print '<img height="'.$heightforphotref.'" class="photo photowithmargin photowithborder" src="'.DOL_URL_ROOT.'/viewimage.php?modulepart=apercu'.$modulepart.'&amp;file='.urlencode($relativepathimage).'">';
		}
		// Else, we display an icon
		else { 
			print img_mime($ecmfilesstatic->filename); 
		}
		print '</a>';

@Vaadasch
Copy link
Contributor

The PR has been merged.
@rqi14, can you close this one ?

@rqi14 rqi14 closed this Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants