Adding entry to title context menu

[Emiliano Heyns]

In Z6 BBT adds an entry to the title context menu which offers the BBT title-caser for use on the title, which I did by adding an entry to zotero-field-transform-menu. That doesn't seem to work in Z7. Any leads on how to do this in Z7?

[Abe Jellinek]

The item box is a CE instead of an XBL binding now, but #zotero-field-transform-menu is still there and adding a menuitem to it should do the same thing it always did. You’ll need to be more specific about what you’re doing and what isn’t working, but easiest would probably be to take a look at the DOM in the Firefox devtools and check if your menuitem is actually being added, whether it has the right namespace, and so on.

On Jul 31, 2023, at 3:56 PM, Emiliano Heyns <emilian...@iris-advies.com> wrote:

In Z6 BBT adds an entry to the title context menu which offers the BBT title-caser for use on the title, which I did by adding an entry to zotero-field-transform-menu. That doesn't seem to work in Z7. Any leads on how to do this in Z7?

--
You received this message because you are subscribed to the Google Groups "zotero-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to zotero-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/zotero-dev/3fc59b54-72f6-4183-bc05-8e73b87ec918n%40googlegroups.com.

[Abe Jellinek]

(Yes, looking at your code, you’re creating an HTML element when you should be creating a XUL element: https://github.com/retorquere/zotero-better-bibtex/blob/ec6ae3e169245d76ff18e9004cd7f03f2a9e6a4d/content/ZoteroItemPane.ts#L64C1-L69)

On Jul 31, 2023, at 3:56 PM, Emiliano Heyns <emilian...@iris-advies.com> wrote:

In Z6 BBT adds an entry to the title context menu which offers the BBT title-caser for use on the title, which I did by adding an entry to zotero-field-transform-menu. That doesn't seem to work in Z7. Any leads on how to do this in Z7?

[Abe Jellinek]

Never mind, I guess elements.create() defaults to XUL? In any case, need to be more specific.

[Emiliano Heyns]

The DOM looks like this after my code ran:

<menupopup xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="zotero-field-transform-menu"><menuitem id="creator-transform-title-case" label="Title Case" class="menuitem-non-iconic"/><menuitem id="creator-transform-sentence-case" label="Sentence case" class="menuitem-non-iconic"/><menuitem label="BBT sentence-case" id="better-bibtex-transform-sentence-case" class="better-bibtex-KT5Z7GZP menuitem-non-iconic"/></menupopup>

but the menu popup doesn't show my new entry.

[XY Wong]

• createElement()/createElementNS() → createXULElement() for remaining XUL elements (example)

I don't think your elements.create really does it right when creating XUL elements.

[XY Wong]

Also, if you want to have a type hint for the `create` return and auto-detect namespace, e.g. create("menuitem") returns a XUL.MenuItem and create("div") returns an HTMLDivElement, you could look into this:

https://github.com/windingwind/zotero-plugin-toolkit/blob/ca24c8183bfeed24a32a6a64687cc3dbc88bb88e/src/tools/ui.ts#L165-L301

This implementation does not require manually defining most elements' namespaces.

[Emiliano Heyns]

I've modified elements.create to use createXULElement (how did you get the grey boxes around them here in google groups BTW?) but the menu entry still does not show up.

wrt the return type, the magic happens here I think? I have no idea how the typescript compiler figures this out, but I'll take it!

[XY Wong]

The grey box is copy-paste from the doc of Zotero. Probably some HTML style.

I'm not sure what happens to your menuitem. Are they in an XHTML window?

The return type is actually defined here: https://github.com/windingwind/zotero-plugin-toolkit/blob/ca24c8183bfeed24a32a6a64687cc3dbc88bb88e/src/tools/ui.ts#L59-L151

There are multiple function signatures, the TS compiler checks the input type in the following order: fragment->HTML->XUL->SVG->any according to the pre-defined types.

[Emiliano Heyns]

Ah OK, so it's just the return type declaration on the function. The TS compiler doesn't figure out based on how it's called which of them is actually returned.

the menu is in the itembox in the zoteroPane. I'm not sure whether that is an XHTML window, but given that my menuitem looks like the other menuitems, I'd have expected it to show. I've looked at the code in itemBox.js and I don't see anything there that would hide it. It does work in Zotero 6 BTW.

[XY Wong]

The dynamic return type is achieved by Generics. TS compiler knows the return type, but it is not directly inferred from the actual code.

I can create a menu item there via console call. Weird it does not work in your case.

[Emiliano Heyns]

Thanks for checking it out. It's a papercuts issue for Zotero 7 support, at some point I'll figure it out, but it's not a blocking issue.

[Emiliano Heyns]

Why are the menuitem IDs `creator-transform-title-case` and `creator-transform-title-case` BTW? These don't apply to creator fields do they?

[Emiliano Heyns]

With this the extra line sometimes show up, but I have no idea yet under what circumstances precisely.

const transform_menupopup: any = this.document.getElementById('zotero-field-transform-menu')
    const bbt_sentencecase_id = 'better-bibtex-transform-sentence-case'
    transform_menupopup.addEventListener('popupshowing', () => {
      Zotero.debug('popup showing: running')
      let bbt_sentencecase: any = this.document.getElementById(bbt_sentencecase_id)
      if (!bbt_sentencecase) {
        bbt_sentencecase = transform_menupopup .appendChild(
          this.elements.create('menuitem', {
            label: 'BBT sentence-case',
            id: bbt_sentencecase_id,
            class: 'menuitem-non-iconic',
            oncommand: () => { textTransformField.call(this.itemBoxInstance, (this.document as any).popupNode) },
          })
        )
      }

      bbt_sentencecase.disabled = !canTextTransformField.call(this.itemBoxInstance, (this.document as any).popupNode)
    })

I'd be fine with just offering the BBT sentence-caser to Zotero to circumvent this problem. It's not big (90 LoC), but it needs XRegExp. One thing that can be done in just a few LoC is exempt <span class="nocase"> parts from sentencecaseing, but the BBT sentencecaser does some work to properly treat words that have internal markup as a single word. If Zotero does npm modules (I think it does?) there wouldn't even need to be much of a PR, because this would do it:

import { toSentenceCase } from '@retorquere/bibtex-parser'

function sentenceCase(text) {
  let sentencecased = toSentenceCase(text)

  // restore protected parts from original
  text.replace(/<span class="nocase">.*?<\/span>|<nc>.*?<\/nc>/gi, (match: string, offset: number) => {
    sentencecased = sentencecased.substr(0, offset) + match + sentencecased.substr(offset + match.length)
    return match
  })

  return sentencecased
}

but that would require tree-shaking to get rid of the bibtex parser. And I could well imagine Zotero wouldn't like being dependent on modules out its control. So a PR would be a fine route for me.

Another option would be to monkey-patch itemBox.textTransformString(val, 'sentence'), but I'm not so keen on replacing stock behavior these days.

[Dan Stillman]

On 8/1/23 8:13 PM, Emiliano Heyns wrote:
> I'd be fine with just offering the BBT sentence-caser to Zotero to
> circumvent this problem. It's not big (90 LoC), but it needs XRegExp.

We'd happily take a PR with improvements to the sentence-caser. We have
XRegExp already, but I'd guess that it isn't required in Zotero 7.

And yes, "creator-transform-" was just copied from the creator menu
options by mistake.

[Emiliano Heyns]

Not required in the sence that the xregexp syntax is natively supported in Zotero 7? Or something else? 

[Dan Stillman]

On 8/2/23 3:28 AM, Emiliano Heyns wrote:

Not required in the sence that the xregexp syntax is natively supported in Zotero 7?

Yes, but it's not "XRegExp syntax" — XRegExp was just a polyfill for various newer regex features that hadn't yet been implemented by newer browsers. Judging by the docs, Firefox has supported everything for years at this point, so I think we can just remove XRegExp entirely.

[Emiliano Heyns]

Ah superb! What branch should I target with my PR? Is there a linter I can use to make sure I match the code style?

[Dan Stillman]

On 8/2/23 6:22 PM, Emiliano Heyns wrote:
> Ah superb! What branch should I target with my PR? Is there a linter I
> can use to make sure I match the code style?

'main' branch, and yes, there's an eslint config. Thank you!

[Emiliano Heyns]

My pleasure. I don't currently get output from eslint after running `npm i` and `npx eslint`, and I don't see a script in the package.json that runs eslint.

[Emiliano Heyns]

Not yet supported in Zotero 6 alas, so I can't yet strip it out of BBT. But for the Zotero 7 main branch no issue.

[Emiliano Heyns]

@Dan I'm still stuck on this.

I'm not sure whether it's some technical difficulties I'm encountering, or that I've been going against forum etiquette the past week, but messages of mine keep getting deleted. This "I'm stuck" message was just deleted, so I'm reposting it, but the message I'm responding to ("I don't currently get output from eslint") was one of them, and I reposted that too. If these both run against etiquette, it was not done intentionally.

[Dan Stillman]

On 8/5/23 6:54 AM, Emiliano Heyns wrote:
> I'm not sure whether it's some technical difficulties I'm
> encountering, or that I've been going against forum etiquette the past
> week, but messages of mine keep getting deleted. This "I'm stuck"
> message was just deleted, so I'm reposting it, but the message I'm
> responding to ("I don't currently get output from eslint") was one of
> them, and I reposted that too. If these both run against etiquette, it
> was not done intentionally.

We haven't deleted any messages of yours. I've only received the
messages of yours that currently show in the web interface. No idea what
the deleted ones are.

I'm also not clear what you're saying about eslint. Running `eslint`
without arguments isn't something that's expected to do anything.
There's an eslint config in the zotero/zotero repo, and a modern editor
should flag lines as appropriate. You can also run `eslint` with a file
path, but that's not something we'd normally do, since most files still
have old code with many rule violations.

[Emiliano Heyns]

I'm afraid "any modern editor" does not include text-console nvim, which is what I use day-to-day :). I'll see what I can do setting up VSCode.

[Dan Stillman]

On 8/5/23 7:07 AM, Emiliano Heyns wrote:

I'm afraid "any modern editor" does not include text-console nvim, which is what I use day-to-day :). I'll see what I can do setting up VSCode.

I'm sure it's possible to get eslint working in nvim (e.g., with this), but running VSCode for a few minutes is likely the easiest option, yes.

(I use a decidedly non-modern editor and have managed to get it running eslint on the current file, albeit considerably less elegantly than in VSCode.)

[Emiliano Heyns]

OK, I have it all linted, are there unit tests I can run?

[Dan Stillman]

On 8/5/23 8:02 AM, Emiliano Heyns wrote:
> OK, I have it all linted, are there unit tests I can run?

./test/runtests.sh runs ~1700 tests, but I doubt you affected anything
that's covered, so I wouldn't worry about it. Since this has more
sophisticated logic, we'll likely want tests for this, but we can deal
with that on the PR.

[Emiliano Heyns]

Alright, posted at https://github.com/zotero/zotero/pull/3251. It reports a merge conflict which I don't understand; the part that's supposedly conflicting is the part I am replacing.

[Emiliano Heyns]

I copy-pasted this from my bibtex parser, for which I wrote the sentence-caser. It does pass the 300-odd tests there, but I had wanted to run the Zotero tests if only to see I hadn't made an embarrassing typo somewhere leaving invalid code. I've tried runtests.sh, but it errors out with

Error: Cannot find module './lib/colors'
Require stack:
- ~/github/zotero/node_modules/colors/safe.js

- ~/github/zotero/js-build/build.js

and indeed ~/github/zotero/node_modules/colors/lib/colors does not exist.

[Dan Stillman]

On 8/5/23 8:41 AM, Emiliano Heyns wrote:
> I had wanted to run the Zotero tests if only to see I hadn't made an
> embarrassing typo somewhere leaving invalid code. I've tried
> runtests.sh, but it errors out with
>
> Error: Cannot find module './lib/colors'
> Require stack:
> - ~/github/zotero/node_modules/colors/safe.js
> - ~/github/zotero/js-build/build.js

Did you actually build and run the app?

https://www.zotero.org/support/dev/client_coding/building_the_desktop_app

[Emiliano Heyns]

Ah, I didn't, but perhaps that's also not necessary if it moves to utilities. I'm on top of the simplification right now, so that first.

[Emiliano Heyns]

New pull request at https://github.com/zotero/utilities/pull/26