Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated /docs/source/PCs/ARM/RK3399/Manuals/Hardware/index #259

Merged
merged 7 commits into from
Mar 23, 2024
Merged

Conversation

iSOLveIT
Copy link
Contributor

@iSOLveIT iSOLveIT commented Mar 21, 2024

Fixed image target link issues in /docs/source/PCs/ARM/RK3399/Manuals/Hardware/index.

When setting the value for the :target: option under the ..image directive, it will be best if we use relative target links.

So instead of using an absolute path which works not under the /docs/ path like below:

.. |PPC-A72-170-C| image:: /Media/ARM/A72/CS12102R170P/CS12102R170P-Front-Low.jpg
   :class: index-item-img
   :target: /PCs/ARM/RK3399/Manuals/Hardware/CS12102R170P.html

We can set the :target: option to a relative path like below:

.. |PPC-A72-170-C| image:: /Media/ARM/A72/CS12102R170P/CS12102R170P-Front-Low.jpg
   :class: index-item-img
   :target: RK3399/Manuals/Hardware/CS12102R170P.html

.. OR

.. |PPC-A72-170-C| image:: /Media/ARM/A72/CS12102R170P/CS12102R170P-Front-Low.jpg
   :class: index-item-img
   :target: ./RK3399/Manuals/Hardware/CS12102R170P.html

The relative path is just a link to the RST document (i.e. RK3399/Manuals/Hardware/CS12102R170P.rst) but will have an .html suffix (i.e. RK3399/Manuals/Hardware/CS12102R170P.html)

@iSOLveIT iSOLveIT requested a review from printfinn March 21, 2024 23:11
@printfinn
Copy link
Contributor

Hi Randy,

Thanks so much for investigating the issue.

When I was rewriting these index pages, I tried it like you proposed, the problem is we have many index pages before combining different product lines into one index page.

For example, we had this: https://docs.chipsee.com/PCs/ARM/RK3399/Manuals/Hardware/index.html, it's a legacy index page (https://github.com/Chipsee/Documentation/blob/main/docs/source/PCs/ARM/RK3399/Manuals/Hardware/index.rst?plain=1).

