Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

enhance: add :graph/ready event; check search version of each repo #4232

Merged
merged 3 commits into from Feb 23, 2022
Merged

enhance: add :graph/ready event; check search version of each repo #4232

merged 3 commits into from Feb 23, 2022

Conversation

cnrpman
Copy link
Collaborator

@cnrpman cnrpman commented Feb 16, 2022 •
edited

  • Add :graph/ready event
    • Happens when a graph is ready to show
  • Check FTS cache version on :graph/ready, rebuild search index when cache is stale
  • Test on macOS
  • Test on Windows
  • Test on Linux

@cnrpman cnrpman force-pushed the fix-search-version branch 2 times, most recently from c499231 to 04a96c4 Compare February 16, 2022 17:22
@cnrpman cnrpman added the :type/enhancement Enhancement to product. Does not affect the overall basic use. label Feb 16, 2022
@cnrpman cnrpman mentioned this pull request Feb 17, 2022
Merged
@cnrpman cnrpman marked this pull request as ready for review February 18, 2022 15:25
@logseq-cldwalker
Copy link
Collaborator

@cnrpman I'd be happy to QA this if you'd like. Do you have some instructions on how I could test this?

@cnrpman
Copy link
Collaborator Author

cnrpman commented Feb 19, 2022 •
edited

@cnrpman I'd be happy to QA this if you'd like. Do you have some instructions on how I could test this?

Cool! It's an enhancement towards Desktop app. The major idea is to automatically apply rebuild-search-index on user's graphs if the full text searching cache (sqlite3 cache) of the graph is outdated. The check is triggered when a graph is ready to show.
It's about writing the search cache version defined in HERE into <electron log path>/search.versions

So I think the test is about:

  1. remove the search cache (/Users/<usrname>/Library/Application Support/Logseq/search/ on macOS) or edit search cache version files (/Users/<usrname>/Library/Application Support/Logseq/search.versions/ on macOS)
  2. open the app
  3. try to switch / unlink / relink graphs arbitrarily, to see if the rebuild process is triggered.

logseq-cldwalker
logseq-cldwalker approved these changes Feb 22, 2022
View reviewed changes
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@cnrpman QAed for the case of removing a search directory and looks great! 👍 🚢 I gave some suggestions on removing commented out code and print statements but your call on what you think is worth doing

src/electron/electron/search.cljs Show resolved Hide resolved
src/electron/electron/search.cljs Outdated Show resolved Hide resolved
src/main/frontend/handler/events.cljs Outdated Show resolved Hide resolved
src/main/frontend/handler/events.cljs Outdated
;; TODO: when "only restore the current graph instead of all the graphs" is done,
;; remove invoke of :graph/ready in graph/switch and restore-and-setup!
(defmethod handle :graph/ready [[_ repo]]
(js/console.log "graph ready")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like leftover debugging. If it's for logging, can we use lambdaisland.glogi instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The removal is done

andelf
andelf approved these changes Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@andelf andelf left a comment

Choose a reason for hiding this comment

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

LGTM

@andelf andelf merged commit 3517090 into logseq:master Feb 23, 2022
logseq-cldwalker added a commit that referenced this pull request Feb 24, 2022
@logseq-cldwalker
Remove unused requires from #4232
37210e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andelf andelf andelf approved these changes

@logseq-cldwalker logseq-cldwalker logseq-cldwalker approved these changes

@llcc llcc Awaiting requested review from llcc

@RCmerci RCmerci Awaiting requested review from RCmerci

Assignees
No one assigned
Labels
:type/enhancement Enhancement to product. Does not affect the overall basic use.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cnrpman @logseq-cldwalker @andelf