zotero / reader

PDF/EPUB/HTML reader for Zotero
Other
129 stars 30 forks source link

Add search feature in outline view #121

Closed nagi1999a closed 1 month ago

nagi1999a commented 7 months ago

Implement zotero/zotero#3021.

Thanks for the great project! I have implemented a simple recursive search method to bring the search functionality in outline view and reuse the search box component from the annotation panel.

However, since I have minimal experience in React programming, I don't know if the code changes obey the coding style. Please reply to this thread with any opinions or suggestions; thanks in advance!

dstillman commented 6 months ago

Thanks for the contribution!

This is working, but I'm seeing an error while typing:

JavaScript error: resource://zotero/reader/reader.js, line 21495: TypeError: props.onUpdateOutline is not a function
zotero(1)(+0000004): TypeError: props.onUpdateOutline is not a function

    handleOutlineSearchInput@resource://zotero/reader/reader.js:21495:11
    handleInput@resource://zotero/reader/reader.js:21395:12
    callCallback@resource://zotero/react-dom.js:3942:16
    […]

(This is when running within Zotero, to be clear. I didn't test running the reader in dev mode.)

nagi1999a commented 6 months ago

Thanks for the contribution!

This is working, but I'm seeing an error while typing:

JavaScript error: resource://zotero/reader/reader.js, line 21495: TypeError: props.onUpdateOutline is not a function
zotero(1)(+0000004): TypeError: props.onUpdateOutline is not a function

    handleOutlineSearchInput@resource://zotero/reader/reader.js:21495:11
    handleInput@resource://zotero/reader/reader.js:21395:12
    callCallback@resource://zotero/react-dom.js:3942:16
    […]

(This is when running within Zotero, to be clear. I didn't test running the reader in dev mode.)

Thanks for the reply! It turns out that I forgot to pass onUpdateOutline as a prop to the Sidebar component. I have fixed it and changed a function name to match the camel case convention. Please rebuild again to check whether the error still exists. Thanks again for verifying and reporting the error!

dstillman commented 6 months ago

Oh, so this is only showing top-level rows, with a recursive search of their contents. The way we always do filtering of hierarchical results is to automatically expand ancestors of matching rows and make non-matching context rows gray. You can see the search bar in the collections pane for the desired behavior here.

Screenshot 2024-05-08 at 2 52 59 AM

(My Library is also technically gray here — there's just different styling of selected rows.)

nagi1999a commented 6 months ago

I see! I would like to have some discussion here. Since some documents (e.g., specs, standards) may have hundreds of rows in the outline, and expanding them all may flood the whole outline view. I think a threshold of about 50 is needed to determine whether to expand all the results.

In addition, since zotero/zotero#3751 has also not yet been implemented, I prefer to implement the feature and call the expand function after search results are returned, with some additional style class to gray out ascendents that do not match. Does it seem reasonable?

nagi1999a commented 6 months ago

I have implemented the double-click feature, but I use a different way to achieve ancestors expansion in the search feature to preserve the expanded status of the pre-search state. For the concern of flood in a large number of outline items, it seems that it would not cause lags (at least on my computer), so I decided to expand all at the moment. Please check it out again; thanks!

image

nagi1999a commented 4 months ago

@mrtcode @dstillman Hello, I have modified the code to adapt recent changes to the outline view. Please help review the changes; thanks!

mrtcode commented 1 month ago

It looks pretty good, but there are a few more things to fix:

  1. No proper spacing between the outline button and the search input: Screenshot 2024-10-21 at 22 08 01
  2. There are several formatting issues, such as random spaces and inconsistent use of tabs and spaces.
  3. Please squash all your commits and rebase your branch onto master instead of using merge commits.
nagi1999a commented 1 month ago

Thank you for reviewing! I will look into these issues.

nagi1999a commented 1 month ago

@mrtcode I have squashed all the commits into one and rebased them onto the master. Also, fixes some format issues and adds a little margin between the search box and the buttons; please check again; thank you!

mrtcode commented 1 month ago

Thanks!