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

thiersa · 2023-11-06T17:16:24Z

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.

danwos

Thanks for the fix, makes sense 👍

I think there is only one bug in it...

And would it be okay for you to add 1-2 lines to the docs of
simplepdf-theme, that SCSS sources get compiled from static/styles/sources.

danwos · 2023-11-07T07:34:04Z

sphinx_simplepdf/builders/simplepdf.py

+
+        if self.app.config.simplepdf_theme is not None:
+            try:
+                theme_folder = os.path.dirname(import_module(self.app.config.html_theme).__file__)


Shouldn't this be simplepdf_theme instead of html_theme?

j123b567 · 2023-11-09T20:16:36Z

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 · 2023-11-13T12:39:57Z

@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 · 2023-11-13T14:15:42Z

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 · 2023-11-13T15:31:31Z

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 <#91 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADGYQQ6GAKUCFQ4YTEPB2STYEITRTAVCNFSM6AAAAAA673NLCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYGI2DKNRVGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

j123b567 · 2023-11-13T16:27:07Z

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

danwos · 2023-11-13T17:32:01Z

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

danwos · 2023-11-13T19:34:46Z

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.

danwos

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

danwos · 2023-11-13T19:35:41Z

setup.py

@@ -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?

thiersa · 2023-11-13T19:42:00Z

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 <#91 (comment)> : > @@ -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 <#91 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADGYQQ43KESHDM3N2G47ECLYEJZK5AVCNFSM6AAAAAA673NLCGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRYGEYTAOJWGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

thiersa · 2023-11-13T19:55:16Z

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 <#91> and #94 <#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 <#91 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADGYQQ2TG6JTQPXWOBTUDMDYEJY6FAVCNFSM6AAAAAA673NLCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYHA4TONJVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

j123b567 · 2023-11-14T07:54:34Z

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?

danwos · 2023-11-14T08:21:33Z

sphinx_simplepdf/themes/simplepdf_theme/__init__.py


-__version__ = '0.1.0'
+
+__version__ = '0.3.0'


Here also: I would like to stay with 0.1.0 and raise it with an additional PR.

j123b567 · 2023-11-14T10:48:54Z

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 · 2023-11-16T20:22:04Z

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 · 2023-11-17T07:08:38Z

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 · 2023-11-17T07:10:50Z

sphinx_simplepdf/themes/simplepdf_theme/__init__.py

@@ -14,10 +16,33 @@ def get_html_theme_path():
    return cur_dir


+def create_custom_css(app):
+
+    def get_config_var(name, default):


Can we keep the doc_string of the original function here?
I know the code is understandable without it, but it also explains the use case and therefore why it is needed.

danwos · 2023-11-17T07:15:16Z

sphinx_simplepdf/themes/simplepdf_theme/__init__.py

 # See http://www.sphinx-doc.org/en/stable/theming.html#distribute-your-theme-as-a-python-package
 def setup(app):
    app.add_html_theme('simplepdf_theme', path.abspath(path.dirname(__file__)))
-    # app.add_css_file('styles/main.css')
+    app.connect('builder-inited', create_custom_css)


I think with this line we call create_custom_css every time, so for each builder including html.

We should add a check inside create_custom_css if the current builder is simplepdf.

danwos · 2023-11-17T07:25:19Z

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 · 2023-11-17T08:32:39Z

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

thiersa · 2023-11-23T12:36:08Z

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.

lino-alves · 2024-10-24T12:06:54Z

Is there anything preventing this merge and the release of new packages?

We have a custom theme in our environment and these changes here in theory would solve our problem.

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

dcb16ef

danwos requested changes Nov 7, 2023

View reviewed changes

Ad Thiers added 3 commits November 7, 2023 14:13

add ci pipeline

e9d6085

Fix pipeline (from poetry to setuptools+twine)

f79caf3

Bump version: 1.6.0 -> 1.6.1a1

97d1362

Move SASS css generation to the theme

1921ef2

Move CSS generation to theme

d010690

j123b567 mentioned this pull request Nov 13, 2023

Decouple theme and builder #94

Closed

Ad Thiers added 2 commits November 13, 2023 20:15

Cleanup code

50538e6

Cleanup artifacts

94278e9

danwos requested changes Nov 13, 2023

View reviewed changes

Reset version to 1.6.0

4f3d2ce

danwos reviewed Nov 14, 2023

View reviewed changes

Theme version back to 0.1.0

868f54f

danwos requested changes Nov 17, 2023

View reviewed changes

Put docstring back in get_config_var and get_theme_var

9faded6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thiersa commented Nov 6, 2023

danwos left a comment

danwos Nov 7, 2023

j123b567 commented Nov 9, 2023 •

edited

Loading

danwos commented Nov 13, 2023

j123b567 commented Nov 13, 2023

thiersa commented Nov 13, 2023 via email

j123b567 commented Nov 13, 2023

danwos commented Nov 13, 2023

danwos commented Nov 13, 2023

danwos left a comment

danwos Nov 13, 2023

thiersa commented Nov 13, 2023 via email

thiersa commented Nov 13, 2023 via email

j123b567 commented Nov 14, 2023

danwos Nov 14, 2023 •

edited

Loading

j123b567 commented Nov 14, 2023

thiersa commented Nov 16, 2023

danwos commented Nov 17, 2023

danwos Nov 17, 2023

danwos Nov 17, 2023

danwos commented Nov 17, 2023

j123b567 commented Nov 17, 2023

thiersa commented Nov 23, 2023

lino-alves commented Oct 24, 2024


		__version__ = '0.1.0'

		__version__ = '0.3.0'

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

Are you sure you want to change the base?

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

Conversation

thiersa commented Nov 6, 2023

danwos left a comment

Choose a reason for hiding this comment

danwos Nov 7, 2023

Choose a reason for hiding this comment

j123b567 commented Nov 9, 2023 • edited Loading

danwos commented Nov 13, 2023

j123b567 commented Nov 13, 2023

thiersa commented Nov 13, 2023 via email

j123b567 commented Nov 13, 2023

danwos commented Nov 13, 2023

danwos commented Nov 13, 2023

danwos left a comment

Choose a reason for hiding this comment

danwos Nov 13, 2023

Choose a reason for hiding this comment

thiersa commented Nov 13, 2023 via email

thiersa commented Nov 13, 2023 via email

j123b567 commented Nov 14, 2023

danwos Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

j123b567 commented Nov 14, 2023

thiersa commented Nov 16, 2023

danwos commented Nov 17, 2023

danwos Nov 17, 2023

Choose a reason for hiding this comment

danwos Nov 17, 2023

Choose a reason for hiding this comment

danwos commented Nov 17, 2023

j123b567 commented Nov 17, 2023

thiersa commented Nov 23, 2023

lino-alves commented Oct 24, 2024

j123b567 commented Nov 9, 2023 •

edited

Loading

danwos Nov 14, 2023 •

edited

Loading