readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
550 stars 186 forks source link

Highlight Support in ReadiumJS Disabled? #657

Closed rkwright closed 6 years ago

rkwright commented 6 years ago

This issue is a Discussion

Related issue(s) and/or pull request(s)

None?

Expected Behaviour

Highlights should work

Observed behaviour

Instead, with release 0.27 they are disabled by default

Test file(s)

Not available?

Product

ReadiumJS, 0.28-alpha

Additional information

None?

rkwright commented 6 years ago

from Ken Jones, ken@circularsoftware.com

I've noticed that the method I have been using to highlight overlaid panels on sheet music has stopped working on both readium.github.io/readium-js-viewer and readium.firebaseapp.com

e.g.

tracktracker

I am using a CSS brightness adjustment to affect the appearance of the panels in time with the SMIL.

rkwright commented 6 years ago

from @danielweck

The highlighter feature has been removed from the default build configuration of readium-shared-js (consequently, this impacts the default deployment of the cloud reader at Firebase and Surge). The actual code has not been removed though, you can easily re-activate the plugin by uncommenting this line: https://github.com/readium/readium-shared-js/blob/62490d18031c6dd675639b442a7b791274bfe7b1/plugins/plugins.cson#L3 PS: note that readium.firebaseapp.com is always up to date (automatic builds from the develop branch), whereas readium.github.io/readium-js-viewer has historically been a little bit behind updates, and will be deprecated soon. I hope this helps. Let us know if you have any problems building your own version of the cloud reader. There are instructions in the README: https://github.com/readium/readium-js-viewer/blob/develop/README.md#prerequisites

rkwright commented 6 years ago

from @danielweck

Oh, and you probably want to deactivate the Hypothesis annotation plugin as well (comment the line): https://github.com/readium/readium-shared-js/blob/62490d18031c6dd675639b442a7b791274bfe7b1/plugins/plugins.cson#L6

I am building a version of the cloud reader for you...

rkwright commented 6 years ago

from @danielweck

https://readium-highlights.surge.sh

Example with a reflowable text document (to verify that the selection highlights work):

https://readium-highlights.surge.sh/?epub=https%3A%2F%2Freadium.firebaseapp.com%2Fepub_content%2Faccessible_epub_3&goto=%7B%22idref%22%3A%22id-id2611884%22%2C%22elementCfi%22%3A%22%2F4%2F2%5Bintroduction%5D%2F2%2F1%3A0%22%7D&

llemeurfr commented 6 years ago

In fact, activating hypothesis by default may have been a bad move, as many implementers are in fact relying on the default highlight module. We may decide to revert to the default highlight module for 0.28; any thoughts?

rkwright commented 6 years ago

Many? Who? Bear in mind that this change only affects those deploying their own instance of CloudReader - not the SDK and not the Chrome app. When we made the decision to enable it, it was to enable it (the hypothes.is plugin) by default and document how to (easily) disable it.

danielweck commented 6 years ago

@llemeurfr I think that the default build of readium-shared-js (which determines which plugins are activated) cannot possibly suit everybody's needs. Most developers actually build their own version of it anyway, as this is not really designed as an off-the-shelf component containing features that can be switched at runtime. There is some compile-time configuration.

danielweck commented 6 years ago

The Chrome app is totally controlled by the Readium management team, so they decide what features are shipped (normally, the highlights plugin is deactivated).

The cloud reader deployed at Firebase and Surge is Readium's own build preference, and it contains Hypothesis instead of the highlights plugin (which did nothing but visually-select text, anyway).

danielweck commented 6 years ago

I see a problem with native apps though. In the GitHub repository (i.e. the actual source tree, develop and master branches), there is a ready-to-use build of readium-shared-js. The intent was / is to provide an "off the shelf" shared-js component that can be directly integrated in a native apps's reader.html (by referencing the pre-built single or multiple AMD bundles). That's how the ReadiumSDK launcher apps work, via Git submodules. Problem: the default readium-shared-js build now contains the Hypothesis plugin, which consequently gets activated in the native launcher apps. Not a big deal for an existing Readium integrator that already compiles its own version of shared-js (thereby overriding the default build config), but really confusing for others. We actually did talk about this with Juan etc. but the more urgent goal was to deliver Hypothesis integration in the mainstream cloud reader, so we put a punt on this issue. See: https://github.com/readium/readium-shared-js/issues/418

rkwright commented 6 years ago

@danielweck Thanks. Sigh. This points up the problem that we don't really know (or have any way to know) how people are using the SDK or the CloudReader. Do we know for a fact who might be using this pre-built version of shared-js?

danielweck commented 6 years ago

And to be clear: Hypothesis is a proper cloud-based annotation service, thus why its default integration in the deployed Readium web reader made sense for our demo purposes.

The highlights plugin is a core utility feature with limited scope, not capable of managing actual annotations. It is meaningless to end-users, and only adds value when developers build functionality on top of it.

danielweck commented 6 years ago

PS: this debate about "which plugin to activate in the default build" is likely to arise again. I think we need a default config for the cloud reader, one for the Chrome app (soon to be deprecated, so perhaps not such a priority), and one of the readium-shared-js build-output that ReadiumSDK native apps use. The rest of the world will have to ensure their configuration meets their deployment needs.

danielweck commented 6 years ago

I have now realised that the reported bug has nothing to do with the highlights plugin that used to be shipped by default in the build of the cloud reader (now superceded by Hypothesis). However this discussion remains relevant, and the title is correct ("Highlight Support in ReadiumJS Disabled?")

danielweck commented 6 years ago

Closed via resolution of https://github.com/readium/readium-js-viewer/issues/658

DougsAraujo commented 6 years ago

@danielweck The hypothesis plugin when enabled in the cson file works normally, however the plugin highlights, enable but does not make the markup, what can I do?

danielweck commented 6 years ago

@DougsAraujo see https://github.com/readium/readium-shared-js/issues/448#issuecomment-385627644