HugoGranstrom / nimiSlides

A Reveal.js theme for nimib for making slideshow presentations in Nim
https://hugogranstrom.com/nimiSlides
MIT License
80 stars 6 forks source link

Migrate to nimib 0.3 - BlockMaker #5

Closed HugoGranstrom closed 2 years ago

HugoGranstrom commented 2 years ago

The hardest parts are migrated and are less hacky now. fragmentCore, fragmentStart and fragmentEnd are still a bit hacky but I think this is necessary for it to work. ~There are still a lot of nbText: *html code* everywhere but they aren't important right now as they work fine. Also has some problems with nim-mustache not liking HTML and turning < into &lt; for example. Will have to look into if we can escape the tags somehow. nbText goes through the markdown processor which must somehow escape it, because it works.~ Found it! I have seen you prefixing fields with & in the templates but didn't know what they did. Seems like it's escaping the text :D

Overall it was a quite smooth transition and implementing animateCode was a breeze by reusing the functionality of nbCode :D

@pietroppeter Would appreciate your eyes on this when you have the time. Especially if I made something more convoluted than it had to be done :D

TODO:

pietroppeter commented 2 years ago

Found it! I have seen you prefixing fields with & in the templates but didn't know what they did. Seems like it's escaping the text :D

yep indeed that is the case. In mustache you can use & or a triple mustache to escape the content. I started at some point using exclusively the & since I think it make it more easy to spot and it is a very important detail (might need to highlight it somewhere in nimib docs)

pietroppeter commented 2 years ago

~Skip the initReveal call entirely and just dump everything in global?~ Must keep this adding {.experimental: "flexibleOptionalParams".} to the user file

just saw this and I have a review comment that was suggesting to dump everything in global (since with last PR I did that for nimib). Does this mean that current devel for nimib will not work for either 1.4 or 1.6 or that it has some kind of issues?

pietroppeter commented 2 years ago

the {.inject.} for the variable shouldn't be needed

I was trying to understand this comment to learn something more about templates but I was not able to find the correct commit that makes it clear. Also rather weirdly, comments in this conversation start duplicating themselves at some point...

HugoGranstrom commented 2 years ago

I was trying to understand this comment to learn something more about templates but I was not able to find the correct commit that makes it clear. Also rather weirdly, comments in this conversation start duplicating themselves at some point...

Haha yes, it is really messy 😅 I had to change it multiple times because I hadn't taken edge cases into consideration so there isn't a single commit 🙃 I don't understand it either tbh, all I know is that it doesn't work without it 🤷

HugoGranstrom commented 2 years ago

@pietroppeter I've found the problem I'm getting with -d:nimibPreviewCodeAsInSource now. It turns out to be related to rawBlock here: https://github.com/HugoGranstrom/nimib-reveal/blob/1b2b7d99a20042f869760df8a41e0091d26217a7/nimiSlides.nim#L132

When nimib is creating the block, it now always parses the code of the block if I understand it correctly? In this case as the call to rawBlock is made inside nimiSlide itself, it will try to look at line 132 as expected. Problem is that getCodeAsInSource doesn't know that and tries to find line 132 in the user's file instead. Hence my errors previously.

Either we become smarter about which files to look in, or we become selective about which blocks have to actually read the code. What do you think?

pietroppeter commented 2 years ago

@pietroppeter I've found the problem I'm getting with -d:nimibPreviewCodeAsInSource now. It turns out to be related to rawBlock here:

https://github.com/HugoGranstrom/nimib-reveal/blob/1b2b7d99a20042f869760df8a41e0091d26217a7/nimiSlides.nim#L132

When nimib is creating the block, it now always parses the code of the block if I understand it correctly? In this case as the call to rawBlock is made inside nimiSlide itself, it will try to look at line 132 as expected. Problem is that getCodeAsInSource doesn't know that and tries to find line 132 in the user's file instead. Hence my errors previously.

