WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.04k stars 166 forks source link

Standardizing h3/h4/h4 interface and dictionary markup. See Issue #2316 #2317

Closed skratchdot closed 3 years ago

skratchdot commented 3 years ago

For more info, see Issue #2316

I've updated the markup for 37 interfaces and 29 dictionaries. For a list of links, please see the following gist: https://gist.github.com/skratchdot/6f98d30ae8d7a3de01feb475c4b6df93#file-updated-idls-md

Out of the 37 interfaces in the spec, this PR standardizes the markup for the 36 interfaces that have header tags (and show up in the left navigation). There is 1 interface in the spec that does not have it's own header/left nav entry:

Here is the script I used to update the index.bs file: https://gist.github.com/skratchdot/6f98d30ae8d7a3de01feb475c4b6df93#file-update-index-sh

#!/bin/bash

interfaces="AnalyserNode AudioBuffer AudioBufferSourceNode AudioContext AudioDestinationNode AudioListener AudioNode AudioParam AudioParamMap AudioProcessingEvent AudioScheduledSourceNode AudioWorklet AudioWorkletGlobalScope AudioWorkletNode AudioWorkletProcessor BaseAudioContext BiquadFilterNode ChannelMergerNode ChannelSplitterNode ConstantSourceNode ConvolverNode DelayNode DynamicsCompressorNode GainNode IIRFilterNode MediaElementAudioSourceNode MediaStreamAudioDestinationNode MediaStreamAudioSourceNode MediaStreamTrackAudioSourceNode OfflineAudioCompletionEvent OfflineAudioContext OscillatorNode PannerNode PeriodicWave ScriptProcessorNode StereoPannerNode WaveShaperNode"
dictionaries="AnalyserOptions AudioBufferOptions AudioBufferSourceOptions AudioContextOptions AudioNodeOptions AudioParamDescriptor AudioProcessingEventInit AudioTimestamp AudioWorkletNodeOptions BiquadFilterOptions ChannelMergerOptions ChannelSplitterOptions ConstantSourceOptions ConvolverOptions DelayOptions DynamicsCompressorOptions GainOptions IIRFilterOptions MediaElementAudioSourceOptions MediaStreamAudioSourceOptions MediaStreamTrackAudioSourceOptions OfflineAudioCompletionEventInit OfflineAudioContextOptions OscillatorOptions PannerOptions PeriodicWaveConstraints PeriodicWaveOptions StereoPannerOptions WaveShaperOptions"

for i in $interfaces; do
  lcase="$(tr [A-Z] [a-z] <<< "$i")"
  gsed -i -E "s/^<h3 .*[=\"]$i[\" \>].*$/<h3 interface lt=\"$lcase\" id=\"$i\">/" index.bs
  gsed -i -E "s/^<h4 .*[=\"]$i[\" \>].*$/<h4 interface lt=\"$lcase\" id=\"$i\">/" index.bs
done

for i in $dictionaries; do
  lcase="$(tr [A-Z] [a-z] <<< "$i")"
  gsed -i -E "s/^<h4 .*[=\"]$i[\" \>].*$/<h4 dictionary lt=\"$lcase\" id=\"$i\">/" index.bs
  gsed -i -E "s/^<h5 .*[=\"]$i[\" \>].*$/<h5 dictionary lt=\"$lcase\" id=\"$i\">/" index.bs
done

If we ever need to change the h3/h4/h4 markup again, we can probably re-use something similar.


:boom: Error: 500 Internal Server Error :boom:

PR Preview failed to build. (Last tried on Apr 13, 2021, 11:17 PM UTC).

