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

Moved from Ace editor to Monaco editor #84

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

fudgepop01
Copy link
Member

ADDED

  • ksy schema
  • monaco editor (with autocomplete support)
  • inlineish errors
  • "modes" to the hex viewing window
  • yaml 1.2 support

FIXED

  • scrolling through the hex viewer on mac devices
    • done by adding a new mode that automatically toggles based on user input

ADDED
- ksy schema
- monaco editor (with autocomplete support)
- inlineish errors
- "modes" to the hex viewing window
- yaml 1.2 support

FIXED
- scrolling through the hex viewer on mac devices
@GreyCat GreyCat requested a review from koczkatamas March 30, 2019 21:18
src/AppLayout.ts Outdated
@@ -1,5 +1,6 @@
import { LayoutManager, Container, Component, ClosableComponent } from "./LayoutManagerV2";
import * as ace from "ace/ace";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you're phasing out ace, may be it's possible to remove it from here?

src/AppLayout.ts Outdated
// if (lang === "yaml")
// editor.setOption("tabSize", 2);
// editor.$blockScrolling = Infinity; // TODO: remove this line after they fix ACE not to throw warning to the console
// parent.container.on("resize", () => editor.resize());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No point in leaving commented out stuff, git will keep old version just fine.

Copy link
Member

@koczkatamas koczkatamas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the following issues running the ./build script. Could you fix them? (Otherwise the build will fail on our CI too AFAIS).

lib/ts-types/monaco.d.ts:1084:22 - error TS2310: Type 'IStandaloneCodeEditor' recursively references itself as a base type.

