elliotherriman / calico Goto Github PK

Not a big deal but there's a difference between all the ways a story can be loaded. The difference lies in what's passed to a patch callback (storyContent).

• new Story("file.ink") => storyContent is a string (JSON string of the compiled ink)
• new Story("file.json") => storyContent is a string (content of the JSON file)
• new Story({"json":"object"}) (as when one load story.js and use the global storyContent defined there) => storyContent is a string (JSON string of passed object)
• new Story('{"json":"string"}') => storyContent is an object ⚠️ (parsed JSON)

The 4 formats are handled by calico but one of them returns a different result and fails later in the code.
In bindExternalFunctions, content is supposed to be a string in the the 4th case, it's an object and it throws an exception.

It's easy to fix: call resolve() with the string input instead of the parsed input.

I know use of the eval patch is discouraged but I found an issue with it.
There's an undocumented eval_enabled option to set to true to enable it but the code then checks options.eval_enabled where it should check story.options.eval_enabled.

scrollafterchoice has one not ideal choice & one bug:

• It scrolls to the new content minus 20% window height. It feels weird. It might be OK for some project but I think it should scroll right to the new content by default. The 0.2 in var target = endOfText - window.innerHeight * 0.2; should be an option.
• It doesn't take the outerdiv padding into account. Even with the removal of the 20%, there's still 10% of scroll missing: it's the padding-top: 10vh of the first paragraph.

What are your thoughts on adding some code quality tools to this codebase?
I'm thinking:

• prettier for a consistent and somewhat standard formatting
• eslint to prevent mistakes and follow useful conventions
• maybe TypeScript?

