add a new tab for embedded previews with google books functionality

[hackartisan]

refs https://vufind.org/jira/browse/VUFIND-1009

[demiankatz]

Thanks for sharing this -- I'll try to find time for some thorough testing later in the week. In the meantime, just one observation based on a glance at the diffs: I think there may be a problem with the use of "embedded_preview". Generally speaking, our service names correspond with our class names -- so why not register/configure the tab as simply preview rather than embedded_preview? In one context, it appears that embedded_preview may be intended as an on-screen label, but if this is the case, the new line seems to be missing from en.ini.

I don't think any of this will prevent me from testing later, but feel free to push up adjustments in the meantime if you see fit.

Thanks again!

[hackartisan]

I think I was just trying to be clear in the config file which preview I meant. But sure, I am happy to change that. I don't think it would be used as display text anywhere. Please correct me if I'm wrong.

Since pushing this PR I have found that I have a need to separate the data that powers the previews (config info, id numbers, etc) from the presentation of the link itself. This is because we've moved the link down into the holdings tab. So I need the data to still be accessible on the main record view or else our preview tab will disappear when you click away from holdings. I'm working to split this out now; I hope to do so in a way that is acceptable to the project as a whole.

[demiankatz]

My main concern is that the service names and classes are consistent... so I'll leave it up to you whether you're happier with an embeddedpreview service that points to an EmbeddedPreview class, or simply preview and Preview. Either one makes me happy.

You're right that embedded_preview is not being used as display text in your code -- I was forgetting the exact shape of the tab definition configuration.

I'll wait on further review until you finish adjusting the data to meet your needs -- please post another comment here to remind me to take another look once you've got things working the way you want.

[hackartisan]

Okay see how this strikes you! Thanks,Demian!!

[hackartisan]

Oh I should note I only modified the bootstrap theme. I didn't want to go crazy before I got your feedback.

[demiankatz]

Thanks again. A few comments:

1.) While it's unlikely that anyone has modified preview.phtml, we should remember to put a note about its elimination in the changelog after this is merged.

2.) The "embedded_preview" text is still being used in a couple of places related to the hide_if_empty setting; I think this should be changed to preview to match everything else.

3.) The getPreviewIds() method is missing a PHPDoc comment.

4.) Is there any advantage to making the preview tab visible if the hide_if_empty setting is absent? I understand this would be the most consistent implementation, but if the actual effect of enforcing this behavior is that we display an unnecessary and/or broken tab to certain users, it might be better to simply force a non-visible initial state rather than worrying about reading the configuration.

Still haven't tried the code yet, but I'll test it out after you reply to this round of comments. We should also likely ask Eoghan to take a look at this when it's closer to completion, as he's pretty familiar with the preview code and may have some additional useful feedback.

[demiankatz]

Okay, I'll try to keep on top of my end of this. I'll also give Eoghan a heads-up now so he can offer feedback sooner rather than later.

[hackartisan]

Thank you for looking!!

I'm happy to do away with the hide_if_empty bit; this functionality does not add unnecessary data loads so there is really no downside to having it initially hidden. And that will take care of your hated embedded_preview text.

I would love to get this merged this this week since i'm out next week and we're launching our new codebase a week after that! Of course I can also pull these changes locally so it's not the end of the world if it doesn't work out

I'll comment when I make another push.

P.S. reposted out of order I guess since it looks like you just responded -- I accidentally deleted because gh keeps making it look like I've double-posted

[demiankatz]

Yeah, the GitHub comment posting is very glitchy.

[hackartisan]

commit pushed. Thanks!!

[demiankatz]

A couple more small issues from another code review:

1.) Is there a strong reason to change SolrDefault/core.phtml? Since you made getPreviews() backward compatible, why not just leave this file as-is and leave the more granular methods available for customizations as needed.

2.) The current logic in record/view.phtml can result in the tab having multiple class attributes. That's not technically legal and may be interpreted differently by different browsers -- that logic should be adjusted so that you get a single class attribute containing one or more classes.