Either we become smarter about which files to look in, or we become selective about which blocks have to actually read the code. What do you think?

this is a tricky one. From this use case, it seems that we might need to build blocks that do not automatically read the code (either from source or from AST) so we should provide a new newNbBlock mechanism which does not do any code processing (name to be found).

For the future instead I realized that the use you make it inside slide is due to the fact that we do not have (yet) a container block and when that is materialized those begin and end of section should be likely done as template for the container block.

pietroppeter commented 2 years ago

Saw your commits on local reveal and wanted you to point out something that might be interesting for this and possibly other aspects of configuration. Nimib handles its (very basic currently contains only srcDir and homeDir) configuration file nimib.toml with the library from status toml_serialization (see nimib/config.nim). The setup in a way that other themes can take advantage of this configuration file.

for example you could have a configuration file with a [nimislides] section like so:

[nimislides]
localReveal = "assets"

inside your theme function you would add something like:

let nimislidesCfg = Toml.decode(nb.rawCfg, NimislidesCfg, "[nimislides]")

if nimislidesCfg.localReveal != "":
  nb.useLocalReveal(nimislidesCfg.localReveal)

where you would have defined a type:

type
  NimislidesCfg* = object
    localReveal*: string

details of the code above might not be correct but hopefully the idea is conveyed.

I am working for nimibook to start using this config feature.

HugoGranstrom commented 2 years ago

@pietroppeter Ohh 😮 that looks really nice, thanks :D And it makes sense to have common settings like this in one place.

HugoGranstrom commented 2 years ago

@pietroppeter Do nbText need readCode = true? I left it so because you had old tests that expected it. I suspect my failed attempt at fragmentList is because of that exactly, that I use nbText inside the template and it in turn tries to read that code instead

pietroppeter commented 2 years ago

that is a good question. the reason why I was expecting nbText to readCode is that in general that was the default before readCodeAsInSource. With readCodeAsInSource, reading code might cause issues when used in a template so, yeah, either we find a way to make readCodeAsInSource to work also inside templates (no idea how at the moment and what would be the issue) or we might want to limit reading code to the essentials. Or... mmh, I am getting an idea where we might be able to decide to disable readCode automatically when we are not in the mainfile (I think it could be doable by checking file where is instantiated against what nb object thinks about that)...

Anyway, I can see how that could have caused you the issue in the past, is it something that you have solved now in this codebase or you are asking because you would want to maybe see a change?

HugoGranstrom commented 2 years ago

OK yes that makes sense. Your idea of checking if the files match sounds like it could work quite well 👍 The only problem would be if you compose multiple blocks in the same file, then the body of the outermost block would contain code originating from multiple files.

Anyway, I can see how that could have caused you the issue in the past, is it something that you have solved now in this codebase or you are asking because you would want to maybe see a change?

I have templates that I would like to use nbText in (e.g pass in seq[string] and run nbText on each of them wrapped in a fragment, see recent fragmentList commit). I could of course solve this for the moment by making my of nbTextNoReadCode, but in the future and for future theme-makers it has to be addressed. And changing nbText to not read the code is a simple fix until we try out your more ambitious idea of checking if the files are the same.

pietroppeter commented 2 years ago

ok you have conviced me it is something we need to address. Idea could be to make indeed readCode false by default in nbText (another option would be to provide with a nbText(readCode=false): "bla bla bla" api). I added this as a todo item for nimib0.3: https://github.com/pietroppeter/nimib/pull/81 I will update also here when I will have this completed

HugoGranstrom commented 2 years ago

Thanks :) What are a use-cases for wanting the code of a nbText block? Say we have this:

var lines: seq[string]
nbText:
  lines.join("\n")

I can't come up with any at least. If someone wants it, they probably want to roll their own block entirely and make other changes as well. At least that's how I see it. So just making it the default and not offering a separate overload would be my choice if it was up to me

pietroppeter commented 2 years ago

