Implementation of syntax checks for Bespin

RESOLVED FIXED

Status

RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: whimboo, Assigned: malte.ubl)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090215 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090215020429

It would be great if Bespin could not only highlight the code but also would run syntax checks against the contents. That could prevent failures in early stages of the development. Underlining broken code to visualize failures would be great. That could be implemented for JS, HTML, and CSS.
(Assignee)

Comment 1

10 years ago
Created attachment 367381 [details] [diff] [review]
Syntax Checker and Outline View

The attached patch implements a syntax checker and primitive outline view for JavaScript.
Syntax is checked on the fly and errors are displayed inside the info-panel.
The new command outline shows a clickable list of functions inside the current document.

Please note: I added a pavement task to download narcissus. Unfortunately I had to patch the parser to reports line numbers in syntax error in a way that is compatible with Safari workers.
I also had to paste narcissus into the bootstrap_worker.js file because Safari currently implements no way to load source into a worker (FF 3.1 implements both importScripts and XMLHttpRequest)
Attachment #367381 - Flags: review+
(Reporter)

Comment 2

10 years ago
Comment on attachment 367381 [details] [diff] [review]
Syntax Checker and Outline View

You cannot give r+ on your own. Ben, I hope you are the right one to ask for review of the attached patch?
Attachment #367381 - Flags: review+ → review?(bgalbraith)
(Reporter)

Updated

10 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Updated

10 years ago
Summary: Implementation of syntax checks → Implementation of syntax checks for Bespin

Comment 3

10 years ago
I'll take a look tonight and see if we can get this in. Because it's such a massive patch, may take until Monday to get in.

Comment 4

10 years ago
Here are some comments from my look at the patch (NOTE: these will be minor... this is great work!)

- There are a mix of tweaks that aren't to do with this work in here (e.g. the commented out "/*if(!thing) {", having autoconfig not needed, etc.
- The new "bespin:editor:languagechange" isn't needed as we already have an event that does that in: bespin:settings:syntax
- maybe we split out the gears stuff (gears.js, USE_GEARS etc so it can be shared elsewhere [e.g. storage stuff])
- Is JSON2 needed since we have dojo.toJSON etc?
(Assignee)

Comment 5

10 years ago
- There are a mix of tweaks that aren't to do with this work in here (e.g. the
commented out "/*if(!thing) {", having autoconfig not needed, etc.

Sorry, I'll remove this

- There are a mix of tweaks that aren't to do with this work in here (e.g. the
commented out "/*if(!thing) {", having autoconfig not needed, etc.

Hmm, is that other event really the only (future) place where a language change could occur?

- maybe we split out the gears stuff (gears.js, USE_GEARS etc so it can be
shared elsewhere [e.g. storage stuff])

Can do that. There might also be a situation where we use one feature from HTML5 and another from Gears (If a browser has incomplete support). I also modified Google's code to only execute if we actually need gears at all.

- Is JSON2 needed since we have dojo.toJSON etc?

We need some kind of JSON implementation inside the web worker, because both Safari and FF do not yet support passing complex datatypes via postMessage. In FF 3.1 we could use the native support. Because dojo is currently not available in the worker I chose to attach json2.js
(Reporter)

Updated

10 years ago
Assignee: nobody → malte.ubl
Status: NEW → ASSIGNED

Comment 6

10 years ago
Dion beat me to the patch on 3/14 on in chatting we decided he's going to shepard this through to a safe landing, so I'm begging off unless someone explicitly asks for my input :-)

Comment 7

10 years ago
(In reply to comment #5)
> - There are a mix of tweaks that aren't to do with this work in here (e.g. the
> commented out "/*if(!thing) {", having autoconfig not needed, etc.
> 
> Sorry, I'll remove this

No problem. That was more "for someone else reviewing".
 
> - There are a mix of tweaks that aren't to do with this work in here (e.g. the
> commented out "/*if(!thing) {", having autoconfig not needed, etc.
> 
> Hmm, is that other event really the only (future) place where a language change
> could occur?

So far. You could be right though..... but in either case we should unify so set:syntax uses the languagechange, so only one item does the editor.language tweak and we minimize events.
 
> - maybe we split out the gears stuff (gears.js, USE_GEARS etc so it can be
> shared elsewhere [e.g. storage stuff])
> 
> Can do that. There might also be a situation where we use one feature from
> HTML5 and another from Gears (If a browser has incomplete support). I also
> modified Google's code to only execute if we actually need gears at all.

Sounds great. Definitely don't want to load the Gears stuff unless needed.

> - Is JSON2 needed since we have dojo.toJSON etc?
> 
> We need some kind of JSON implementation inside the web worker, because both
> Safari and FF do not yet support passing complex datatypes via postMessage. In
> FF 3.1 we could use the native support. Because dojo is currently not available
> in the worker I chose to attach json2.js

Makes sense.

Let me know when you have something that you feel good about and I will start to get it integrated! I don't know if you have already created your own repo, but it would be cool to do it that way. E.g. Julian has done just that: http://bitbucket.org/j4c/bespin/ It means that we can quickly sync up to each others work.

Thanks! Can't wait to get this into tip!
(Assignee)

Comment 8

10 years ago
I have a repository up @ http://bitbucket.org/malde/bespin-malde/

With respect to your comments:
I removed the unrelated change to the registry and I settled for using the set:syntax change for now.

I'd feel comfortable putting this into tip. Currently there is no way disable the parser without disabling syntax highlighting. Should this be added?

Brendan noticed my blog post and we've been in touch about merging my changes into narcissus tip. Once this is done we can eliminate bundling narcissus with bespin. I will also prepare a workaround to enable access to XMLHttpRequest for workers inside Safari.

Comment 9

10 years ago
Fantastic stuff all around.

It would be nice to be able to turn on and off the parser. I can get this in before the chance... but it would be nice, also so then we can test the performance. We need to make sure the puppy hums :)

Comment 10

10 years ago
Malte,

Got your stuff in tip and added a couple of things.

the goto command now uses your work so you can: goto resize and it calls something new in parser:

bespin:parser:gotofunction

And I abstracted out a new event:

bespin:editor:moveandcenter

as we do that in a few places.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

10 years ago
Dion, can it somewhere be tested? Is there a staging site around?
This is a mass migration from Mozilla Labs :: Bespin to Bespin :: General.

This bug likely still needs to be triaged and categorized.
Component: Bespin → General
Product: Mozilla Labs → Bespin
QA Contact: bespin → general
Target Milestone: -- → ---
Version: 0.1 → 0.1.1

Updated

7 years ago
Attachment #367381 - Flags: review?(ben) → review+
You need to log in before you can comment on or make changes to this bug.