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

Broken display of AbstractStrings when preferred MIME type istextmime #1113

Closed
jakobjpeters opened this issue May 18, 2024 · 4 comments · Fixed by #1114
Closed

Broken display of AbstractStrings when preferred MIME type istextmime #1113

jakobjpeters opened this issue May 18, 2024 · 4 comments · Fixed by #1114

Comments

@jakobjpeters
Copy link
Contributor

Description

Users implementing show methods for an AbstractString will have a broken display if the preferred MIME type istextmime. A motivating example is in Typstry.jl, which implements show for "image/png" and "image/svg+xml". Both of these methods work in Pluto.jl, but only "image/png" works in IJulia.jl. This is because "image/svg+xml" is preferred over "image/png", but this format assumes that an AbstractString is "raw data". Instead of dispatching to show, it is returned as a String.

I don't really understand this assumption, why is it being used? The only thing I have found so far is the same assumption in repr and presume that's where it came from. A notable difference is that users are able to implement repr to patch in a fix.

Possible Solutions

  • Remove the assumption the an AbstractString is "raw data"
    • I don't know if or what relies on this assumption
  • Export israwtext
    • Users can implement this to override the assumption
    • Requires packages to depend on IJulia.jl
  • Change this line to String(repr(mime, x; context = InlineIOContext(buf)))
    • Users can implement a fix for repr, while preserving the preexisting behavior otherwise because repr uses the same assumption
    • Extra work to convert some values to a String

1. The output of versioninfo()

IJulia v0.24.2

julia> versioninfo()
Julia Version 1.10.3
Commit 0b4590a5507 (2024-04-30 10:59 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, tigerlake)
Threads: 8 default, 0 interactive, 4 GC (on 8 virtual cores)
Environment:
  JULIA_EDITOR = hx

2. How you installed Julia

juliaup

3. A minimal working example (MWE), also known as a minimum reproducible example

julia> using IJulia

julia> struct S <: AbstractString end

julia> Base.ncodeunits(::S) = 0;

julia> Base.iterate(::S) = nothing;

julia> Base.show(io::IO, ::MIME"image/svg+xml", ::S) = print(io, "s");

julia> S()
""

julia> show(stdout, "image/svg+xml", S())
s
julia> IJulia.display_dict(S())
Dict{String, Union{String, JSON.Writer.JSONText}} with 2 entries:
  "text/plain"    => "\"\""
  "image/svg+xml" => ""
@stevengj
Copy link
Member

stevengj commented May 19, 2024

Why would an AbstractString subtype have a method to display as image/svg+xml? That doesn't sound like a string?

The reason for this behavior is that it allows you to do things like display("image/svg+xml", "...raw SVG text as an ordinary string...") and it will show an SVG image from the given string data interpreted as XML. (But we can probably avoid that behavior for the display_dict if needed.)

@jakobjpeters
Copy link
Contributor Author

jakobjpeters commented May 19, 2024

My particular use case is for a TypstString in Typstry.jl, which represents source text for the Typst typsetting system. This text can be compiled to PDF, PNG, and SVG via Typst_jll.jl, so there's a direct correspondence between a TypstString and an SVG. Although the underlying implementation differs, I'd like typst"$ 1 / x $" to be rendered in notebooks in a similar manner to L"$ \frac{1}{x} $".

Thank you for the clarification! Does display_dict determine resulting output options? This comment seems to indicate so. If so, then is the solution to keep the current behavior for calls to limitstringmime except from within display_dict?

@stevengj
Copy link
Member

Yes, it would probably suffice to change the definition of limitstringmime to:

function limitstringmime(mime::MIME, x, forcetext=false, mayberawtext=true)

change this line to

if israwtext(mime, x) && mayberawtext

and change this line to:

display_mimestring(m::MIME, x) = (m, limitstringmime(m, x, false, false))

@stevengj
Copy link
Member

stevengj commented May 20, 2024

Alternatively, an even simpler and probably better solution would be to change this line to

israwtext(m::MIME, x::AbstractString) = !showable(m, x)

so that we only treat x as "raw" MIME data if it doesn't have it's own show method for that MIME type.

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 a pull request may close this issue.

2 participants