pa-decarvalho / mkdocs-asciinema-player

Mkdocs Plugin to include asciinema player in your documentation.
https://pa-decarvalho.github.io/mkdocs-asciinema-player/
MIT License
7 stars 1 forks source link

Frame possible initialization issue #24

Closed llaville closed 2 months ago

llaville commented 3 months ago

Hello @Paul-Riviere

I've just finalized one of my documentation project (now online at https://llaville.github.io/box-manifest/4.x/), and found a possible frame initialization issue with version 0.10.1

With a fresh web browser cache (without documentation contents), here is what I got when I visit the play terminal sessions part on main page.

llaville github io_box-manifest_4 x_#play-terminal-sessions

To have ability to play movies, I need to click on F5 to refresh page and see the correct frame

llaville github io_box-manifest_4 x_correct-frame

I've the same issue : online on GH-Pages and with my local virtual host, and also with mkdocs serve command.

NB: when I test it locally with mkdocs serve -f mkdocks.local.yml, the config file is same as this one (https://github.com/llaville/box-manifest/blob/4.x/mkdocs.yml#L3 except I removed site_url setting).

FYI : all documentation including movies are available on branch 4.x into docs folder of my repo https://github.com/llaville/box-manifest/tree/4.x

Tested on Firefox version 129

llaville commented 3 months ago

Oh, and I've also noticed that I can't play movie online on GH-Pages, while it's ok locally with mkdocs serve command.

127 0 0 1_8000_#play-terminal-sessions

llaville commented 3 months ago

Tested right now with Microsoft Edge (version 127.0.2651.105) and got same behaviour issue

Paul-Riviere commented 3 months ago

Hello @llaville , first of all thanks for this issue.

I've looked at your repo, and noticed that on the gh-pages your cast files'l links seem to be broken : "assets/asciinema/installation.cast" instead of "4.x/assets/asciinema/installation.cast" on your README.md.

Even if the links are broken, I don't have the same behavior as you have, on Chrome (Version 127.0.6533.120).

On Microsoft Edge (Version 127.0.2651.105) I can't reproduce what you have.

Do you have any errors in the console when you have the issue ? We had surch behavior when the generated documentation didn't load properly Asciinema's js script.

NB: I only tested with the gh-pages and didn't clone your repo for now, please let me know if the probleme persists even if you change the cast link.

llaville commented 3 months ago

Hello @llaville , first of all thanks for this issue.

Hello, and thanks for your reply

I've looked at your repo, and noticed that on the gh-pages your cast files'l links seem to be broken : "assets/asciinema/installation.cast" instead of "4.x/assets/asciinema/installation.cast" on your README.md.

It's not an error. I want to have a portable documentation, that may be serve by any web host. That's means, it should be displayed (correctly) on GH-Pages but also with mkdocs serve command or any Virtual Host.

Do you have any errors in the console when you have the issue ? We had surch behavior when the generated documentation didn't load properly Asciinema's js script.

After many (commit) tries :

I've applied this one :

It works as expected on all situations : GH-Pages, locally with mkdocs serve and also on my vhost.

Even, if there is an initialization wasm error (see screenshot)

llaville github io_box-manifest_4 x

FYI:

    window.addEventListener("load", function(event) {
        AsciinemaPlayer.create("/./assets/asciinema/installation.cast", document.getElementById("asciinema-player-0"), {

For me, it should be served as given, that means AsciinemaPlayer.create("./assets/asciinema/installation.cast" (without leading slash)

window.addEventListener("load", function(event) {
        AsciinemaPlayer.create("/box-manifest/./assets/asciinema/installation.cast", document.getElementById("asciinema-player-0"), {

Finally, removing site_url mkdocs setting fixed issue, but IMO is only a workaround !!!

llaville commented 3 months ago

Works with MS Edge and Firefox !

llaville commented 3 months ago

IMO, we should have the same principle as image serving :

For example:

Portable documentation, is as image serve, with always the same site_url, i.e : https://github.com/llaville/sarif-php-sdk/blob/1.0/mkdocs.yml#L3

The Graph Composer serve at https://llaville.github.io/sarif-php-sdk/1.0/getting-started/, did not require to specify subfolder 1.0 (see https://github.com/llaville/sarif-php-sdk/blob/1.0/docs/getting-started.md?plain=1#L10, and is a relative path to host server)

Same, for a new folder 2.0 https://llaville.github.io/sarif-php-sdk/2.0/ We still keep the same mkdocs setting (site_url) : https://github.com/llaville/sarif-php-sdk/blob/2.0/mkdocs.yml#L3

And with relative path https://github.com/llaville/sarif-php-sdk/blob/2.0/docs/README.md?plain=1#L9 It's serve as expected : https://llaville.github.io/sarif-php-sdk/2.0/

Paul-Riviere commented 3 months ago

Hello @llaville , I figured out what seems to break your cast files's links.

When you deploy you gh-pages on the gh-pages branch, if you navigate through it on the repo directly, you have a "4.x" folder, and not the code directly. I'm pretty sure that's intended, but this is the code creating this foler : https://github.com/llaville/box-manifest/blob/4.x/.github/workflows/gh-pages.yml#L17C13-L17C33

As your gh-pages branch doesn,t have an index at the root folder, I think that the links generated by mkdocs starts where the index is (when the site_url is defined) so when you define the site_url without the "4.x" this generates links without it, but as you have a "4.x" folder, all links are broken.

What I suggest is :

Also, if you put the 4.x in your URL the way you're doing it with this subfolder, when you upgrade to the 5.x for example, the links you gave to people, or anywhere, will be broken. If you manage versions directly inside your docs folder, you can easily manage versions, and keep served the documentation of older major releases you have, so that users can access old documentation.

I assume these aren't perfect solutions, but personally I would create a subfolder inside the docs folder, because as you said, removing the site_url is only a workaround that needs to be fixed.

Hope you'll find out how to manage it correctly, let's keep the issue open, in case we have something to do on the plugin repo 🙂

llaville commented 3 months ago

Hello @Paul-Riviere

First, I would like to thanks you to take time to give me a such so long answer. I appreciated a lot, even if I'm not agree with you.

As Mkdocs documentation mentioned it : https://mkdocs.readthedocs.io/en/859/user-guide/configuration/#site_url

mkdocs readthedocs io_en_859_user-guide_configuration_#site_url

This setting is only for canonical URL, used by web crawlers like Google, in SEO context, but IMO it should not break render of a page !

Currently, if I define the site_url in my mkdocs.yml config file like that :

site_url: https://llaville.github.io/box-manifest/4.x

Pages built included, as expected in <head> the :

<link rel="canonical" href="https://llaville.github.io/box-manifest/4.x/">

And on the index.html page generated, we can find such following contents :

<script>
    window.addEventListener("load", function(event) {
        AsciinemaPlayer.create("/box-manifest/4.x/./assets/asciinema/installation.cast", document.getElementById("asciinema-player-0"), {
            cols: 80,
            rows: 24,
            autoPlay: false,
            preload: false,
            loop: false,
            startAt: "0:00",
            speed: 1.0,
            theme: "asciinema",
            fit: "width",
            controls: "auto",
            pauseOnMarkers: false,
            terminalFontSize: "small",
            terminalFontFamily: "",
            terminalLineHeight: 1.33333333,
        });
    });
</script>

But it will probably works only online in GH-Pages context, and not when you test it locally with : mkdocs serve or on a virtual host (mine serve pages at my_project dvl to. See following screenshot (error on console)

my_project dvl to

I suggest to modify your code (https://github.com/pa-decarvalho/mkdocs-asciinema-player/blob/0.10.1/src/mkdocs_asciinema_player/plugin.py#L147) by this line

parsed_json["file_path"] = parsed_json["file"]

Or just used it into your template https://github.com/pa-decarvalho/mkdocs-asciinema-player/blob/0.10.1/src/mkdocs_asciinema_player/templates/asciinema-player.html#L8

If I simulate the FIX, and edit the contents of my index.html and replaces

        AsciinemaPlayer.create("/box-manifest/4.x/./assets/asciinema/installation.cast", document.getElementById("asciinema-player-0"), {

by

        AsciinemaPlayer.create("./assets/asciinema/installation.cast", document.getElementById("asciinema-player-0"), {

My Vhost is able to play the cast file, and the canonical link is present.

my_project dvl to--fixed

To finish, removing site_url is a workaround for me to be able to play cast file in all contexts (without to remember, if I should remove or keep the setting).

Hope you understand my point of view !

llaville commented 3 months ago

FYI, I've just published final version 4.0 of my documentation online on GH-Pages (with site_url defined), and of course in this context, and only this one, it works as expected with your plugin v0.10.1

https://llaville.github.io/box-manifest/4.0/

pa-decarvalho commented 2 months ago

Hello @llaville,

I wanted to change the functionality of the file variable to allow for a URL.

Example:

{
    "file": "https://raw.githubusercontent.com/pa-decarvalho/mkdocs-asciinema-player/main/docs/assets/asciinema/asciinema_example.cast"
}

Here are the changes made to test it:

plugin.py:

- parsed_json["file_path"] = self.site_url + parsed_json["file"]

asciinema-player.html:

- AsciinemaPlayer.create("{{ file_path }}", document.getElementById("{{ 'asciinema-player-' ~ match_id }}"), {
+ AsciinemaPlayer.create("{{ file }}", document.getElementById("{{ 'asciinema-player-' ~ match_id }}"), {

This works well whether the site_url variable is defined or not. However, the paths no longer work as I wanted. Using site_url allowed me to have a consistent path to the assets that didn't change regardless of the page on the site (which was equivalent to having "site_url" + "file").

So, with this change, on the home page (index.md), this does not cause any issues, but on another page, the path needs to be adjusted.

index.md page:

{
    "file": "assets/asciinema/asciinema_example.cast"
}

themes.md page:

{
    "file": "../assets/asciinema/asciinema_example.cast"
}

For themes.md, ../ is necessary because the URL becomes "https://base_url/themes/".

This remains open for discussion as to whether this behavior is desired or not.

So, I have two suggestions:

  1. Directly insert the file variable into the asciinema-player.html file (which I like less).
  2. Create two variables. Keep the file_path variable as it currently works, and add another variable, perhaps free_path, which would not include the site_url and would also allow for setting a URL.

For the second suggestion, we could ensure that if file_path is defined, then free_path is ignored:

Example of file_path:

{
    "file_path": "assets/asciinema/asciinema_example.cast"
}

Example of free_path:

{
    "free_path": "box-manifest/4.x/assets/asciinema/installation.cast"
}

or

{
    "free_path": "https://raw.githubusercontent.com/pa-decarvalho/mkdocs-asciinema-player/main/docs/assets/asciinema/asciinema_example.cast"
}
llaville commented 2 months ago

I don't like idea to introduce the free_path that won't work as file_path. Ideally I want to write a Mkdocs Documentation site that may be served in all conditions (on all servers) without to change the contents of *.md pages.

Actually with the mkdocs.yml config file and site_url set to value https://llaville.github.io/box-manifest/4.0 => See : https://github.com/llaville/box-manifest/blob/4.x/mkdocs.yml#L3

I am able to read asciinema movies :

mkdocs serve
INFO    -  Building documentation...
INFO    -  Cleaning site directory
INFO    -  Documentation built in 0.55 seconds
INFO    -  [16:06:23] Watching paths for changes: 'docs', 'mkdocs.yml'
INFO    -  [16:06:23] Serving on http://127.0.0.1:8000/box-manifest/4.0/

After all it's not a such IMPORTANT issue, as I am able to see movies on mkdocs serve URL : http://127.0.0.1:8000/box-manifest/4.0/

I'll stay like this because, as I said, the most important IMO is to have a portable documentation without need to apply some changes.

I'll always have the same canonical URL whatever the host is used : (it's theorical wrong, but I don't have any web crawler that will index my localhost ;-) so it's just a warning !

pa-decarvalho commented 2 months ago

Hi @llaville ,

Since the current setup works for you and no further changes are necessary, we can proceed to close this issue.

If you ever encounter any issues or need to revisit this topic, feel free to reopen the issue at any time.

Thanks again for your collaboration!