Skip to content

Citation Popup: Search through the list of references using a given query#12

Merged
laurent22 merged 26 commits intomainfrom
search-citation-popup
Jul 3, 2021
Merged

Citation Popup: Search through the list of references using a given query#12
laurent22 merged 26 commits intomainfrom
search-citation-popup

Conversation

@xUser5000
Copy link
Copy Markdown
Contributor

What has been done

  • Pass reference data as JSON to the citation popup.
  • Integrate AutoComplete.js: I found it to be more suitable for the task than Typeahead.js. In addition, it has a lot of customizations with zero dependencies.
  • Enable selecting references from search results.
  • Enable removing references from the selected area.
  • Prevent showing already-selected references in the search results.
  • Return array of selected references and insert them into the note body.

Demo

Demo

Bugs

The following bugs still need to be solved:

  • Inserted references often don't go to the desired location in the note body; some of them go to the desired location and the rest is inserted at the top of the note body.
  • Enter key does the functionality of the "OK" button; It should select the highlighted reference in the search results.

Copy link
Copy Markdown
Member

@laurent22 laurent22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see you are making good progresses. While you develop, could you also test that your code works with large BibTex files? I would suggest finding a 1GB file and see how it performs as that would be a good way to stress test your code. It's better to do this from time to time as you make progresses, because it will inform how you should develop certain parts of the plugin.

Comment thread src/add-bibtex-reference.command.ts Outdated
Comment thread src/ui/citation-popup/index.ts Outdated
Comment thread src/ui/citation-popup/index.ts Outdated
Comment thread src/ui/citation-popup/view.js Outdated
Comment thread src/ui/citation-popup/view.js
@xUser5000
Copy link
Copy Markdown
Contributor Author

xUser5000 commented Jul 1, 2021

  • I looked into different libraries and frameworks (like Svelte, Stimulus, Bind-dom ) that do state-based UI rendering but I felt like they are overkill when compared to the task at hand. Based on that, I decided to write it myself. Performance-wise, it is not that great, but I think it is more extendable and clean than before. I don't know if this is the best thing to do.
  • In addition, I solved the bug related to the wrong cursor position.
  • There was also another bug that I found when stress-testing the code with ~1200 BibTeX references; the bug was that the parsing library does not guarantee that there will be a property issued.["date-parts"]. Accordingly, when calling the getDate() function on a reference that has the property undefined, it throws an error saying "Cannot read properly 0 of undefined". I solved this bug by simply checking if the desired property is defined or not.

Comment thread src/ui/citation-popup/view.js Outdated
Comment thread src/ui/citation-popup/view.js Outdated
Comment thread src/ui/citation-popup/view.js Outdated
Comment thread src/ui/citation-popup/view.js Outdated
@xUser5000
Copy link
Copy Markdown
Contributor Author

Regarding the difference between maps and plain js objects, it turns out that objects are much faster than maps in initialization and assigning values to keys. Objects are not as flexible as maps, but, in this situation, I don't that much flexibility (e.g getSize(), delete()); I only need to add key-value pairs and then look up the reference that corresponds to a given key. In conclusion, plain objects are better suited for this task.

@xUser5000
Copy link
Copy Markdown
Contributor Author

I've replaced the map with a plain js object and fixed minor UI bugs.
Now my question is: How to implement escaping in view.js? Do I download a new library just for that or? I also want to know whether or not this is necessary because I already escape the JSON object passed to the view.

Comment thread src/ui/citation-popup/view.js Outdated
Comment thread src/ui/citation-popup/view.js Outdated
@laurent22
Copy link
Copy Markdown
Member

Now my question is: How to implement escaping in view.js?

That's all you need:

<li id="${encode(ref["id"])}">

@xUser5000
Copy link
Copy Markdown
Contributor Author

That's all you need:

<li id="${encode(ref["id"])}">

But I don't have access to html-entites from the view, so there are three options at hand:

  • Download a new library specifically for this.
  • Write a basic encode() function from scratch.
  • Let the DOM handle the escaping for me. For example,
const li = document.createElement("li");
li.textContent = ref["id"];   // Auto-escaping

@laurent22
Copy link
Copy Markdown
Member

But I don't have access to html-entites from the view, so there are three options at hand:

All these options are fine so it's up to you.

