pml-lang / pml-companion

Java source code of the 'PML Companion (PMLC)'
https://www.pml-lang.dev
GNU General Public License v2.0
22 stars 1 forks source link

Image source is relative to target directory, not source directory #46

Closed coemgenuslcp closed 3 years ago

coemgenuslcp commented 3 years ago

The image tag help says this about its source attribute:

Description    : The relative file path to the image file. The path is relative to the target HTML file.
Examples       : source = images/ball.png

I get that source translates directly to href in the generated HTML anchor. However, I see a few problems here:

  1. User expectation. Since PML is a compiled language, a user who means his PML file /workspace/doc.pml to include an internal image /workspace/images/ball.png would naturally expect the source attribute to be relative to the source folder containing the PML file (or, less ideally, to the working directory where the converter is run), but certainly not to the target directory.
  2. Portability. With the path being relative to the target directory instead of the source directory, the PML code now has to be made aware of the location of the target directory, which otherwise should be defined only in the converter arguments, thereby making the PML code non-portable: The PML code effectively has to either know where the HTML file will be generated (e.g. source = "../../../workspace/images/ball.jpg") or else rely on hard-coded absolute paths (source = "/workspace/images/ball.jpg"), neither of which is practical when the developer decides to deploy his HTML to a server or move it to another machine.
  3. Completeness. With the converter expecting the image to already exist in the target directory, the image is effectively omitted from the build. This means that the image tag represents not an image but only an image reference, meaning any internal images must be deployed to the target outside the PML conversion process.

I propose it would be more helpful, in the case of internal images, for said images to be bundled by the converter into the output directory, along with the HTML file and CSS. Then the source attribute would be relative to the source directory instead of the target directory, a state that is much more in line with a common user's expectation of the proper usage of an image tag in a compiled language. The same rationale extends to the video and audio tags as well.

Of course, this solution concerns only the case of internal images, not the case of external image references. Supporting both cases would perhaps necessitate a new attribute.

coemgenuslcp commented 3 years ago

By the way, this is one fantastic project. I wish it much success.

pml-lang commented 3 years ago

Good point!

it would be more helpful, in the case of internal images, for said images to be bundled by the converter into the output directory

That's the case already.

The problem here is that the documentation is wrong.

Instead of saying: "The path is relative to the target HTML file." ... it should say: "The path is relative to the 'resources_directory' command line argument (default is subdirectory 'resources' in the working directory)"

Here is how it actually works:

The doc for argument resources_directory states:

Optional directory of resources (e.g. media files) that will be copied into the output directory. If a relative path is specified then it is relative to the current working directory. By default, sub-directory 'resources' in the current working directory is used, but only if such a directory exists.

I've changed the documentation for tags image, audio, and video.

The change will be included in the next PML version 2.0.

I will close this issue when PML 2.0 is public.

Thanks for your feedback.

pml-lang commented 3 years ago

this is one fantastic project. I wish it much success.

Thank you so much! Glad you like it.

coemgenuslcp commented 3 years ago

Ah, I had indeed forgotten to specify the resources_directory, and so the generated directory was being placed under my converter installation directory's lib subfolder by default, a totally different location from the output_directory that I had specified. (Perhaps if not specified the default should be under the output_directory, but we can debate that in another issue.)

For the benefit of future readers: pmlc ci --command convert not only describes the arguments but also shows the default locations on your filesystem for the arguments you neglect to specify. Very helpful.

pml-lang commented 3 years ago

pmlc ci --command convert not only describes the arguments but also shows the default locations on your filesystem for the arguments you neglect to specify.

That's an excellent idea.

Maybe it would also be useful to display the actual values for directories (and other command line arguments) used when the command is executed.

I suggest you open a new issue for this enhancement (to keep things separated).

coemgenuslcp commented 3 years ago

Yes, I was seeing it is the case for "default value", so it might be a good case as well for "value at run" or the like. I'll post a suggestion for it.

2.
Id             : HTML_header, html_header, header
Required       : no
Default value  : /home/user/myapps/PML/pml-to-html-converter_1.5.0-1_amd64/lib/runtime/resources/html/default_html_header.txt

I must also mend my previous comment. Apologies for the confusion when I said my omission of resources_directory resulted in a resources directory being generated somewhere else. I believe now that was wrong. What seems to really have happened is that, with the default value being null, the resources directory was not generated at all, but still the image worked (in my case) because the relative path in the image anchor in the output file coincidentally also resolved to the same location where my input image was.

pml-lang commented 3 years ago

The problem here is that the documentation is wrong.

This bug has been fixed in version 2.0.0. See attribute Image Source in chapter image node

pml-lang commented 3 years ago

Closed because the enhancement is now well described by @coemgenuslcp in #48