I'm now going to try running the code, regardless of these things... but wanted to mention them now so you can start fixing them if you like. I'll report back after I've finished testing.

[demiankatz]

A few more thoughts/questions:

1.) I'm not totally sure I understand the purpose of the noview/partial/full settings -- based on experimentation, it seems like using the "partial" setting also includes "full" books, but if that's the case, I don't know what the point of the "full,partial" setting might be. In any case, there seems to be some inconsistent application of the settings, since if I set both GoogleOptions['link'] AND GoogleOptions['tab'] to noview, then I don't seem to get any preview links on the result list, but I still have a preview tab visible for items that can be previewed. Are these settings not getting processed correctly?

2.) I can imagine that some libraries might want the preview button to link directly to the preview tab rather than going off-site. I don't want to make unnecessary headaches for you, but just something that occurred to me. Maybe something to record in a JIRA ticket if you don't feel like dealing with it right now.

3.) I notice that the Google Books preview doesn't respect the current selected language. Perhaps it's an English-only API... but I know that for maps, we're able to pass the language code over for internationalization support. Do you happen to know if that's possible here as well?

Okay, on to more testing... I'll let you know if anything else arises.

[demiankatz]

A more serious problem: this code somehow seems to have broken OpenLibrary and HathiTrust previews. If I apply it, I don't get any of them. If I revert it, they come through. Are you able to debug this on your end, or do you need assistance from here? I can email you a MARC record that yields previews in all three places if that would be helpful.

[hackartisan]

Hey Demian,
I will look now at OpenLibrary and HathiTrust. As long as I don't have to have any special membership to access that data (which it doesn't look like I do) I should be able to test. Thank you for the record.

Regarding noview/partial/full settings:
a) It makes sense to me that partial would also include full, however that is not specified / guaranteed in the google documentation. Especially with 'noview' also in the mix, I think the parsing should be consistent to account for all possible combinations.
b) For the tab to show up, the preview must also be embeddable. I doubt a preview with "noview" viewability is going to be embeddable. This probably explains the seemingly inconsistent situation you saw.

Regarding sending the link to the tab:
This may cause problems when things aren't embeddable (see above). It is not a part of my use case so I don't really have ability to tackle this, but if someone wants to take it up in the future I would be inclined to wait for them to open a jira ticket about it.

