Theme fix - use style sources from theme instead from built-in

[thiersa]

In the main branch the code that compiles SCCS source files into CSS always uses the SCCS sources from the builtin theme ('simplepdf_theme').

This fix gets the SCCS sources from the theme package itself.

[j123b567]

It is something that I recently needed but after some thought, this particular implementation makes a very strong assumption that all user themes will always have static/styles/sources directory with scss which may not be true.

The user can use a custom theme and for example add app.add_css_file('custom.css') to override just some aspects of the original style of the theme. This use-case will break completely with this PR.

[danwos]

@j123b567, you are right, the folder structure of static/styles/sources is given for the theme. But for me all the other scenarios are still possible, using html_css or app.add_css_file().

But for sure, the PR needs some love to document the "forced" theme structure in the docs.

[j123b567]

From this point, in all custom themes, you will be forced to have at least empty static/styles/sources/main.scss and then you can use app.add_css_file()

IMHO the main problem is that the theme should be decoupled from the builder in the first place and not make it worse.

It should be possible to connect to the event "builder-inited", "env-updated", or other events and compile the theme in the callback and not in the builder

Did you try this possibility in the past and it was a dead end?

[thiersa]

I agree with your feedback that actually the functionality of creating the CSS is a responsibility of the theme itself. Last weekend I spent time of refactoring the code to get it into the theme. I got it to work. The trick indeed was to connect it to the builder-inited event. Right now I'm commuting, but will check in the new code after brushing it up. Ad

Op ma 13 nov. 2023 15:15 schreef Jan Breuer @.***>:

From this point, in all custom themes, you will be forced to have at least empty static/styles/sources/main.scss and then you can use app.add_css_file()

IMHO the main problem is that the theme should be decoupled from the builder in the first place and not make it worse.

It should be possible to connect to the event "builder-inited", "env-updated", or other events and compile the theme in the callback and not in the builder

Did you try this possibility in the past and it was a dead end?

— Reply to this email directly, view it on GitHub https://github.com/useblocks/sphinx-simplepdf/pull/91#issuecomment-1808245654, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGYQQ6GAKUCFQ4YTEPB2STYEITRTAVCNFSM6AAAAAA673NLCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYGI2DKNRVGQ . You are receiving this because you authored the thread.Message ID: @.***>

[j123b567]

@thiersa ou, I just implemented it also in #94

[danwos]

Just reviewed #94 and it looks good to me. If nobody has any concerns with #94, I will merge it soon.

[danwos]

Hehe, now we have two similar solutions #91 and #94 :) Good work for both of you, any chance you two build a consense which one to take :) For sure both of you can be added to the AUTHORS, if you like.

[thiersa]

Sure i will.

Op ma 13 nov 2023 om 20:38 schreef Daniel Woste @.***>:

@.**** requested changes on this pull request.

Thanks for the PR 👍 Looks good, only one minor (very subjective) finding.

In setup.py https://github.com/useblocks/sphinx-simplepdf/pull/91#discussion_r1391601230 :

@@ -11,7 +11,7 @@

setup( name='sphinx-simplepdf',

• version='1.6.0',
• version='1.6.2a1',

I like to raise versions in separate PR, so may I ask you to remove this?

— Reply to this email directly, view it on GitHub https://github.com/useblocks/sphinx-simplepdf/pull/91#pullrequestreview-1728110961, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGYQQ43KESHDM3N2G47ECLYEJZK5AVCNFSM6AAAAAA673NLCGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRYGEYTAOJWGE . You are receiving this because you were mentioned.Message ID: @.***>

[thiersa]

Indeed very similar solutions, which is great, I think. Completely in the spirit of Python. The major difference is the way the SASS functions are generated. Jan's version uses partial functions, whereas my solution uses closures. A matter of taste, I guess. Both are good solutions.

Op ma 13 nov 2023 om 20:34 schreef Daniel Woste @.***>:

Hehe, now we have two similar solutions #91 https://github.com/useblocks/sphinx-simplepdf/pull/91 and #94 https://github.com/useblocks/sphinx-simplepdf/pull/94 :) Good work for both of you, any chance you two build a consense which one to take :) For sure both of you can be added to the AUTHORS, if you like.

— Reply to this email directly, view it on GitHub https://github.com/useblocks/sphinx-simplepdf/pull/91#issuecomment-1808897552, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGYQQ2TG6JTQPXWOBTUDMDYEJY6FAVCNFSM6AAAAAA673NLCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYHA4TONJVGI . You are receiving this because you were mentioned.Message ID: @.***>

[j123b567]

I didn't try to understand deeply get_config_var and get_theme_var so this implementation is much simpler and should be preferred.

I just don't like changing version information inside PRs like this so __version__ inside theme and inside extension should be changed only by maintainers. Also, shouldn't the version of the theme match the version of the extension?

[j123b567]

There is another issue - simplepdf_vars and simplepdf_theme_options are used only in the theme so if the registration of these variables (app.add_config_value) should be also moved to the theme setup.

This is not a problem of the sphinx-simplepdf itself but of any derived theme. E.g. if somebody copies or derive the theme, it is still not self-contained. So other targets than simplepdf will fail because of missing app.config.simplepdf_vars. This move will just pronounce the fact, that anybody copying the theme should also handle these config variables.

This PR is all about using a custom theme so we should probably handle this also.

[thiersa]

Just changed back the theme version to 0.1.0 as requested. Another thing, reflecting on Jan's comment about the dependency of the simplepdf_theme_options. This setting is only used to override the settings of html_theme_options. Most (all?) of the templating machinery on the backend is referring to app.config.html_theme_options. So why not just use this:

• the simplepdf builder replaces the html_theme_options by the simplepdf_theme_options (which is already the case).
• the themes (both built-in and any derived theme) use app.config.html_theme_options instead of app.config.simplepdf_theme_options
• IMHO, the information in simplepdf_vars can just as well be contained in simplepdf_theme_option. After all it is information to tweak the theme.

This way themes can be created just like any other html theme, but they do not interfere with a theme that is used for html generation. With the same conf.py you can generate html and pdf. And you can still use the elegant mechanism with sass.compile to parameterize the theme.

Your thoughts?

[danwos]

Generally, it is a good idea to use html_theme_options internally and overwrite it initially with simplepdf_theme_options. However, I can roughly remember trying this at the beginning of this project and it failed somehow. Maybe because of the caching mechanisms of Sphinx or the way incremental build works. But I'm not 100% sure. May be worth testing it again.

Regarding simplepdf_vars, I would keep it as it is and not integrate it into simplepdf_theme_options. Mostly because I don't see any benefit and it would be a breaking change.

[danwos]

There is another issue - simplepdf_vars and simplepdf_theme_options are used only in the theme so if the registration of these variables (app.add_config_value) should be also moved to the theme setup.

I like to have all sphinx registration options in one place. Easier to maintain and maybe in the future the builder itself needs to have access to it as well.

Also I think a simplepdf theme is always used together with the simplepdf builder/extension. So if we move it to the theme, there is then also a chance that users are missing it.

I would solve this problem with better documentation. Maybe a short chapter to create a custom theme...

[j123b567]

@danwos sounds good so no need to do anything special now.

[thiersa]

Just added back the docstrings in the two config functions in the theme. Good point to keep simplepdf_vars to prevent a breaking change. As a theme developer you still have the choice to use it or not.

As far as overwriting the html_theme_options is concerned; I haven't experienced any issues, none of mine regressions failed. Maybe @danwos can recall a failing edge-case.