One use case would be to be to have a web interface like a Jupiter notebook where you can change a cell from rendered output to source and viceversa. Note that there is nothing like that currently. The other use case I need to check is the markdown cheat sheet which does show the source (but uses anyway a custom block). I tend to agree that currently making nbText by default not reading the source might be the best option, but I am not yet completely sold

HugoGranstrom commented 2 years ago

That is actually a pretty dope use case 😎 It sounds like it would need some pretty big changes regardless to make something like that work and this is a very minor change compared to those. Do as you see fit, it doesn't hurt adding that overload now, but it could otherwise always be added in the future when needed. :) Hopefully we have come up with a smart way around this when all of this becomes relevant in the future.

pietroppeter commented 2 years ago

With this implementation the typewriter will compile more or less the same Karax code for every fragment?

Wouldn't be better to have some JavaScript (added with a nb.useTypewriter) that will do that for every paragraphs with (for example) a typewriter class?

HugoGranstrom commented 2 years ago

With this implementation the typewriter will compile more or less the same Karax code for every fragment?

Indeed. That's the point of a reusable widget ;)

Wouldn't be better to have some JavaScript (added with a nb.useTypewriter) that will do that for every paragraphs with (for example) a typewriter class?

I see what you mean, but I don't see how that would simplify things really. I prefer atomic widgets over using shared code that must interact between widgets. Also then we would need to use the nbCodeToJsInit + addCodeToJs API which also would require us to insert a addToDocAsJs at the end requiring us to add a nimiSlidesEnd template or something like that that the user must remember to run before running nbSave. Or am I complicating things again?

I don't really care how the produced javascript code looks like tbh, all I want is that it is working. Then the code can be however ugly and repeating as it wants

HugoGranstrom commented 2 years ago

Ok, I re-read your message and you propose something like this?

<p class="typewriter" id="uniqueId">Type this text</p>

And then the javascript looks for all <p> tags with the typewriter class and associates each one with an object holding its id and fragmentIndex. Maybe could work but I wouldn't hide it behind a nb.useTypewriter, there's no point in doing that. I guess I'd have to if I want to insert it using nbCodeToJs 🤔

pietroppeter commented 2 years ago

Well the nb.useTypeWriter would be to add a block that contains the code to convert to javascript, not to be used for the single class instance.

HugoGranstrom commented 2 years ago

OK I understand what you mean then, it would require a few {.exportjs.} and {.importjs} to make it work. But I'm against the general use of the useSomething API. The user shouldn't have to remember to call a template to setup a widget just to use the widget if it is possible to avoid. Otherwise, we might end up with code like this:

useWidget1()
useWidget2()
useWidget3()
useWidget4()
useWidget5()

just to use a couple of widgets. For the user, it only brings more complexity. Do you understand my reasoning?

pietroppeter commented 2 years ago

Yes, I see, although html files being very big because they have a lot of JavaScript inside is also a issue. Anyway better to start having the widgets and after that improving on these details. :)

HugoGranstrom commented 2 years ago

Yes, I see, although html files being very big because they have a lot of JavaScript inside is also a issue.

That's a good point that the file size may grow a bit if we have to repeat the "boilerplate" code compiler creates each time. But for karax components that is a necessary evil as they have to be compiled in separate files. In this case karax is overkill of course and it could in theory be merged with other codes. It's also worth investigating minimizing the javascript code

HugoGranstrom commented 2 years ago

Adding a call to closure-compiler if it is available should be trivial to implement in nimib for example.

HugoGranstrom commented 2 years ago

I tried it and totally raw the code size is 200kb for one typewriter. Gzipped it was 33KB (and this is what we should look at as pages often are gzipped nowadays). Compressed and gzipped is was 24KB so not a huge difference, but it means we could fit 40 karax components in 1MB roughly. So unless you are going haywire with karax components we should be pretty safe.

HugoGranstrom commented 2 years ago

That's it for now, time to do a release!

I will have to add CI in a PR soon though...