Regarding internationalization:
This is a good idea and yes it should be possible. (see https://developers.google.com/books/docs/viewer/developers_guide#Localization). It's just a matter of adding a parameter to the load command in gbsPreview.js. But I'm not sure where/how to pull the data.

[demiankatz]

Regarding the noview problem, I agree that it would make sense if we saw a preview button but no tab in this situation, but I'm actually seeing the reverse. When I specify "noview" as my only option, I don't seem to get any preview buttons at all, but I still get tabs for some items (those that also show up under full). Is it possible that only "embeddable" status is getting checked and level is being ignored by the tab? Or maybe it's some other quirky Google behavior. I just don't understand why the tab would be more INCLUSIVE; I'd expect the reverse.

Regarding sending link to tab -- I'm fine with holding off on that. Just an idea that seemed worth writing down.

Regarding internationalization, you should be able to pull the current language from the $this->layout()->userLang variable in the PHP templates. In most themes (except jquerymobile), it should be set as the lang attribute of the top tag, so that might be an approach for pulling it out of the document when in JS code (and we should probably add it to jquerymobile for consistency).

[hackartisan]

just pushed the hathi / ol fix.

[hackartisan]

I think I changed SolrDefault/core.phtml just so it was sort of on the record somewhere in the code that it could be split out. Is that a good reason?

[demiankatz]

That's not a bad reason, but the two lines seem less clear to me when reading the code than the one line did. How would you feel about leaving the old line, but putting a comment above it saying "You can split this out with methods x and y if you need to move the link to a different location"?

[demiankatz]

Thanks also for the Hathi / OL fix. I'm about out of time for today, but I'll give this another look tomorrow (unless you end the day with things unfinished and prefer that I wait).

[hackartisan]

If the preview tab is not active but has been made visible it looks like <li class> is that bad?

[hackartisan]

Okay you're right there was a bug with tab visibility; fix just pushed.

I believe the only thing outstanding from this conversation thread is the internationalization bit. Please correct me if I missed anything else!

note to self: check format of language info. Currently supported RFC 3066 language codes include hy, bg, ca, zh-CN, zh-TW, hr, cs, da, nl, en, fil, fi, fr, de, el, hi, hu, is, id, in, it, ja, ko, lv, lt, no, pl, pt-BR, pt-PT, ro, ru, sr, sk, sl, es, sv, th, tr, uk, and vi.

[eocarragain]

I had a read through the diff and it looks good to me. Nothing useful to add beyond that, I'm afraid. Thanks!

[demiankatz]

Eoghan,

Thanks for the review!

[demiankatz]

Anna,

Regarding the < li class > issue you mention, is that a Javascript side effect? The template changes you've made look fine to me and don't seem likely to cause that situation.

Regarding language codes, those look compatible with the ones used by VuFind. Probably worth testing what happens if you pass in an invalid code, though, just for the sake of future compatibility.

Beyond that, I don't think you've missed anything. I'll do more testing once I've caught up with the morning email.

[hackartisan]

Yes, when the javascript toggles the 'hidden' class and that was the only class, it just says <li class>. I guess that's okay or I assume jquery wouldn't let you do it (in jquery we trust).

[hackartisan]

Thank you for taking a look, Eoghan. And thank you, Demian, for catching bugs.

[demiankatz]

I'm willing to have faith in jQuery in this instance as well.

[hackartisan]

Here's the internationalization piece. Tested with bad lang code; the preview loads fine using English.

[demiankatz]

Just in time for me to start testing -- thanks!

How do you want to handle the other themes? Do you have time to port this, should I work on that today, or do you want to split them somehow?

I don't expect to find major problems at this point, so if you want to start porting in parallel with my last round of testing, that might save a little time -- bootstrap --> bootstrap3 is probably the easiest place to start.

[hackartisan]

I'll work on bootstrap3. man, there are a lot of themes.

[demiankatz]

Regarding Primo, it sounds like it probably doesn't hurt to add the tab, though we have no easy way of testing.

Regarding PHPCS, I'll be happy to do the fixes on this end. If you add me as a contributor to your repo, I can just push them up directly. Otherwise, I'll make a note and fix them after I merge the PR.

Regarding internationalization, I was actually referring to the target of the link rather than the icon, but it's great to know that the icon itself can also be changed (and I see that putting garbage codes in that URL still yields a valid image, so there's no harm in making that enhancement).

Chris fixed the bootstrap3 problem and pushed it to master. If you merge master into this branch, you should be able to finish up your bs3 port.

I'll take care of jquerymobile shortly.

Thanks for all of your work on this!

[demiankatz]

Anna,

jquerymobile turned out to be easy -- the mobile theme doesn't support previews, so I just put in enough code to prevent the new mechanism from breaking anything.

I also took care of the style fixes.

I've opened a pull request against your branch with these adjustments. Please merge it when you get a chance.

I'll stand by for completion of the bootstrap3 theming, and then I think we're ready to merge to master. Thanks again!

[hackartisan]

grabbed Chris' changes via rebase instead of merge. Everything is set -- will it be a problem for me to force push??

[demiankatz]

No, go ahead and force-push.

[hackartisan]

All set! Thank you, Demian!!!

[demiankatz]

Great -- I'll get this merged to master either later today or tomorrow, depending on how my schedule works out.

[demiankatz]

(I plan to flatten the commit history -- hopefully that won't be a problem for you).

[hackartisan]

No problem!

[demiankatz]

Okay, merged to master! A few related enhancements to come as separate commits (config upgrade, i18n improvements, etc.)