Added conditional to support sphinx gallery pre and post v0.11

[simonwm]

Thanks @mgeier for all the great work you put into nbsphinx!
If I understood sphinx-gallery/sphinx-gallery#984 correctly, this merge request would fix spatialaudio#655, and the only thing missing is backwards compatibility with sphinx-gallery pre v0.11.
As the solution is temporary anyway, would a simple conditional not suffice? It worked for my case in environments using sphinx-gallery 0.10.1 and sphinx-gallery 0.11.1 (The results did not look identical, but the behaviour for 0.11.1 was greatly improved...)
I would love to see this issue fixed without having to pin the version of sphinx gallery.

[mgeier]

Thanks, this looks promising, but my main concern is: what happens if sphinx_gallery is not installed?

I don't want to add a hard dependency to sphinx_gallery, because people who create their own CSS won't need it. In that case, it would be good if the new style is used.

Originally, I was concerned about the dependency to packaging, but I guess Sphinx also depends on it, right?

[simonwm]

Thank you for the quick feedback!
I also don't like to add extra dependencies, especially if they are just for a temporary fix to support something which will be deprecated "soon". Therefore I added the imports of importlib (standard library) and packaging (non-standard, but seems to be quasi standard?) only locally in that specific function. While I also think that function local imports are bad style, this helps cleaning up the debris after deprecating the support for sphinx_gallery < 0.11.0.
I don't see the dependency to sphinx_gallery as hard (but that might depend on interpretation), as it is just referenced via reflection features and only the string "sphinx_gallery" appears in the code. Nevertheless the previous patch would have thrown an importlib.metadata.PackageNotFoundError if it was not available. The dependency to packaging is a step harder, but still function local and only evaluated at runtime, so if the function is never called (should be the case if no gallery is used?), the error would not have appeared.
Nevertheless, I now framed the potentially throwing pieces of the code with a specific try-except using the sphinx_gallery>=0.11 behaviour as fallback.
BTW: If sphinx_gallery is not installed, the div classes "sphx-glr-thumbnails" and "sphx-glr-thumbnails" would not have worked anyway, right? So implicitly requiring the existence of sphinx_gallery if a gallery is used might be acceptable - but I guess the browser would still have shown something and that something could be made nice enough with some custom css...

[mgeier]

Thanks for the update!

I agree, local imports make sense in this case.

I think the packaging dependency is not a problem, since Sphinx itself depends on it, so we can assume that it's available. But checking for an import error doesn't hurt either.

BTW: If sphinx_gallery is not installed, the div classes "sphx-glr-thumbnails" and "sphx-glr-thumbnails" would not have worked anyway, right?

Well I would assume that if somebody doesn't want to use the CSS from sphinx_gallery, they can still define their own custom CSS to style the sphx-glr-* classes however they want. If anybody has done that, though, their gallery will break when we switch to the new HTML.

---

FYI, I would like to wait a bit with merging this, because there still seems to be some activity in sphinx-gallery/sphinx-gallery#998, and maybe the HTML is changed again.

But once that's clarified, it would be great if you could keep the line lengths to 79 characters. If you don't have time for that, I can also do it for you.

---

As an alternative, I'm playing with the thought of dropping the whole sphinx_gallery compatibility and providing nbsphinx-specific CSS (which I think wouldn't be very much). This way we would not be affected by any future changes in sphinx_gallery and the upgrade for users would be seamless (they would just unnecessarily have sphinx_gallery in their extensions).

What do you think about that?

[simonwm]

Thank you for your explanations!

If someone defined their own CSS to style the sphx-glr-* classes, I guess they had to dig deep into the code to find out what they have to style - or use the developer features of any browser - so acceptable effort I guess... Anyway, I assume it is not a recommended way to use nbsphinx, so it should be ok if they would have to redo their CSS. Backwards compatibility for hacks cannot be expected from a project...