1084     export interface IStandaloneCodeEditor extends IStandaloneCodeEditor {
                          ~~~~~~~~~~~~~~~~~~~~~


lib/ts-types/text-encoding.d.ts:5:15 - error TS2300: Duplicate identifier 'TextEncoder'.

5 declare class TextEncoder
                ~~~~~~~~~~~


lib/ts-types/text-encoding.d.ts:12:15 - error TS2300: Duplicate identifier 'TextDecoder'.

12 declare class TextDecoder
                 ~~~~~~~~~~~


node_modules/typescript/lib/lib.dom.d.ts:13122:11 - error TS2300: Duplicate identifier 'TextDecoder'.

13122 interface TextDecoder {
                ~~~~~~~~~~~


node_modules/typescript/lib/lib.dom.d.ts:13129:13 - error TS2300: Duplicate identifier 'TextDecoder'.

13129 declare var TextDecoder: {
                  ~~~~~~~~~~~


node_modules/typescript/lib/lib.dom.d.ts:13134:11 - error TS2300: Duplicate identifier 'TextEncoder'.

13134 interface TextEncoder {
                ~~~~~~~~~~~


node_modules/typescript/lib/lib.dom.d.ts:13139:13 - error TS2300: Duplicate identifier 'TextEncoder'.

13139 declare var TextEncoder: {
                  ~~~~~~~~~~~


src/AppView.ts:96:9 - error TS2554: Expected 1 arguments, but got 2.

96         editor.setValue(content, -1);
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~


src/KsyAutoCompleter.ts:111:24 - error TS2304: Cannot find name 'YAML'.

111             this.ksy = YAML.parse(editor.getValue(), false, null, true);
                           ~~~~


src/KsyAutoCompleter.ts:112:51 - error TS2339: Property 'getSelectionRange' does not exist on type 'IStandaloneCodeEditor'.

112             this.context = this.getContext(editor.getSelectionRange().start.row + 1);
                                                      ~~~~~~~~~~~~~~~~~


src/KsyAutoCompleter.ts:137:70 - error TS2339: Property 'session' does not exist on type 'IStandaloneCodeEditor'.

137         var linePadding = KsyAutoCompleter.getPaddingLen(this.editor.session.getLine(row - 1));
                                                                         ~~~~~~~


src/KsyAutoCompleter.ts:144:36 - error TS2339: Property 'session' does not exist on type 'IStandaloneCodeEditor'.

144             var line = this.editor.session.getLine(row - 1);
                                       ~~~~~~~


src/Playground.ts:3:22 - error TS2307: Cannot find module 'yamljs'.

3 import { YAML } from "yamljs";
                       ~~~~~~~~


src/utils.ts:59:16 - error TS2554: Expected 0 arguments, but got 1.

59         return new TextEncoder("utf-8").encode(str);
                  ~~~~~~~~~~~~~~~~~~~~~~~~


src/utils/Conversion.ts:7:16 - error TS2554: Expected 0 arguments, but got 1.

7         return new TextEncoder("utf-8").encode(str).buffer;
                 ~~~~~~~~~~~~~~~~~~~~~~~~


src/v2.ts:43:57 - error TS2345: Argument of type 'IStandaloneCodeEditor' is not assignable to parameter of type 'Editor'.
  Property 'on' is missing in type 'IStandaloneCodeEditor'.

43         this.ksyChangeHandler = new EditorChangeHandler(this.view.ksyEditor, 500,
                                                           ~~~~~~~~~~~~~~~~~~~


src/v2.ts:44:14 - error TS7006: Parameter 'newContent' implicitly has an 'any' type.

44             (newContent, userChange) => this.inputFileChanged("Ksy", newContent, userChange));
                ~~~~~~~~~~


src/v2.ts:44:26 - error TS7006: Parameter 'userChange' implicitly has an 'any' type.

44             (newContent, userChange) => this.inputFileChanged("Ksy", newContent, userChange));
                            ~~~~~~~~~~


src/v2.ts:46:62 - error TS2345: Argument of type 'IStandaloneCodeEditor' is not assignable to parameter of type 'Editor'.

46         this.templateChangeHandler = new EditorChangeHandler(this.view.templateEditor, 500,
                                                                ~~~~~~~~~~~~~~~~~~~~~~~~


src/v2.ts:47:14 - error TS7006: Parameter 'newContent' implicitly has an 'any' type.

47             (newContent, userChange) => this.inputFileChanged("Kcy", newContent, userChange));
                ~~~~~~~~~~


src/v2.ts:47:26 - error TS7006: Parameter 'userChange' implicitly has an 'any' type.

47             (newContent, userChange) => this.inputFileChanged("Kcy", newContent, userChange));
                            ~~~~~~~~~~


src/v2.ts:171:13 - error TS2554: Expected 1 arguments, but got 2.

171             this.view.jsCode.setValue(Object.values(compilationResult.releaseCode).join("\n"), -1);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


src/v2.ts:172:13 - error TS2554: Expected 1 arguments, but got 2.

172             this.view.jsCodeDebug.setValue(compilationResult.debugCodeAll, -1);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


src/worker/KaitaiWorkerV2.ts:6:22 - error TS2307: Cannot find module 'yamljs'.

6 import { YAML } from "yamljs";
                       ~~~~~~~~

@fudgepop01
Copy link
Member Author

thanks for reviewing! I'll get right to fixing that stuff up

for some reason though I can't for the life of me figure out
why the compiler refuses to aknowledge the existance of
the js-yaml.d.ts file
@fudgepop01
Copy link
Member Author

The issues that remain when I try to run the build script come from a lot of the "V2" files, which appear to be unused at the moment - at least when i'm testing. The others are typescript definition issues, which I'm unsure how to fix unfortunately. For whatever reason it refuses to recognize the js-yaml typedef file.

However, because these are all only issues with the typescript definition files, I'm sure that they shouldn't affect the performance of the webapp at all once they do compile.

@fudgepop01
Copy link
Member Author

disregard that last comment - it seems that building actually broke things for me. So, i'll work on trying to fix things once more and i'll get back to you once I do

the typescript compiler does NOT like the lack of an import
statement in kaitaiWorker.ts for KaitaiStream. If I try to include
it, it compiles without issue but it breaks when it attempts to
load in the browser because it tries to use requirejs to actually
import it. This doesn't work.

Everything else however, should work.
@fudgepop01 fudgepop01 changed the title Moved from Monaco editor to Ace editor Moved from Ace editor to Monaco editor Apr 12, 2019
fudgepop01 and others added 4 commits April 15, 2019 10:41
* Added support for doc and doc-ref properties in many places
* Added support for params on types
* Added various meta properties
* Added support for verbose enums
* Added support for endian switching
* Added pad-right property
* Added -orig-id property
* Allowed arbitrary properties starting with a dash in various places
* Allowed meta sections on non-top-level types
* Allowed string values for terminator property (needed for non-decimal
  number literals)
* Allowed using booleans in place of strings in a few places
* Changed encoding property from an enum to a plain string - it's
  impossible to list all available encodings (especially since they
  vary between target languages)
* Removed incorrect patterns from switch-on and enum keys
Copy link
Member

@GreyCat GreyCat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really cool, a massive amount of work!

This is probably the largest code review I've done in my life, please bear with it ;)

.gitignore Outdated
# this SHOULD keep critical files ... but it doesn't for some reason
!lib/_npm/vs
!lib/_npm/monaco
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, what are you doing here, and what are these "critical files"? If I'm not mistaken, the very idea is that lib/_npm should be populated during project build, and not kept in this repo.

Playground.html Show resolved Hide resolved
@@ -0,0 +1,143 @@
/*!-----------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely must not be checked into this repo.

@@ -0,0 +1,335 @@
const ksySchema =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not part of monaco, why would we want to put it into the same directory?

const ksySchema =
{
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "ksy schema",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually very-very cool, and should be at very least advertised at kaitai-io/kaitai_struct#47. Ideally, this should be a separate product, may be even in a separate repo, as this should be as widely reusable as possible.

@@ -0,0 +1,1895 @@
(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be unused copy of old YAML library? If so, just delete it, no need to keep it.

this.contentOuter = $(`<div class="contentOuter" tabindex="1"></div>`).appendTo(this.scrollbox);

// displays weather it's in scroll or select mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whether?

// editor.$blockScrolling = Infinity; // TODO: remove this line after they fix ACE not to throw warning to the console
// editor.setReadOnly(isReadOnly);
// if (callback)
// callback(editor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably can be just deleted; we can totally rely on git to keep version history for us

console.log(error);
if (!error.error.s$1) return;

let path = error.error.s$1.split(" ")[0].split("/");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really cool! this error.error.s$1 parsing magic seems pretty awkward, I need to see if I can provide some more natural bindings through ScalaJS...

src/v1/app.ts Outdated
// exec: function (editor: any) { app.reparse(); } });
// app.ui.ksyEditor.addCommand(
// { name: "compile", bindKey: { win: "Ctrl-Enter", mac: "Command-Enter" },
// exec: function (editor: any) { app.recompile(); } });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, let's not keep the history in commented out code

@GreyCat
Copy link
Member

GreyCat commented Apr 18, 2019

Some extra comments:

  • There is vendor.yaml file in this repo root, which lists all open source dependencies. Please update it to remove Ace & old YAML parser and include Monaco & new YAML parser. It's vital to keep it up to date, as this guarantees that we show proper open source credits.

@KOLANICH
Copy link

Could you move the JSON Schema into a separate repo? It is be useful for other stuff, for example there are Language Servers for YAML + JSON Schema.

@fudgepop01
Copy link
Member Author

sure thing! I'm currently in the process of fixing all the issues @GreyCat brought up.

However, I unfortunately wouldn't quite know how to link this up to the repository as a dependency. It would be fantastic if someone who knows how could do that once I fix things and create a separate repo for the schema.

@KOLANICH
Copy link

KOLANICH commented Apr 18, 2019

wouldn't quite know how to link this up to the repository as a dependency.

git submodule

@dgelessus
Copy link

By the way, there's been a slight mixup with the JSON schema. As @GreyCat pointed out, the schema exists twice: once in a plain .json file and once assigned to a constant in a .js file. Right now the two schemas are also different - the .json version used to be an older version of the object from the .js file, but when I made my PR to improve the schema I accidentally based it on the outdated .json version. So now we have two different versions of the schema.

I'll try to fix this soon and merge everything into one again (which will also fix a few of the issues that @GreyCat found in the review above). The situation with having two copies of the same data in two files isn't great though. Ideally you'd perform the conversion from .json to .js automatically as a build step, but I don't know anything about the JS ecosystem so I have no idea how to implement that.

+1 from me for putting the JSON schema into a separate repo though. Personally I use the WebIDE very rarely since it doesn't work well with my workflow, but would still like to use the JSON schema to get basic IDE support for ksy files.

@fudgepop01
Copy link
Member Author

Here's a link to the proper schema repositories: https://github.com/dar2355/Ksy-Schema

I'm still working on fixing the bugs introduced by properly using the _npm directory, but lots of great progress is being made towards a better Pull Request

the yamljs package didn't allow for the standard json-esq
formatting, which also coincidentally is the only way to get
autosuggestions working properly. The js-yaml fixes this issue
and supports the json-like syntax, allowing for a much smoother
coding experience
validate: true,
schemas: [
{
uri: `http://myserver/ksy-schema.json`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


requirejs.config({ baseUrl: "js/v1/", paths: paths });
require(["app.unsupportedBrowser"]);
require(["../../lib/_npm/js-yaml/js-yaml"], (YAML) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if require.js can be dropped and ECMAScript modules used instead.

@ChristianLW
Copy link

Any updates on this? I'm thinking of creating a PR for things relating to Ace, but if this is not too far from completion it doesn't really make sense for me to.

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 this pull request may close these issues.

6 participants