Some comments on score annotation and <SelectableScore>

[gklyne]

I'm posting these comments here for information, and so they have some visibility to MELD developers... They are based on my experiences creating hello-meld-4 and hello-meld-5 through adaptation of TrompaMusic examples, work conducted as part of a small Software Sustainability study based on MELD.

• there appears to be no selection type for whole piece (e.g. no fragment): I would have thought being able to attach annotations to the whole piece is a fairly common requirement?

• In annotationItem.js, about line 600, code does a crude attempt at URI resolution. A better approach would be to use a URL handling library to do the resolution properly. E.g. rdflib.js contains a module rdflib.js includes https://github.com/linkeddata/rdflib.js/blob/master/src/uri.ts. https://nodejs.org/api/url.html or even https://developer.mozilla.org/en-US/docs/Web/API/URL/URL would be better options.

• In selectablScoreApp.js it's not clear why the motivation switch logic is required - it appears to be setting class values on annotations. E.g., why isn't this handled in annotationItem.js?

• I would have liked documentation of "<SelectableScore>" abstraction used. [Documentation issue noted in SSI report]. I did later find documentation at https://github.com/trompamusic/selectable-score - maybe place a link in a source code comment?

• I found the music-scholars-annotation code structure seems to entangle a number of different concerns; e.g. duplicated logic between annotationItem.renderSwitch and selectableScoreApp.onReceiveAnnotationContainerContent (refactor for DRY?).

  A particular thought I had for this was that each annotation type could be implemented as a separate React element, with all of the associated logic in a single module. Then, at the level of (something like) annotationSubmitter.js/annotationItem.js, the appropriate annotation type element would be selected, and avoid the need for further tests. Maybe use an "annotation object factory" element to encapsulate all the annotation switching logic?

[gklyne]

Also: I've just been trying to use the app to authenticate using the inrupt.com enterprise server - it doesn't seem to respond to the "go" button whenI enter my own POD URI (as opposed to using one of the buttons below).

[gklyne]

Re the authentication issue: it appears that the solid-auth-client popup is incompatible with the Inrupt enterprise server. There is a note in solid-auth-client saying "⚠️ New projects should use solid-client-authn or solid-auth-fetcher instead, which leverage the secure DPoP authentication mechanism from the current Solid specification, as implemented by all the various Solid server implementations."

What I'm seeing is this:

auth-popup.html:23 GET https://pod.inrupt.com/.well-known/openid-configuration 401

auth-popup.html:28 Error logging in with WebID-OIDC

auth-popup.html:28 Error: Error fetching openid configuration: 401 
    at auth-popup.html:23
    at async http:/localhost:8080/auth-popup.html:28
    at async Se (auth-popup.html:28)
    at async Object.<anonymous> (auth-popup.html:28)

The failure seems to be that the client code expects to be able to retrieve /.well-known/openid-configuration, which apparently isn't supported by the Inrupt enterprise server. It's possible that this is because .well-known is defined only for the root of a web domain, and Inrupt places user pods in their own subpath (e.g. /gklyne/ in my case). But that's just a guess. My public profile is at https://pod.inrupt.com/gklyne/profile/card#me .

[alastair]

Hi @gklyne, thanks for the detailed review!

Here are some rough responses, happy to continue discussing them in this ticket or in some others

there appears to be no selection type for whole piece (e.g. no fragment): I would have thought being able to attach annotations to the whole piece is a fairly common requirement?

Very valid argument, it should be added! We can open a new ticket for this

In annotationItem.js, about line 600, code does a crude attempt at URI resolution.

Agreed, let's open a new ticket

[notes about abstraction, documentation, and separation of concerns]

These are all valid points. This tool grew rather ... organically, and there are definitely some parts which do things that they shouldn't. There are a few moving parts here that we want to improve:

• I've been working on the annotation tools, which is going to give us a single place for an annotations data model, as well as api methods to read and write annotations
• Wtih @musicog we have a few ideas about how we might want to change SelectableScore - given that we have the above annotations library we can probably move some things out of this library and into the annotations one. Regarding documentation, we could add a bit more to the readme of this app to say which major dependencies we use
• Depending on budget constraints, your comments about refactoring to separate concerns is definitely something that we should improve. I think with the above annotation library, and a better knowledge about how annotation tasks are performed in general (especially taking into account other tools that we developed during trompa), we should be able to split this up in a way that makes it easier to understand, and easier to reuse react components in other projects as well.

It's possible that this is because .well-known is defined only for the root of a web domain, and Inrupt places user pods in their own subpath (e.g. /gklyne/ in my case).

I don't think this is the case. From the webid-oidc spec: "This is always located at the OP's host followed by /.well-known/openid-configuration"
It's possible that this spec has changed since I last read it, and the other auth projects that you mentioned do this discovery differently

It's interesting to see that the inrupt pod uses subpaths! When I spoke with @musicog about this a few years ago I was surprised that the sample node-solid-server didn't support this. The developers were very against this type of user path for some reason. Great to see that someone has done it.

There is a note in solid-auth-client saying "⚠️ New projects should use solid-client-authn or solid-auth-fetcher instead"

It's a shame that there seems to be such a habit here to make one piece of software and then throw it away and make another one. We've seen this happen in a few places in trompa. I promise that solid-auth-client was the recommended tool to use when we first started the project... Perhaps it's not too difficult to perform an upgrade to one of these.

Thanks for the notes!

[gklyne]

Hi Alastair, Picking up on just the .well-known issue.

On 19/05/2021 09:12, Alastair Porter wrote: It's possible that this is because .well-known is defined only for the root of a web domain, and Inrupt places user pods in their own subpath (e.g. /gklyne/ in my case). I don't think this is the case. From the webid-oidc spec <https://github.com/solid/webid-oidc-spec/blob/master/application-user-workflow.md#3-request-op-configuration>: "This is always located at the OP's host followed by /.well-known/openid-configuration" It's possible that this spec has changed since I last read it, and the other auth projects that you mentioned do this discovery differently

After a brief look at the spec, I think you're right. I was making an assumption that the openid-configuration was being defined per-pod, rather than per-host, but the webid-oidc spec suggests otherwise. What's happening is that the Inrupt enterprise server is responding with "401 unauthorized" when attempting to access it - which is ironic as its the first step in the process of obtaining authorization! Seems like an Inrupt bug to me? (You mention the (Solid) software spec changing - I sympathize. But Solid is still maturing, and this area is one that I think the community have been struggling to get right after Google/Chrome pulled the plug on their original design. I suspect this is a change that has been forced upon them. Hopefully, the new interface is designed to insulate apps from any future underlying changes.)