More PR Preview relies on a number of web services to run. There seems to be an issue with the following one: :rotating_light: [HTML Diff Service](http://services.w3.org/htmldiff) - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request. :link: [Related URL](https://services.w3.org/htmldiff?doc1=https%3A%2F%2Fpr-preview.s3.amazonaws.com%2FWebAudio%2Fweb-audio-api%2Fpull%2F2317%2Fe29a1cd.html&doc2=https%3A%2F%2Fpr-preview.s3.amazonaws.com%2Fskratchdot%2Fweb-audio-api%2Fpull%2F2317.html) ``` 500 Internal Server Error

Internal Server Error

The server encountered an internal error or misconfiguration and was unable to complete your request.

Please contact the server administrator at sysreq@w3.org to inform them of the time this error occurred, and the actions you performed just before this error.

More information about this error may be available in the server error log.

``` _If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please [file an issue](https://github.com/tobie/pr-preview/issues/new?title=Error%20not%20surfaced%20properly&body=See%20WebAudio/web-audio-api%232317.)._
w3cbot commented 3 years ago

dontcallmedom marked as non substantive for IPR from ash-nazg.

skratchdot commented 3 years ago

It appears a few links above are "broken", so I either need to remove them or "fix" them. I will try to annotate what's working and what's not (and compare to the current published doc).

skratchdot commented 3 years ago

It might make more sense semantically to have:

<h3 interface lt="NameOfInterface" id="nameofinterface">

I should test the behavior of that as well!

skratchdot commented 3 years ago

Okay- I found the issue from yesterday- I was not scripting out changes to h4 tags as well. I've updated the main PR description and this is ready to merge (from my perspective). Let me know if I should change anything else!

rtoy commented 3 years ago

I think there are a couple of things wrong here (but not with the actual changes to the index.bs.)

First, the merge request should be to the main branch, not to the gh-pages branch. Second, do not check in index.html.

Travis CI will take care of creating index.html automatically, and when it's merged, it will update everything needed to get the spec generated in the right place/branch.

This might also explain when the preview/diff didn't work.

skratchdot commented 3 years ago

I think there are a couple of things wrong here (but not with the actual changes to the index.bs.)

First, the merge request should be to the main branch, not to the gh-pages branch. Second, do not check in index.html.

Travis CI will take care of creating index.html automatically, and when it's merged, it will update everything needed to get the spec generated in the right place/branch.

This might also explain when the preview/diff didn't work.

Interesting. I didn't even have a main branch locally (I think due to how long ago I forked this repo). Anyways, I think I've fixed things this time. I've rebased and changed the base branch. Hopefully the preview site works this time! 🤞

skratchdot commented 3 years ago

Wondering if the pr build/preview is failing due to the source org being my github username? I'm seeing a travis job failing: https://travis-ci.com/github/skratchdot/web-audio-api/builds/222837896 🤷

rtoy commented 3 years ago

I've seen that rvm failure before. Not sure how I fixed it or if I just ignored it because it was on my fork, and not the official spec repo.

rtoy commented 3 years ago

I can't currently test the changes since preview is not working, but I have a question. If you navigate to PeriodicWave heading, the first paragraph has a link to PeridocWave. If you click it, were does it go? Currently for me it goes to the section. Does it now go to the IDL? The links to AudioContext seem to go to the IDL section. This makes more sense to me since bikeshed already has special syntax to reference sections.

skratchdot commented 3 years ago

I can't currently test the changes since preview is not working, but I have a question. If you navigate to PeriodicWave heading, the first paragraph has a link to PeridocWave. If you click it, were does it go? Currently for me it goes to the section. Does it now go to the IDL? The links to AudioContext seem to go to the IDL section. This makes more sense to me since bikeshed already has special syntax to reference sections.

In the "API Overview" section- all the links take me to the IDL definitions. Only the links in the "left nav" take me to the section heading. So if this PR is merged, any {{PeriodicWave}} will link to the IDL definition. We can make those link to the header, but if we do that- then there will be no links to the IDL (as far as I can tell). I am also noticing some inconsistant html in the "API Overview" section: https://github.com/WebAudio/web-audio-api/blob/e29a1cd5a988707cec926a885fe1fa41386d3261/index.bs#L513

It seems like that should read: An {{AudioContext}} to be consistent with the rest of that section.

I don't really have a preference to where things link, but it seems like whichever "solution" we go with, all the h3/h4 markup for the interfaces should be consistent.

skratchdot commented 3 years ago

I need to read up more on bikeshed. If I'm reading this correctly: https://tabatkins.github.io/bikeshed/#autolinking

I think {{PeriodicWave}} should link to the IDL definition and [[PeriodicWave]] will link to the header? I need to test this.

Also, reading the data model contract: https://tabatkins.github.io/bikeshed/#dfn-contract

I don't think I'm actually doing part 4 data-dfn-type MUST be provided, and set one of the accepted values. But maybe the interface attribute is shorthand for data-dfn-type="interface"? 🤷

rtoy commented 3 years ago

I think we're ok here. I don't think there's much use of [[foo]] for section references (but we do use a lot of [[foo]] to mean an internal slot). So all links like {{foo}} should go to the definition of foo, which is usually the IDL defining it or the function/attribute description.

I think none of the headings are actually defining any term, so what you have for the headings seems fine.

rtoy commented 3 years ago

Grabbed your branch and built it locally. I get some new bikeshed errors:

+LINE -1: Couldn't find target document section audioworklet:
+[[#audioworklet|direct script processing and synthesis]]
+LINE ~295: Couldn't find target document section audioworklet:
+[[#audioworklet|directly using scripts]]
+LINE ~325: Couldn't find target document section analysernode:
+[[#analysernode|real-time time-domain and frequency-domain analysis / music visualizer support]]
  ✔  Successfully generated, but fatal errors were suppressed

I think with the new scheme, the links here should be using [[#CamelCase]] to get to the section.

I didn't check all the links, but the ones I did test go to the IDL section as we wanted.

skratchdot commented 3 years ago

@rtoy - Good call on the bikeshed errors. At some point I started ignoring those and just confirming the links.

I fixed the errors you mentioned in 245578f09d3af31a7a6a81cba9eb45e57c33ef33

But I'm now seeing:

LINE 2022: Can't find the 'contextOptions' argument of method 'OfflineAudioContext/constructor(numberOfChannels, length, sampleRate)' in the argumentdef block.
LINE 3129: Can't find the 'destinationNode' argument of method 'AudioNode/connect(destinationParam, output)' in the argumentdef block.
LINE 3129: Can't find the 'input' argument of method 'AudioNode/connect(destinationParam, output)' in the argumentdef block.
LINE 3230: Can't find the 'destinationNode' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
LINE 3243: Can't find the 'destinationNode' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
LINE 3258: Can't find the 'destinationNode' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
LINE 3258: Can't find the 'input' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
 ✔  Successfully generated, but fatal errors were suppressed

I'm not sure if those should be fixed in this PR or what.

When I compile the main branch, I see a similar set of errors:

LINE 2022: Can't find the 'contextOptions' argument of method 'OfflineAudioContext/constructor(numberOfChannels, length, sampleRate)' in the argumentdef block.
LINE 3129: Can't find the 'destinationNode' argument of method 'AudioNode/connect(destinationParam, output)' in the argumentdef block.
LINE 3129: Can't find the 'input' argument of method 'AudioNode/connect(destinationParam, output)' in the argumentdef block.
LINE 3230: Can't find the 'destinationNode' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
LINE 3243: Can't find the 'destinationNode' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
LINE 3258: Can't find the 'destinationNode' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
LINE 3258: Can't find the 'input' argument of method 'AudioNode/disconnect(destinationParam, output)' in the argumentdef block.
 ✔  Successfully generated, but fatal errors were suppressed
rtoy commented 3 years ago

You should use compile.sh to run bikeshed. This compares the expected errors with the actual errors. If there are differences, the output will list them. If compile.sh says there are no differences, we're all set.

Travis CI passes, so I think everything is fine now.

skratchdot commented 3 years ago

Yeah- I see travis passing now: https://travis-ci.com/github/skratchdot/web-audio-api/builds/222988145

But seeing:

Skipping a deployment with the pages provider because this branch is not permitted: issue-2316

(I think b/c the branch is in my repo)

rtoy commented 3 years ago

I think that's ok, because only changes to the main branch should be deployed. Don't know why the preview is failing though.

While randomly clicking on headers, I noticed that AudioBufferSourceOptions section doesn't link to the IDL. The other dictionaries also end up at the section heading, not he IDL. These might need to be fixed too.

skratchdot commented 3 years ago

I think that's ok, because only changes to the main branch should be deployed. Don't know why the preview is failing though.

While randomly clicking on headers, I noticed that AudioBufferSourceOptions section doesn't link to the IDL. The other dictionaries also end up at the section heading, not he IDL. These might need to be fixed too.

Good call. I can update the dictionaries as well.

skratchdot commented 3 years ago

I updated the dictionary markup in 1ebd936dc465c97ca7f0920f63315098d63358d5

I need to update the description with links to the 29 dictionaries. I'll start testing that all 58 (29*2) of the links in that commit "work"

skratchdot commented 3 years ago

All 58 of the dictionary links seem to work with 1 exception:

IIRFilterOptions

For some reason, when you click IIRFilterOptions in the header section- it doesn't take you to the IDL- and the "popup" opens to the right of the page. Kind of weird.

Also, all the IDL links have dictdef- prepended to them

rtoy commented 3 years ago

For reference, the original webaudio bikehsed issue is #2185, which is now being tracked in tabatkins/bikeshed#1740.

rtoy commented 3 years ago

Ah, for the IIRFilterOptions link, I think you need to say:

<h4 dictionary ...>
{{IIRFitlerOptions}}</h4>

to get the section heading to link to the IIRFilterOptions IDL.

skratchdot commented 3 years ago

Ah, for the IIRFilterOptions link, I think you need to say:

<h4 dictionary ...>
{{IIRFitlerOptions}}</h4>

to get the section heading to link to the IIRFilterOptions IDL.

Yeah- I was just now fixing that :) Should be fixed in 9910392dc0de477f682a5df73a23a3130a7c044d

rtoy commented 3 years ago

Assuming the links to your repo aren't really stable, I would not include them in the description. You can just list all the nodes/headers/dictionaries that you updated.

(I think the description is what gets committed as the merge log. I forget if that's exactly true. I think you also get an option to edit the message before merging.)

skratchdot commented 3 years ago

Assuming the links to your repo aren't really stable, I would not include them in the description. You can just list all the nodes/headers/dictionaries that you updated.

(I think the description is what gets committed as the merge log. I forget if that's exactly true. I think you also get an option to edit the message before merging.)

Good point. I'll just put them in a gist (with versions that point to my repo- and the real repo), that way we can confirm before and after merging the PR.

rtoy commented 3 years ago

Excellent. Since a gist is stable, more or less, a link to the gist is fine.

Also, if you used a simple sed/awk/perl script to do this, pasting the script into the description is helpful.

I looked over everything and it looks fine. I did some random testing and everything is fine too.

The hard part is if there were some stray links that now no longer work. But I think we can fix those as we discover them.

skratchdot commented 3 years ago

@rtoy - I updated the description. I have a gist with a few files in it. The script I used to write the links is there: https://gist.github.com/skratchdot/6f98d30ae8d7a3de01feb475c4b6df93#file-write-links-sh

I just slightly modified it to generate the following files:

The script I made to modify the index.bs file is here: https://github.com/skratchdot/web-audio-api/blob/gh-pages/index-with.sh

I will clean that up and paste in the description.

rtoy commented 3 years ago

Everything looks fine. When you are satisfied with everything, let me know, and we can merge this in.

skratchdot commented 3 years ago

Everything looks fine. When you are satisfied with everything, let me know, and we can merge this in.

I'm done making changes (unless there's anything else we find) 😃. I hope there are no unintended consequences 🤞 I did manually click all the links in: https://gist.github.com/skratchdot/6f98d30ae8d7a3de01feb475c4b6df93#file-links-skratchdot-md and things looked good, but it's definitely hard to confirm "everything still works"!

Hopefully all goes smoothly after the merge :)

rtoy commented 3 years ago

Looks good. The new version is live now and doing some random checking works as expected. Thanks!