If I write the relative link like the proposed method, the links in this old index pages will not work, it will show a 404 because the link will go to this page(http://localhost:8000/PCs/ARM/RK3399/Manuals/Hardware/RK3399/Manuals/Hardware/CS10600R070.html), you can test it locally. But some people might still visit these old index pages (which is relatively rare), so it will cause those people to navigate to 404.

I hope this give you more information

@iSOLveIT
Copy link
Contributor Author

If I write the relative link like the proposed method, the links in this old index pages will not work, it will show a 404 because the link will go to this page(http://localhost:8000/PCs/ARM/RK3399/Manuals/Hardware/RK3399/Manuals/Hardware/CS10600R070.html), you can test it locally. But some people might still visit these old index pages (which is relatively rare), so it will cause those people to navigate to 404.

You are right. If you still need those index pages then this solution won't work well. Let me take another look and get back to you.

@iSOLveIT
Copy link
Contributor Author

So upon reading the .. image directive's RST docs, https://docutils.sourceforge.io/docs/ref/rst/directives.html#target, I realised you could pass a reference name as the value for the :target: option like below:

.. |PPC-A72-101| image:: /Media/ARM/A72/CS12800R101P/CS12800R101P-Front-Low.jpeg
   :class: index-item-img
   :target: `PPC-A72-101`_

This solves the problem for both the new and old index pages but this approach is not working in the docs because we refer to the reference name (PPC-A72-101_) before it is even defined. Hence we get this error:

/home/isolveit/Documents/myCodes/Codes/Work/chipsee_demo/chipsee_docs/docs/source/PCs/ARM/RK3399/Manuals/Hardware/index:31: WARNING: undefined label: 'ppc-a72-101'

/home/isolveit/Documents/myCodes/Codes/Work/chipsee_demo/chipsee_docs/docs/source/PCs/ARM/RK3399/Manuals/Hardware/index:31: WARNING: undefined label: 'ppc-a72-101'

I am researching a way to fix this then I am sure it will fix the issue. Will get back to you soon.

@printfinn
Copy link
Contributor

That looks promising!

@printfinn
Copy link
Contributor

printfinn commented Mar 22, 2024

@printfinn
Copy link
Contributor

Maybe we can just leave a link to the combined index page in the legacy index page, and let user click; or let Wordpress redirect user from legacy index page to combined index page?

@printfinn
Copy link
Contributor

After some more investigation, it seems there is no way to handle this :target: PPC-A72-101_ link in Sphinx, sadly.


Can you please also take a look at the icon images in the software part (http://chipsee.kinsta.cloud/docs/PCs/ARM/index-rockchip.html#software)? The images are here:

<img src="/_static/images/os_logo_linux.png" class="img-fluid text-center" width="100px">

And the files are here: https://github.com/Chipsee/Documentation/blob/4a45fae3460b8bf900d1c0b0155916b9da75a14c/docs/source/_static/images/os_logo_linux.png.

Same relative path problem, I don't know if Sphinx is able to handle these.


Please also take a look at the 3D models, we need them like in this page: https://docs.chipsee.com/PCs/Pi/A72/Manuals/Hardware/CS10600RA4070P-D.html#d-model, you can click the big red link and view them in a 3D viewer.

But they don't seem to work in sub folder, like this: http://chipsee.kinsta.cloud/docs/PCs/Pi/A72/Manuals/Hardware/CS10600RA4070P-D.html#d-model.

The 3D model page uses an orphan .RST page: https://github.com/Chipsee/Documentation/blob/main/docs/source/ThreeD/three_d_model.rst?plain=1, and just load a three.js package.

I also need to HEAD for 3D files with JS, to find out how many 3D files there are of a product. For example, the link above has one PPC 3D file, but this one: https://docs.chipsee.com/PCs/Pi/A72/Manuals/Hardware/CS10600RA4070.html#d-model, has two 3D file, because there is no "if-else condition" if Sphinx, I have to HEAD with Javascript every time the page is opened by the user to determine, check this function I wrote for this:

function addParamToThreeD() {
)

With the sub folder, these 3D files in the /_static/3d_models/ will also be changed to /docs/_static/3d_models/, but if I add a /docs/ prefix, I won't be able to test them on my development machine... And I don't know how to add a /doc/ subfolder for testing, because sphinx-autobuild doesn't give that option, it always serves from root path (without /doc/).


I'm not sure if this is the Sphinx way to do all these, I have very little knowledge about Sphinx, if you can find a way to give our users same information, you can alter the codes, it doesn't have to be the way I wrote it, anything that can work can be a solution for us.

@iSOLveIT
Copy link
Contributor Author

Seems like Sphinx doesn't support internal link for images, which is specified by reST but not implemented in Sphinx:

Yeah. By default, the Sphinx target option doesn't support referencing internal hyperlink targets from a different document.
I am currently working on writing a patch to the Sphinx image directive to fix this issue. I will get back to you soon on it.

@iSOLveIT
Copy link
Contributor Author

I will also check the other issues you have stated here and see if I could do something about that too.

…rget:` option

- Updated some software index files to show how it is done.
@iSOLveIT
Copy link
Contributor Author

Hi @printfinn ,
I managed to fix the :target: option URI issue by adding a custom image directive that corrects the URI passed to the :target: option under conf.py.

So if you want to add a document's HTML file as a target link to an image, you just need to pass an absolute path like below:

.. |EPC/PPC-A72-101-C| image:: /Media/ARM/A72/CS12800R101/PPC-A72-101-C-Front-Low.jpeg
   :class: index-item-img
   :target: /PCs/ARM/RK3399/Manuals/Hardware/CS12800R101.html

.. OR

.. image:: /_static/images/os_logo_linux.png
   :width: 100px
   :class: img-fluid text-center
   :target: /PCs/ARM/RK3568/Manuals/Software/Buildroot_Linux_Qt_5_15.html

Then the custom image directive will fix the URI provided to the :target: option by using the document in which the ..image directive was called.

I also updated some software index files to show how it can be done.

I will like to tackle the 3D model issue separately to better understand how to fix it.

@printfinn
Copy link
Contributor

printfinn commented Mar 23, 2024

Thanks Randy, that solves the image not displaying problem for the project.

One more thing to ask, I tested the generated href link of images, it still has to be relative path right? Because I see in the code you gave in the last comment, you used absolute path for image target.

I tested on my own machine, and if I use absolute path in image :target:, it doesn't automatically work like the sphinx toc tree's absolute links right?

I mean:
If I start a python simple server (or you can do with nginx), :

➜  Documentation git:(Update-Relative-Path) ✗ cd docs/build 
➜  build git:(Update-Relative-Path) ✗ ls
html
➜  build git:(Update-Relative-Path) ✗ cd html 
➜  html git:(Update-Relative-Path) ✗ ls
Displays              ThreeD                _images               _static               objects.inv           searchindex.js
Nomenclature          TutorialGuide         _sources              genindex.html         pdf_metadata.json     sitemap.xml
PCs                   _downloads            _sphinx_design_static index.html            search.html
➜  html git:(Update-Relative-Path) ✗ cd ..
➜  build git:(Update-Relative-Path) ✗ ls
html
➜  build git:(Update-Relative-Path) ✗ python3 -m http.server
Serving HTTP on :: port 8000 (http://[::]:8000/) ...

This

.. image:: /_static/images/os_logo_linux.png
   :width: 100px
   :class: img-fluid text-center
   :target: /PCs/ARM/PX30/AIO/Manuals/Software/Buildroot_Linux_qt_5_14.html

Generates: http://localhost:8000/PCs/ARM/PX30/AIO/OS_Downloads/index.html;

But this

.. toctree::
   :maxdepth: 1
   :titlesonly:
   :hidden:

   AIO-PX30-101 OS Downloads </PCs/ARM/PX30/AIO/OS_Downloads/index>
   Debian 10 </PCs/ARM/PX30/AIO/Manuals/Software/Debian_10>
   Buildroot Linux Qt 5.14 </PCs/ARM/PX30/AIO/Manuals/Software/Buildroot_Linux_Qt_5_14>

Generates: http://localhost:8000/html/PCs/ARM/PX30/AIO/OS_Downloads/index.html#px30-osdownloads.
Screenshot 2024-03-23 at 10 18 22

Does it mean we still need to use relative path in target for everything of image directives?

@iSOLveIT
Copy link
Contributor Author

iSOLveIT commented Mar 23, 2024

Does it mean we still need to use relative path in target for everything of image directives?

The value for the :target: option should be an absolute path. The custom image directive will then resolve it into a relative path relative to the file that called the .. image:: directive.

☑️ Fixed issue in this commit: I have reviewed the code and seen the issue you were asking about. Although the implementation works, it fails as you described above. Let me correct the issue and get back to you.

You can try the example you gave above and it will work.

@printfinn
Copy link
Contributor

You can try the example you gave above and it will work.

It works! I'm working on rewriting all these links in the index pages now.

@iSOLveIT
Copy link
Contributor Author

It works! I'm working on rewriting all these links in the index pages now.

Great 👍🏽. You must merge this PR into the main branch to add the custom image directive and other changes.

@printfinn
Copy link
Contributor

printfinn commented Mar 23, 2024

I've pushed my changes of the other index pages to this branch.

As for the 3D models, I've written a message in the doc. We can merge this PR first, then you can work on the 3D model, and use another PR to fix the 3D models part. Is that OK?

@printfinn printfinn merged commit 461606f into main Mar 23, 2024
1 check passed
@iSOLveIT iSOLveIT deleted the link_fix branch March 23, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants