Splash icon indicating copy to clipboard operation
Splash copied to clipboard

Added SwiftUITextOutputFormat to output a Text struct

Open andreweades opened this issue 5 years ago • 14 comments

This OutputFormat outputs a Text struct that can be used in a SwiftUI view. It ignores the font as you normally would apply the font with a .font modifier in SwiftUI. Either we should convert the theme's font to a SwiftUI.Font and and use that or leave it to the user to style fonts separately from highlighting syntax colours. I favour the later as it fits better with the model of SwiftUI and leaves SwiftUITextOutputFormat to be only concerned with constructing the highlighted text but this might be confusing as it ignores the theme's font. However, the attributed text string is different to the font and they could be considered separate and treated separately as SwiftUi pushes us in that direction and an additional .font modifier would override any theme font anyway. Maybe this should really be a view modifier?

andreweades avatar Jan 03 '20 12:01 andreweades

Apologies. I had some CI build issues as I was rushing like an idiot and just uploaded the file thinking it would work. It obviously builds on my Catalina set up for SwiftUI development. I'm getting a 404 so I can't tell what the other fail is.

andreweades avatar Jan 03 '20 13:01 andreweades

Hey @andreweades, thanks for contributing to Splash. I'll try to review this as soon as possible. Just to help you with the CI issues before then, rather than wrapping the entire implementation in conditional compilation, I suggest using the @available attribute on the new format. That should give you a cleaner implementation as well. If you want to verify your changes locally, just build Splash for the iOS simulator for an iOS version lower than 12, and you should see the same errors as on macOS Mojave.

JohnSundell avatar Jan 03 '20 13:01 JohnSundell

Oh. That’s why. I’ll do that next time.

I’m not expecting this to go straight in BTW. There are a couple of questions re fonts and whether it should really be a view modifier anyway.

On 3 Jan 2020, at 13:11, John Sundell [email protected] wrote:

Hey @andreweades https://github.com/andreweades, thanks for contributing to Splash. I'll try to review this as soon as possible. Just to help you with the CI issues before then, rather than wrapping the entire implementation in conditional compilation, I suggest using the @available attribute on the new format. That should give you a cleaner implementation as well. If you want to verify your changes locally, just build Splash for the iOS simulator for an iOS version lower than 12, and you should see the same errors as on macOS Mojave.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JohnSundell/Splash/pull/92?email_source=notifications&email_token=ALOJPAMHMGD7SEE7ICPHWS3Q342RPA5CNFSM4KCNBV4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIBC3GA#issuecomment-570568088, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALOJPAN6DNXTZ7DICPC3IN3Q342RPANCNFSM4KCNBV4A.

andreweades avatar Jan 03 '20 13:01 andreweades

Would love for @duemunk to take a look at this, since I know that he's been working on something similar 🙂 Will also need this to be covered with tests before it can be merged in, just a heads-up. But I'll try to review this within the next few days.

JohnSundell avatar Jan 03 '20 13:01 JohnSundell

Looks really great!! Much simpler than my implementation. Particularly this is super clever:

texts.reduce(Text(""), +)

Great when SwiftUI can surprise in positive ways like that.

duemunk avatar Jan 03 '20 13:01 duemunk

The implementation doesn't support the theme font.

That's right. I had some questions regarding font support as the SwiftUI way would be to use the .font view modifier. We could internalise this in the SwiftUITextOutputFormat or leave it to the user. I noted in my PR that this would be confusing so needs more thought.

Options:

  1. internalise font support and maintain the API
  2. separate colour theme from font selection as SwiftUI would encourage

But then I thought maybe it should just be a view modifier that can be applied to any Text and you would get something like the following:

Text(stringOfCode).syntaxHighlight(.presentation).font(.system(size: 12, design: .monospaced))

which leaves the font selection to the existing modifier.

andreweades avatar Jan 03 '20 15:01 andreweades

After thinking about this a bit more... Maybe do 1 and make a syntaxHighlight view modifier that highlights and applies a font modifier.

andreweades avatar Jan 03 '20 16:01 andreweades

My use case is demoed here.

I highlight the main text and the minimap using the same theme but with a different font applied to each.

andreweades avatar Jan 03 '20 21:01 andreweades

I think using the theme's font is a good approach. The extensions are a nice to have in my opinion.

duemunk avatar Jan 12 '20 15:01 duemunk

I will look into this again when I have some spare time. I will have to schedule some Open Source hours.

andreweades avatar Feb 12 '20 21:02 andreweades

@andreweades Any plans to get this to completion?

Jeehut avatar May 05 '22 14:05 Jeehut

@andreweades Any plans to get this to completion?

I think SwiftUI support for AttributedString supplants this.

andreweades avatar May 05 '22 14:05 andreweades

@andreweades But how to change the design of the font? I'd like to use .system(design: .monospaced), but this didn't work:

Text(
  AttributedString(
    SyntaxHighlighter(format: AttributedStringOutputFormat(theme: .sundellsColors(withFont: .init(size: 13))))
      .highlight(viewStore.codePreview)
    )
)
.font(.system(size: 13, design: .monospaced)) // <-- `.monospaced` gets ignored

Or is there a simple way to provide a SwiftUI.Font to the withFont which expects a Splash.Font?

Jeehut avatar May 05 '22 15:05 Jeehut

I’m pretty sure that if you use my fork it will work as desired as that is how I used it. I was using it for a minimap in a code viewer with a custom font.

On 5 May 2022, at 16:41, Cihat Gündüz @.***> wrote:

@andreweades https://github.com/andreweades But how to change the design of the font? I'd like to use .system(design: .monospaced), but this didn't work:

Text( AttributedString( SyntaxHighlighter(format: AttributedStringOutputFormat(theme: .sundellsColors(withFont: .init(size: 13)))) .highlight(viewStore.codePreview) ) ) .font(.system(size: 13, design: .monospaced)) Or is there a simple way to provide a SwiftUI.Font to the withFont which expects a Splash.Font?

— Reply to this email directly, view it on GitHub https://github.com/JohnSundell/Splash/pull/92#issuecomment-1118717513, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALOJPALNRVT7E5D6X6ZS2BTVIPTZXANCNFSM4KCNBV4A. You are receiving this because you were mentioned.

andreweades avatar May 05 '22 16:05 andreweades