@xUser5000
Copy link
Copy Markdown
Contributor Author

  • Not encoding the JSON string passed by citation-popup/index.ts was causing a bug. For instance, if I try to include a title like "This is an <art", it gets stringified to JSON peacefully but it breaks the HTML in the view. So, I had to encode the JSON string before passing it, then decode it when receiving it in the view.
  • I used he.js to encode and decode data inside the view. With that said, I encountered a bug that was mentioned here by Ahmed and solved it the same way he did (by calling setTimeout() for a fraction of a second).
  • In addition to the above, I did extra minor things like refactoring loading assets into a separate function in citation-popup/index.ts and made the search field empty its contents after the user adds a reference to the selected area.

Comment thread src/ui/citation-popup/view.js Outdated
waits until all the other scripts get loaded by Joplin,
and then starts doing its job
*/
setTimeout(main, 5, 10);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should poll for what you need instead, because this hack might work on some computers but not others, if they are too slow for example.

const intervalId = setInterval(() => {
	if (typeof he !== 'undefined') {
		clearInterval(intervalId);
		main();
	}
}, 500);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way it's normal scripts might not be immediately available. This is like a web page and everything is loaded asynchronously.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

Comment thread src/ui/citation-popup/view.js Outdated
Comment on lines +20 to +21
const invertedRefsIndex = {};
refs.forEach(ref => invertedRefsIndex[ ref["id"] ] = ref);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that again for optimisation purposes? Because I'm certain it makes no difference. If you need a ref by ID, just use refs.find(r => r.id === refId). Duplicated data is always a bad idea because it needs to be kept in sync.

// Modify Results Item Content
item.innerHTML = `
<span style="text-overflow: ellipsis; white-space: nowrap; overflow: hidden;">
${ data.match }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you escape this too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH, I forgot to mention that data.match is a DOM element that's responsible for highlighting the parts of the title that match the search query, so we can assume that it does escaping by default (I guess because it uses textContent internally). Also, it didn't break when I used the title 'This is an <art".

Comment thread src/ui/citation-popup/view.js Outdated
function main () {

/* State */
const refs = JSON.parse( he.decode(inputRefsView.textContent) );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to decode here - textContent is plain text, not HTML.

);

const refs: Reference[] = DataStore.getAllReferences();
html = html.replace("<!-- content -->", fromRefsToHTML(refs));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, did you check how it works with a 1GB file?

@xUser5000
Copy link
Copy Markdown
Contributor Author

xUser5000 commented Jul 2, 2021

What has been done

OK, I've removed the use of the dictionary, replaced setTimeout() with setInterval(), and removed the redundant escaping in view.js.

Stress test

In addition to the above tasks, I stress tested the code with large .bib files. I already did a stress test with a ~300KB file, but this time I tried much larger files.

Method

I couldn't find a BibTeX file with 1GB of data so I downloaded this file. After that, I wrote a script that makes another copy of the file and then starts duplicating its contents over and over again until it reaches a certain size that I specify.

Machine specs

  • Processor: Intel Core i5-10210U (1.60GHz × 8)
  • RAM: 16GB
  • Disk: 240GB SSD
  • OS: Ubuntu 20.04 64-bit

Results

After adjusting the size parameter and observing what happens to the plugin when I execute the main command, I came up with the following conclusions:

  • If the size is above 512MB (about equivalent to 536870912 Bytes, which is about equivalent to 0x1FFFFFE8 Bytes in Hexadecimal), nothing happens for one or two seconds then the below alert appears:
    Cannot create a string longer than 0x1FFFFFE8 characters
  • If the size is below 512MB, nothing happens for about 20 seconds or so, then the call stack will not afford that much data and will throw an error like this:
    Maximum call stack size exceeded
  • If the size <= 12MB (which accounts for about 40,000 references), the plugin works fine and there are no issues at all.

@laurent22
Copy link
Copy Markdown
Member

Looks good now, let's merge. Regarding the stress test, could you put this into a new issue instead please? (otherwise the info will be lost once the PR is closed)

@laurent22 laurent22 merged commit 81ab17c into main Jul 3, 2021
@xUser5000 xUser5000 mentioned this pull request Jul 3, 2021
xUser5000 added a commit that referenced this pull request Jul 3, 2021
## New features
- #12
- #8

## Bug fixes
- #7
- #9
- #11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants