External text files on objects

[Omikhleia]

This proposed change exposes a new Text() command in-game, similar to the Verb() command, and allowing to have text strings kept in a separate file.

item.someText = Text();
item.someText = Text('some string value');
item.someText(); // returns the text string

It comes with the get-text and save-text socket requests, so that clients can possibly open the text in an editor, as they already do for verbs and functions.

Possible use:

• long descriptions, that may preferably be kept outside the JSON object on disk, for easier formatting.
• help files.
• songbooks, maps, etc.,

I have included a demo object, a guitar playing a song -- as an occasion too for me to play with the new timer feature and the onLocationChanged hook (which is quite handy here, simplifying the code).

Possible overuse:

Strings are loaded into memory, so obviously this might not be a general solution for storing huge amounts of data (e.g. for things such as an in-game mail system, etc. where on-demand loading or other mechanisms are probably preferable)

[codecov]

Codecov Report

Merging #169 into master will decrease coverage by 0.87%.
The diff coverage is 86.53%.

@@           Coverage Diff            @@
##           master   #169      +/-   ##
========================================
- Coverage   82.88%    82%   -0.88%     
========================================
  Files          27     27              
  Lines        1700   1723      +23     
  Branches      290    300      +10     
========================================
+ Hits         1409   1413       +4     
- Misses        291    310      +19

Impacted Files	Coverage Δ
src/lib/deserializer.js	100% <100%> (ø)	⬆️
src/lib/serialize.js	100% <100%> (ø)	⬆️
src/lib/print.js	100% <100%> (ø)	⬆️
src/controllers/socket-controller.js	93.33% <100%> (+0.83%)	⬆️
src/controllers/programmer-controller.js	100% <100%> (ø)	⬆️
src/lib/moo-db.js	62.5% <53.33%> (-0.16%)	⬇️
src/lib/world-object-class-builder.js	40.07% <0%> (-7.94%)	⬇️
src/controllers/unauthenticated-user-controller.js	89.83% <0%> (-1.48%)	⬇️
src/controllers/user-controller.js	91.35% <0%> (-0.78%)	⬇️
src/lib/fs-db.js	87.27% <0%> (+3.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8eb022...46594f8. Read the comment docs.

[doughsay]

This is a great addition, thanks a lot. And I'm glad the timers and onLocationChanged hooks are useful.

I think this implementation is definitely the easiest way forward for now, but I can't help but wonder if we can make the interface a bit more natural. Instead of having to call a function that returns the text, it would be nice if the property itself on the object was a Text object itself, which could be implemented as a subclass of String (see here: https://stackoverflow.com/questions/28151450/subclassing-string-in-javascript). This would be much harder to do right, but it would feel a little more natural. Besides removing a pair of parentheses though, I don't know what other benefits it would bring, so I think we can stick with this for now.

Another thing, I haven't tried yet, but can we store color escape sequences into text objects? Text editors aren't great at displaying / editing ansi escape sequences, but there are a few interesting plugins out there: here's one for sublime (https://github.com/aziz/SublimeANSI), here's one for atom (https://atom.io/packages/language-ansi-styles). Not sure what CodeMirror (the editor in the client) would do with them though...

[doughsay]

I'm going to do a more thorough review later in the week before merging.

[Omikhleia]

As for making the interface more natural and removing the parentheses, your are definitively right -- One benefit for developers could be that they don't have to know whether the text is an internal or external string, when used from other places - e.g. if a room as a regular description 'abc' and another a Text('abc'), then in my current implementation, at some other places in their code they might have to check whether to use description or description(), which is definitively not convenient...
... But wait! I just noticed that the serializer/deserializer currently doesn't have rules for String object (as it has for Date and Regexp objects)... Hmmm... Just made a quick test adding the rules, and it seems to work, without even needing subclassing, i.e.

someobject.someText1 = '123'; // internal string stored in JS object
someobject.someText2 = new String('123'); // external string stored separately

And this seems pretty neat...
The only issue I can think of right now is that strict comparisons won't work:

someobject.someText2 === '123' // false

But this is probably acceptable. (Is there a point comparing long strings for strict equality? Besides, if it's anyhow the usual JS behavior for String object, as it is for Date, etc. -- so it's something the programmers would easily understand).

We could keep Text() as a convenient utility (to hide the "new"), or not. (We cannot use String() directly, as it returns a regular string). Personally, I have no issue with "new String()", so would advocate removing the current Text() stuff.

I will want to test this further, check if there are no other loopholes in the reasoning - If not, I can update the merge request. Kind of busy this week too, so it might have to wait for the week-end.

[Omikhleia]

As for colors, it works -- But codemirror just displays the escape sequences (as weird dot character followed by the sequence). Copying them and saving doesn't break. So basically, it works, but it's kind of ugly in codemirror. Some other 'rich text' editor possibly needs to be used here, indeed, eventually.

[Omikhleia]

So here is the latest proposal using String objects.

item.someText = new String();
item.someText = new String('some string value');
item.someText // returns the text string as [String]
items.someText.trim() // trims and return a string

Despite using a String object with the usual pitfalls for these, I find it much cleaner -- at least, there's no need to introduce a new concept.

[doughsay]

One small change, otherwise this looks good to go!

[doughsay]

The Text objects were bold-cyan, but they are now yellow according to print.js. Is that a mistake? Do you want to switch them to yellow or keep them as bold cyan?

[Omikhleia]

Good catch. Yellow was finally intended (as Date, since this is no longer a function). Fixing the test description.

[doughsay]

One other question: if you player.send(someObject.someStringObject) does it send the full string or does it send [String]? i.e. do you always have to add something like .trim() to it?

[Omikhleia]

Running my own client:

this.send(items.guitar.song)

= receiving the whole text (so no [String] and no need to .trim() or .valueOf().

[doughsay]

I don't know what the deal is with travis right now; the tests definitely pass for me. Mind if I just merge as is? The tests will sort themselves out...

Another note: I think rebasing or re-writing git history in any way causes codecov to freak out. Maybe we should avoid rebasing in pull-requests.

[Omikhleia]

Wow... Wholly crashed travis on some obscure error. Tests are running locally - So I am wondering whats wrong.