On the other hand, emitting custom html to mimick the architecture of sphinx_gallery html and depending on their CSS feels also pretty hacky - and it fell on my feet... (; From that perspective, supplying custom CSS to make nbsphinx independent of sphinx_gallery would be a clean path forward.

But somehow it also feels un-pythonic to have two separate python packages independently providing very similar functionality (galleries of python scripts and notebooks)... Maybe some reliable common code base for synergies or even a fusion of both projects might also be a long-term perspective? But I don't have that kind of insight into either project to provide good advice here.

Anyway, in case this PR is not being made obsolete by other developments, I adjusted the line lengths to good old Fortran style.

[simonwm]

Just checking: As the activity in sphinx-gallery/sphinx-gallery#998 is not really overwhelming since a while, what are your plans about this PR here? If no changes are expected for a while, I guess I'll just pin the version of sphinx gallery to 0.10.1 for my project - not my favorite solution though...

[mgeier]

Thanks for keeping an eye on this!

I stated my opinion in sphinx-gallery/sphinx-gallery#1005 (comment), and there I mentioned that I'm not in a hurry. That was nearly two months ago. I didn't see any further activity.

what are your plans about this PR here?

I wanted to wait if the sphinx-gallery people can provide better HTML.

As it is right now, I'm not willing to accept it.
If they don't provide better HTML, I would prefer cutting any dependencies and creating independent HTML (and CSS) for nbsphinx.

If someone defined their own CSS to style the sphx-glr-* classes [...]

Those people (if they exist), will have to change their CSS anyway, since sphinx-gallery re-used their old CSS classes and gave them a completely different meaning.

So for those people, it doesn't matter if they switch to the new (and incompatible) style of sphinx-gallery or if they switch to a potential new (and incompatible) nbsphinx style.

But somehow it also feels un-pythonic to have two separate python packages independently providing very similar functionality (galleries of python scripts and notebooks)...

Yeah, that's why I originally wanted to replicate the HTML of sphinx-gallery to be able to re-use their CSS.

Maybe some reliable common code base for synergies [...]

Yeah, a "gallery" Sphinx directive would be great. It would make sense for the sphinx-gallery project to provide such a thing, but they don't.

There's https://github.com/executablebooks/sphinx-design/, but for this use case it seems a bit heavy-weight to me.

... or even a fusion of both projects might also be a long-term perspective?

I don't think that's feasible, because the technology for analyzing and running the notebooks is quite different.

And I think it is actually good to have multiple independent projects who use different approaches to do similar things.

There is also MyST-NB, which again uses different techniques to implement similar things (but it currently has no gallery feature, see executablebooks/MyST-NB#396).

I guess I'll just pin the version of sphinx gallery to 0.10.1 for my project - not my favorite solution though...

Yeah, I'd recommend that for now.

But I'm not sure how to proceed.
Should I keep waiting for sphinx-gallery to come up with acceptable HTML?
Should I wait for a third-party to come up with a generic "gallery" directive?
Should I just come up with my own few lines of HTML and CSS and be done with it?

What do you think?

[simonwm]

Thanks for the quick and deep answer!
As it does not look like the sphinx-gallery team has much interest in or time for fixing this issue, I think I would remove the dependency and save myself any issues with future changes in their plumbing. Especially if it is done "quickly" as it sounds, I don't see much advantage in keeping the dependency.
If you have any hope that a third party will come up with a generic "gallery" directive in the next years, you can still revisit this later.
But "being done with it" is a great value in itself, especially if you have a lot of open ends in other projects, too.
In that spirit I will pin my sphinx-gallery version for now. But I'll try to follow any updates here and remove the dependency entirely in case you decide to remove it.
Thanks a lot for your advice!

[mgeier]

In case you are interested, I did some experiments in the branch https://github.com/mgeier/nbsphinx/tree/new-gallery-style

Here's a rendered HTML page: https://output.circle-artifacts.com/output/job/5f05281c-5021-4c3c-ac1c-438970164658/artifacts/0/html/subdir/gallery.html

I think for this change it makes sense to finally split the CSS into separate files, see spatialaudio#667.
So I think I'll do that first and then create a separate CSS file for galleries.

[simonwm]

Thanks for sharing! This new design would definitely work.
I guess that it would improve the experience of screening through a large gallery if the titles were all aligned vertically. In the remaining space the img would be centered vertically.
My CSS is a little rusty, so I could not come up with an elegant solution... What works (at least for the rendered example by modifying the HTML manually...) was to wrap the img tag with an extra div (to give the img something to center vertically in), add some CSS to the img for vertical centering (position: relative;top: 50%;transform: translateY(-50%);), and most importantly add some CSS to the wrapping div to fix its size to make the title start at the same place across the grid (height: calc(100% - 30pt);). Alltogether this becomes e.g. :

<a class="reference internal nbsphinx-thumbnail" href="../gallery/no-thumbnail.html">
	<div style="height: calc(100% - 30pt);">
		<img alt="" src="../_static/nbsphinx-no-thumbnail.svg" style="position: relative;top: 50%;transform: translateY(-50%);">
	</div>
	<div>No Thumbnail Available</div>
</a>

This is what it looks like.

It is far from perfect, as the 30pt just account for two line titles with the third line growing out of the box...
A better alignment might be to put the images and titles in two overlapping grids or to have a single grid with alternating cell heights - if any of these is possible...

[mgeier]

Thanks a lot for your suggestions!

it would improve the experience of screening through a large gallery if the titles were all aligned vertically.

Yeah, I think I agree with that.

I've created a new commit to try that: 423067a

Rendered: https://output.circle-artifacts.com/output/job/8537dd9a-78c3-43ca-879d-4c41932c080f/artifacts/0/html/subdir/gallery.html

In the remaining space the img would be centered vertically.

I'm not sure if I agree with that.
I have the feeling that top-alignment looks more deliberate because three margins are identical.

But maybe I'm wrong, I'm still open for suggestions!

A better alignment might be to put the images and titles in two overlapping grids

I've used flex within the grid cells, this looked the simplest to me.

[simonwm]

Thanks for incorporating this, its looking good! And you are right, the top-alignment is cleaner.
The flex solution seems elegant enough.
So... I'm all out of suggestions and looking forward to the release (:

[mgeier]

Thanks for the feedback!

It'll take some time util I'll release this, though.

I first want to make a release with another gallery-related feature (spatialaudio#696) and a few HTML theme updates.

After that I'll start working on spatialaudio#667.
I don't strictly need that for the CSS, but I do need it for the "no thumbnail" images.
And once that is done, I'll finally be able to properly implement the custom gallery style.

[mgeier]

OK, this went faster than I thought ... I've already made a new release and I've created a PR: spatialaudio#706

Feel free to suggest some more CSS fine-tuning there!

[mgeier]

I have merged spatialaudio#706, so this is not needed anymore.

[simonwm]

Awesome thanks a lot! And sorry for not giving extra feedback for the CSS, but it looks great to me, anyway!