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.

Sign up for GitHub

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

Jump to bottom

fix: readerer get theme dir path bug #1405

Merged
merged 1 commit into from
Dec 28, 2020
Merged

fix: readerer get theme dir path bug #1405

merged 1 commit into from
Dec 28, 2020

Conversation

francis-du
Copy link
Contributor

Fix #1404

Now, it works on my build in local.

@ehuss
Copy link
Contributor

ehuss commented Dec 22, 2020

Thanks for the PR! Can you explain more about what the problem is? This seems to change the meaning of the theme configuration variable, which I don't think we can change. It is documented as a directory relative to the root of the book.

@francis-du
Copy link
Contributor Author

francis-du commented Dec 23, 2020 •
edited
Loading

Assuming the root path is /home/book.

let theme_dir = match html_config.theme {
    Some(ref theme) => theme.to_path_buf(),
    None => ctx.root.join("theme"),
};

let theme = theme::Theme::new(theme_dir);
pub fn new<P: AsRef<Path>>(theme_dir: P) -> Self {
    let theme_dir = theme_dir.as_ref();
    let mut theme = Theme::default();

    println!("final theme dir => {}",&theme_dir.display());

    // If the theme directory doesn't exist there's no point continuing...
    if !theme_dir.exists() || !theme_dir.is_dir() {
        return theme; 
    }

  ...
}

If the theme directory is configured.

[output.html]
theme="theme"
default-theme = "ayu"

Beacuse of theme_dir does't join root path, the directory cannot be found here.
and can't load any files under /home/book/theme.

So I think here need to join root path,and the theme must be under theme.
In this way, if I have a custom theme called grogu, I can directly configure
theme name.eg: theme="Grogu". So the theme can also be introduced as a submodule from git.

@ehuss
Copy link
Contributor

ehuss commented Dec 23, 2020

If you have a custom theme, it should be in a directory next to the src directory. For example, theme = "my-theme", in a directory /home/book, then the theme would be in the /home/book/my-theme/ directory. If you want to use a subdirectory, then the configuration would be theme = "theme/my-theme".

@francis-du
Copy link
Contributor Author

francis-du commented Dec 24, 2020 •
edited
Loading

If you have a custom theme, it should be in a directory next to the src directory. For example, theme = "my-theme", in a directory /home/book, then the theme would be in the /home/book/my-theme/ directory. If you want to use a subdirectory, then the configuration would be theme = "theme/my-theme".

If so, theme_dir is a relative path, it can't be found without join root_dir.You can try it.

Here, !theme_dir.exists() = true, and theme is still Theme::default();

// If the theme directory doesn't exist there's no point continuing...
if !theme_dir.exists() || !theme_dir.is_dir() {
    return theme;
}

@ehuss
Copy link
Contributor

ehuss commented Dec 26, 2020

Can you explain in more detail what is not working for you? The following commands should create a book with a custom theme:

mdbook init foo
cd foo
cat << EOF >> book.toml
[output.html]
theme = "my-theme"
EOF
mkdir my-theme
echo "<html><body>{{{ content }}}</body></html>" > my-theme/index.hbs
mdbook build --open

@francis-du
Copy link
Contributor Author

francis-du commented Dec 26, 2020 •
edited
Loading

When I configure the theme , then my custom style is not loaded .

image

The reason is the configured theme_dir is a relative path, it can't be found when HtmlHandlebars reads custom theme files.So I join the root path for it, it works.

image

@ehuss
Copy link
Contributor

ehuss commented Dec 26, 2020

Are you maybe running mdbook from a different directory and passing in the book directory?

In that case, it does seem to not work correctly. I think it would be reasonable to fix that, but this PR would need to change to remove the .join("theme"). I would also remove the call to create_dir_all, as I think it would be preferable to error out if it is misconfigured. Finally, the call to to_path_buf doesn't seem to be necessary.

ehuss
ehuss approved these changes Dec 28, 2020
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks like there were some issues with a git merge, so I squashed the commits. Generally using a separate branch, and rebasing will avoid those kinds of issues.

@ehuss ehuss merged commit a64a7b7 into rust-lang:master Dec 28, 2020
@francis-du
Copy link
Contributor Author

@ehuss Thanks .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Renderer] - When the theme parameter is configured, hbs_renderer cannot get theme dir.
2 participants
@francis-du @ehuss