Render a full list of references at the bottom of the note viewer by xUser5000 · Pull Request #16 · joplin/plugin-bibtex

xUser5000 · 2021-07-09T02:00:48Z

What has been done

Register a markdown-it content script to render references at the bottom of the note viewer in APA format.
Render both inline and block-level references.
Parse and store references on startup
Prevent rendering duplicate references
Use the citekey as the display key of references. (Previously I used a combination of author + year of publication).

laurent22

That looks good overall. Your approach to create the reference list token and parse it in particular seems solid. I've just left a few comments.

By the way, could you create a documentation for the format you support? In particular what is this APA thing and what citation formats your plugin supports or creates. For example you have both "Steinbeck2003" and "bu-EtAl:2010:EMNLP". I suppose there's a reason for that, which should be explained in the Readme.

laurent22 · 2021-07-09T21:01:30Z

+        const refs: Reference[] = parse(fileContent);
+        DataStore.setReferences(refs);
+
+    } catch (e) { }


I seem to remember you were previously displaying an error with the line number when the file could not be parsed. Was that removed? In general I think the user should be informed if their file can't be loaded. Can you use alert() here for example? Or joplin.views.dialogs.showMessage?

laurent22 · 2021-07-09T21:02:44Z

+     */
+    await joplin.contentScripts.onMessage(REFERENCE_LIST_CONTENT_SCRIPT_ID, (IDs: string[]) => {
+        let refs: string[] = [];
+        IDs = [...new Set(IDs)];


You're often converting arrays to set and the other way around. Is there any reason for that? Isn't this code pretty much the same as IDS = IDS?

Isn't this code pretty much the same as IDS = IDS?

Not exactly. Previously, if there are multiple occurrences of the same reference, it will be rendered twice at the bottom.

Ok I see. It's not very obvious that it's doing this so maybe add a comment?

laurent22 · 2021-07-09T21:10:31Z

+				return `
+					<h1 id="references_title" style="display:none">References</h1>
+					<ul id="references_list"></ul>
+					<style onload='${script.replace(/\n/g, ' ')}'/>


Any reason you used a style tag for this? Wouldn't it be possible to wrap your script string simply in a <script> tag? I think that would be more proper and perhaps more reliable too.

Wouldn't it be possible to wrap your script string simply in a <script> tag? I think that would be more proper and perhaps more reliable too.

I tried that approach as it seems to be the most natural but for, some reason, it does not seem to work.

Ok, I think this is fragile code that may break at some point or even work in some operating systems but not others. I think it didn't work with the script tag because you need to wait till the "webviewApi" is available. For this you can use the same pattern - just keep checking typeof webviewApi until it becomes available, then run your initialisation code.

Whatever I put in the script tag does not get executed.

I'm wondering if you're looking at all this the wrong way around. The plugin sets the HTML, and has all the references, so instead of adding a script that post messages, can't you directly set the HTML to the correct value from the plugin? Then you won't need a script at all inside the content script.

In the documentation, there is no description of such approach (in markdown-it plugin section)

It's not something about documentation, since it's specific to your plugin. But come to think of it, what I'm suggesting wouldn't work since the reference data is in the plugin process, and the content script in the main process, so there would still be a need for some kind of ipc call. So let's leave it as it is for now.

xUser5000 · 2021-07-09T22:03:56Z

Renamed _context to context and made it show a message box on startup if there is an error parsing the BibTeX file.

xUser5000 · 2021-07-09T22:31:29Z

By the way, could you create a documentation for the format you support? In particular what is this APA thing and what citation formats your plugin supports or creates. For example you have both "Steinbeck2003" and "bu-EtAl:2010:EMNLP". I suppose there's a reason for that, which should be explained in the Readme.

Yeah sure, before releasing the new version, I'm gonna do that in addition to updating the main GIF that shows the working mechanism of the plugin.

laurent22 · 2021-07-10T11:53:12Z

+						const referenceListView = document.getElementById("references_list");
+						const referenceTitleView = document.getElementById("references_title");
+						if (refs.length > 0) referenceTitleView.style.display = "block";
+						refs.forEach(ref => referenceListView.innerHTML += ref);


Instead of setting innerHTML multiple times, which is slow, you could build up the HTML string in the loop, then set the inner HTML once at the end.

My bad, I keep doing these silly mistakes 😃

xUser5000 · 2021-07-10T12:26:37Z

DONE

xUser5000 added 8 commits July 8, 2021 13:28

fix(Format Reference Util): Use id as the display text

1f52671

feat(APA Util): Convert JSON to APA

2cced05

test(APA Util): Add a test case

f62a032

feat(Content Script): Render full references at the bottom

5c8529e

style(Content Script): Added some comments

57edd51

fix(Content Script): Render inline and block-level references

511c4f6

fix(Content Script): Parse and store references on startup

cf30640

fix(Content Script): Don't render duplicate references

fbab4c1

laurent22 requested changes Jul 9, 2021

View reviewed changes

refactor(Content Script): Rename _content to context

7bad62a

feat(Init): Show alert message on startup

4bb5da0

laurent22 reviewed Jul 10, 2021

View reviewed changes

xUser5000 added 2 commits July 10, 2021 14:14

style(Content Script): Add clarifying comments

7500981

perf(Content Script): Set innerHTML once

e91c766

laurent22 merged commit 5f49ed3 into main Jul 10, 2021

xUser5000 mentioned this pull request Jul 10, 2021

Release 0.3.0 #17

Merged

Conversation

xUser5000 commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What has been done

Uh oh!

laurent22 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xUser5000 commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xUser5000 commented Jul 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xUser5000 commented Jul 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xUser5000 commented Jul 9, 2021 •

edited

Loading

xUser5000 commented Jul 9, 2021 •

edited

Loading