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

Set default value of raw.theme to auto, and allow setting raw.theme to auto #4186

Merged
merged 1 commit into from
May 24, 2024

Conversation

Coekjan
Copy link
Contributor

@Coekjan Coekjan commented May 18, 2024

This pr closes #4182 .

I found out the bug in parse_theme. The function parse_theme always wants to parse the argument theme into EcoString, which raises error when meeting none.

As #4186 (review) , we set default value of raw.theme to auto, and allow setting raw.theme to auto. To implement it, I redesigned the function signature (return value type):

  1. Err(...) indicates argument theme found but error met when casting it into Smart<EcoString>;
  2. Ok((None, None)) indicates argument theme not found;
  3. Ok((Some(Smart::Auto), None)) indicates argument theme found, and it is auto;
  4. Ok((Some(Smart::Custom(path)), Some(data))) indicates argument theme found, and it is str (path to the theme file).

Copy link
Member

@laurmaedje laurmaedje left a comment

Choose a reason for hiding this comment

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

I think theme: none doesn't really make sense like this, since it's still a theme, just the default one. We should probably switch the default to auto instead.

We could also support theme: none to disable highlighting. Not sure whether we need that.

@Coekjan
Copy link
Contributor Author

Coekjan commented May 23, 2024

I think theme: none doesn't really make sense like this, since it's still a theme, just the default one. We should probably switch the default to auto instead.

Agree.

We could also support theme: none to disable highlighting. Not sure whether we need that.

Maybe, in this PR, we can switch to auto, and leave whether we need none for future discussion.

…eme` to `auto`

NOTE: `raw.theme` is no longer allowed to be `none`
@Coekjan Coekjan changed the title Fix parse_theme when argument theme is none Set default value of raw.theme to auto, and allow setting raw.theme to auto May 23, 2024
@Coekjan Coekjan requested a review from laurmaedje May 23, 2024 12:03
@laurmaedje laurmaedje added this pull request to the merge queue May 24, 2024
@laurmaedje
Copy link
Member

Thanks!

Merged via the queue into typst:main with commit 374b82f May 24, 2024
6 checks passed
@Coekjan Coekjan deleted the raw-none-theme branch May 25, 2024 04:44
Enter-tainer pushed a commit to Enter-tainer/typst that referenced this pull request May 26, 2024
PgBiel added a commit to tulio240/typst that referenced this pull request May 30, 2024
Refactor frame metadata into tags (typst#4212)


Require `Send` and `Sync` for worlds (typst#4219)


Optimize counters and state (typst#4223)


Add `windows` method to array (typst#4136)

Improve `CITATION.cff` file (typst#4201)


Fix equation resizing when adding the equation number (typst#4179)


`layout` documentation improvements (typst#4196)

Allow somewhat arbitrary characters as `mat`, `vec` and `cases` `delim` (typst#4211)


Do layout short-circuit in flow instead of realization (typst#4231)


Split `BitSet` into two types and make it a bit nicer (typst#4249)


Set default value of `raw.theme` to `auto`, and allow setting `raw.theme` to `auto` (typst#4186)


Extended cargo installation instructions (typst#4168)

Hint for language-region pair on `text.lang` (typst#4183)


Improve macro docs (+ Native*Data docs) (typst#4240)


Rephrase the sentence on variable scope in Scripting documentation (typst#4250)


Refactor `Capable::vtable` to return `Option<NonNull<()>>` (typst#4252)


Nicer test helper CSS (typst#4269)


Trim weak spacing at line start/end in paragraph layout (typst#4087)


Add ability to choose between minified and pretty-printed JSON (typst#4161)


Refactor PDF export (typst#4154)

Reorder syntax kinds (typst#4287)


Fix figure centering (typst#4276)

Fix `Default` impls for AST nodes (typst#4288)


Bump libc to v0.2.155 (typst#4268)


Bump time dependency (typst#4294)
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.

Cannot set raw theme to none
2 participants