Fix broken getCiteKey() resulting from recent BBT update

[RoBaaaT]

A recent change in the BetterBibTex plugin (https://github.com/retorquere/zotero-better-bibtex/commit/63028dcd03ada843d6f55dfd6745bc92d6587ab2, included in version 6.7.132, released on 2023-10-23) renamed 'citekey' to 'citationKey' in the BetterBibTex source code. This broke the getCiteKey() function in zotero-mdnotes, resulting in exports failing when the 'Use the item's citekey as title' option was enabled by the user (see #224).

This change fixes the problem by updating getCiteKey() to access 'citationKey' instead of 'citekey' if 'citekey' is undefined.

PS: Sorry about the whitespace changes, those were done by my editor. I can resubmit the pull request without them if you prefer.

[LucyDYu]

Before this PR gets approved, you can simply clone the repo, manually add the changes that @RoBaaaT made (just two lines. Thank you for this important fix!) and execute the following commands: cd zotero-mdnotes zip -r ../zotero-mdnotes.xpi *. Then install this updated plugin in zotero. Works for me to solve the citekey issue.

[RoBaaaT]

@retorquere Thanks for your suggestion! However that would not work for users who still have older BBT versions with citekey installed (probably not very common but I imagine that it could happen), right? How about this:

function getCiteKey(item) {
  if (Zotero.ItemFields.getID('citekey') !== false)
    return item.getField('citekey');
  // note: 'citekey' has been renamed to 'citationKey' in BBT - https://github.com/retorquere/zotero-better-bibtex/commit/63028dcd03ada843d6f55dfd6745bc92d6587ab2
  if (Zotero.ItemFields.getID('citationKey') !== false)
    return item.getField('citationKey');
  return undefined;
}

That way we do not even have to check for the existence of Zotero.BetterBibTeX directly, we just check indirectly by looking for the existence of the respective ItemFields.

[retorquere]

@retorquere Thanks for your suggestion! However that would not work for users who still have older BBT versions with citekey installed (probably not very common but I imagine that it could happen), right?

I'm trying to think of a situation where it wouldn't, as getField has done the same for citekey as citationKey for a long time going back, but I can't think of any. But even it that were the case, how would it be better to cater to people that don't upgrade BBT at the expense of people who do?

In any case I would prefer it very much if people would not go around encouraging others to run outdated versions of BBT. Zotero auto-updates plugins by default for a reason.

How about this:

function getCiteKey(item) {
  if (Zotero.ItemFields.getID('citekey') !== false)
    return item.getField('citekey');

I don't thinkZotero.ItemFields.getID('citekey') has ever worked, and it certainly won't work in the future. The test I proposed has two parts because the first part (Zotero.BetterBibTeX) tests for BBT-provided citation keys, and the other part (Zotero.ItemFields.getID('citationKey') !== false) tests for future citation key support even in the absence of BBT.

But come to think of it -- Zotero already has citationKey support for itemtypes dataset, preprint and standard, so that test will always return true. getField without BBT installed will likely error out when requesting it for any itemtype except those though, so perhaps the best way is to just do

try {
  return item.getField('citationKey')
} catch (err) {
  return undefined
}

because item.getField('citekey') and item.getField('citationKey') have been equivalent for a long time. Current BBT still supports both, and I have no plans to retire that. It's just internally that I've switched to citationKey in preperation for the formal Zotero citationKey field.

That way we do not even have to check for the existence of Zotero.BetterBibTeX directly, we just check indirectly by looking for the existence of the respective ItemFields.

That already works, and the try-catch method doesn't even have to know or care about BBT being present, now and in the future.

[RoBaaaT]

Thanks for the detailed feedback! Using try-catch indeed seems like the best way to do it. I've updated the pull request accordingly.

[JannisBush]

Before this PR gets approved, you can simply clone the repo, manually add the changes that @RoBaaaT made (just two lines. Thank you for this important fix!) and execute the following commands: cd zotero-mdnotes zip -r ../zotero-mdnotes.xpi *. Then install this updated plugin in zotero. Works for me to solve the citekey issue.

This will not work as src/markdown-utils.js cannot be found when zipping the folder directly after applying the change. Either run make or manually copy src/markdown-utils.js to content/markdown-utils.js before zipping.

Thanks for the fix :)

[retorquere]

you can take the xpi, unzip that (xpi's are just zipfiles), make the change, and re-zip the xpi.

[ananth1996]

you can take the xpi, unzip that (xpi's are just zipfiles), make the change, and re-zip the xpi.

I tried this and it works perfectly now. What's the timeline for the PR getting accepted?

[DivingWhale]

Thank you, RoBaaaT, for your excellent work. Could author please update this fix in the Releases? It's not easy to access this fix at the moment.

[RojarMain]

Tried implementing

@retorquere Thanks for your suggestion! However that would not work for users who still have older BBT versions with citekey installed (probably not very common but I imagine that it could happen), right?

I'm trying to think of a situation where it wouldn't, as getField has done the same for citekey as citationKey for a long time going back, but I can't think of any. But even it that were the case, how would it be better to cater to people that don't upgrade BBT at the expense of people who do?

In any case I would prefer it very much if people would not go around encouraging others to run outdated versions of BBT. Zotero auto-updates plugins by default for a reason.

How about this:

function getCiteKey(item) {
  if (Zotero.ItemFields.getID('citekey') !== false)
    return item.getField('citekey');

I don't thinkZotero.ItemFields.getID('citekey') has ever worked, and it certainly won't work in the future. The test I proposed has two parts because the first part (Zotero.BetterBibTeX) tests for BBT-provided citation keys, and the other part (Zotero.ItemFields.getID('citationKey') !== false) tests for future citation key support even in the absence of BBT.

But come to think of it -- Zotero already has citationKey support for itemtypes dataset, preprint and standard, so that test will always return true. getField without BBT installed will likely error out when requesting it for any itemtype except those though, so perhaps the best way is to just do

try {
  return item.getField('citationKey')
} catch (err) {
  return undefined
}

because item.getField('citekey') and item.getField('citationKey') have been equivalent for a long time. Current BBT still supports both, and I have no plans to retire that. It's just internally that I've switched to citationKey in preperation for the formal Zotero citationKey field.

That way we do not even have to check for the existence of Zotero.BetterBibTeX directly, we just check indirectly by looking for the existence of the respective ItemFields.

That already works, and the try-catch method doesn't even have to know or care about BBT being present, now and in the future.

Hi, I tried implementing this. (downloaded .xpi, renamed .zip, changed getCitekey to try-catch, and reconverted back to .xpi), but this did not work. Did i miss/skip a step? Thanks!

[retorquere]

Can't say, sorry. As far as I can tell, that should work. You'd need @argenos to get involved.