audioplayer tags are processed immediately but the paragraph they are associated with may appear later (because of animations or a #delay).
I believe audio should be triggered only when the paragraph is shown.
Something like this works nicely.

Tags.add("playonce", function(story, property)
{
	property = process(story, property);
	story.queue.onShow(() => {
		audio.play(story, property, property.options, false); 
	});
});

What do you think?

We may go further and anticipate people wanting to trigger it after the animation (onRendered).
I think a nice way of exposing those possibilities would be to add a "when" option.

#playonce: my_sound >> when:shown
#playonce: my_sound >> when:rendered
#playonce: my_sound >> when:now // default

We could do "rendered + 1000ms" by combining it with delay

#playonce: my_sound >> when:rendered, delay:1000

Tell me if you like it and I'll send a PR.

There are still issues with #linebyline:

1. #linebyline: false doesn't disable linebyline because !!"false" is true, this is not a correct way to parse a boolean string. You could use property === "true" (and anything else would mean false) or use JSON.parse(property) to really parse it (and maybe wrap that in a try-catch).
2. #linebyline doesn't disable linebyline (toggling behaviour) because !story.queue.lineByLine || true will always be true.

I'll send a PR fixing this issues later.

In patches/storage.js, if we set() a value of 0, get() will return false.

ExternalFunctions.add("get", (value) => 
	{ 
		var v = get(value);
		return (isNaN(v) ? v : parseFloat(v)) || false;
	
	});

|| false is the cause of this bug.

Also, instead of checking only for numbers, we could use JSON.stringify() for encoding and JSON.parse() for decoding. It works well to encode strings, booleans, numbers.

The code for the linebyline tag:

  if (typeof property === undefined) {
    story.queue.setLineByLine(!story.queue.lineByLine || true);
  }
  else {
    story.queue.setLineByLine(!!property);
  }

typeof returns a string, so you probably meant typeof property === "undefined".
As it is, the if will always be false and always run the else case.

Hi, thanks a lot for Calico.

I was wondering if it would help others to provide a starter project with Calico and a live server, such as the one I just made :

https://github.com/cmcrobotics/calico-starter

Sadly, all the Calico and Ink.js assets (patches etc..) are duplicated - I would feel more at ease if you owned the project.
For npm init using to work, it needs to be in its own git repository - but we could craft a script that takes the latest Calico release and updates the starter project.

Let me know if you would be interested or how I can hand it over, I can prepare a PR etc...

e.g. this ink

#linebyline
I ran out of the door
#image: door.png #delay: 500
Down the street
And tripped over the package #delay: 100
Falling until I skinned my knees #delay: 500 #class: large
#delay: 700 #class: center
* Everything went black #class: green
#linebyline

results in this html

<div id="container">
        <div id="story" style="height: auto;"><p class="text" style="transition: opacity 500ms ease 0ms;"><span><p>I ran out of <span style="white-space: nowrap">the door
</span></p></span></p><p style="transition: opacity 500ms ease 0ms;"><img src="./images/door.png"></p><p class="text" style="transition: opacity 500ms ease 0ms;"><span><p>Down <span style="white-space: nowrap">the street
</span></p></span></p><p class="text" style="transition: opacity 500ms ease 0ms;"><span><p>And tripped over the <span style="white-space: nowrap">package 
</span></p></span></p><p class="text large center green" style="transition: opacity 500ms ease 0ms;"><span><p>Falling until I skinned my <span style="white-space: nowrap">knees 
</span></p></span></p><p class="choice" style="transition: opacity 500ms ease 0ms;"><a draggable="false" style="cursor: pointer;"><p>Everything <span style="white-space: nowrap">went black</span></p></a></p></div>
    </div>

Note that the class "green" gets applied to the "Falling until I skinned my knees" line rather than the "Everything went black" line

In queue.addClass():

this.contents[index].classList.add(...className);

queue.addClass() spreads the className in case it's an array but it also spreads it if it's a string.
queue.addClass("hello") results in the paragraph having classes "h", "e", "l", and "o".

If the expectation is to always call it with an array, the name of the method & arg should be changed.
I'd prefer if the method could handle both cases.

if (Array.isArray(className)) {
  this.contents[index].classList.add(...className);
} else {
  this.contents[index].classList.add(className);
}

Congratulations on the new release!

Some issues I've encountered:

• the template zip in the release page still includes choicetags.js but the release says it's removed
• the project.js in the template (zip & repo) still imports choicetags.js
• opening the template project results in an error:

Uncaught (in promise) Error: Can't resolve filename because no FileHandler was provided when instantiating the parser / compiler.

That's because the template project uses storylets & the ink file has INCLUDE patches/storylets.ink which the compiler doesn't know how to load.
I suggest providing a much simpler project.js in the default template, at least not including storylets since it doesn't load correctly.

I'm happy that Calico got a new release, now I can work on it knowing it's very much alive :-D
I found a few bugs in the v1 a few weeks ago, I haven't yet checked if they're fixed in v2. I'll report here if I find anything.
Are you open to pull requests or do you prefer issues and tackling them yourself?

Two points that could be improved in Parser.pattern:

1. For choices, patterns are always processed but for lines, patterns are only processed if the line has a tag. I don't see why this should be a requirement.
2. RegExp patterns are only processed if one wraps a regexp in the RegExp function (without new)
   • Parser.pattern(/foo/, () => {}) will not be processed
   • Parser.pattern(new RegExp("foo"), () => {}) will not be processed
   • Parser.pattern(RegExp("foo"), () => {}) will be processed
   • Parser.pattern(RegExp(/foo/), () => {}) will be processed
   • I don't see a reason for this restriction. It's fixable by replacing pattern.matcher == RegExp(pattern.matcher) with `pattern.match instanceof RegExp, which will work for all 4 cases.

When using #music: stop from audiohandler, the preload patch tries to load "stop.mp3" and fails, which removes the progress bar immediately.

I don't know yet how to solve it.

• we could replace #music: stop with #music-stop
• we could have a blacklist in preload, audiohandler would push "stop" in it

All games using autosave will have their data stored with the "save" id.
This is a problem on Itch where all games share the same sessionStorage.
autosave should have a autosave_id option that is passed to memorycard so authors can use a unique custom id.

The doc says #foo is equivalent to #class: foo but that's not true.
Since shorthandclasstags uses Parser.tag() instead of Tags.add() the class is added to a nested span (p > span > span.foo) instead of the p (p.foo).

The doc says:

Some tags can be customised via options, which by convention are separated with a ">>". For example, in musicplayer.js, you can delay a track by using the delay option.

#play: act4 >> delay: 500

This doesn't work. Nothing in the code seems to be handling that syntax and I can see the code for parsing the old format in getTagOptions.
Is this in some uncommited code?

We can't load storyContent directly since v2.

In loadInk(), the code in incorrect.
It first checks if input.endsWith(".ink") which throws an exception if it's not a string.
This check should be nested inside the if (typeof input === "string") { not before.

Here the existing code for reference:

loadInk(input) {
    // if we tried loading an ink file,
    if (input.endsWith(".ink")) {
      ...
    }
    // if we've been handed a string, it might be the story data, or it
    // might be a file name that we need to load
    else if (typeof input === "string") {
     ...
    }
    // otherwise, if it's already loaded as an object, we load that
    else if (input.inkVersionCurrent) {
      ...
    }
  }

By default there's no style on .progressbar so it's invisible.
There's a CSS rule but only in Winter template.
I suggest we add a default style with a white bar and let Winter override the color.
Maybe the color could be a patch option.