-
Notifications
You must be signed in to change notification settings - Fork 678
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.
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
Add ability to open a random note #440
Add ability to open a random note #440
Conversation
const notes = foam.notes | ||
.getNotes() | ||
.map(note => note.uri.path) | ||
.filter(notePath => notePath !== currentFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of filtering the whole array of notes to remove one, which can be expensive (O(n)) as your knowledge base grows, we could optimistically pick a note, and if it's the current one pick another random note, wdyt?
You can still check that there is more than 1 note in the array below to keep that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea
Change random note if optimistic choice was the current file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTGM!
.map(note => note.uri.path) | ||
.filter(notePath => notePath !== currentFile); | ||
if (notes.length === 0) { | ||
const notes = foam.notes.getNotes().map(note => note.uri.path); |
There was a problem hiding this comment.
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 do the mapping (same issue as with the filter
before, we'd be doing an extra scan of the array) - you can access the field you need when you need it
|
||
let randomNoteIndex = Math.floor(Math.random() * notes.length); | ||
if (notes[randomNoteIndex] === currentFile) { | ||
notes.splice(randomNoteIndex, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
splice can also be expensive on big arrays, how about getting randomNoteIndex + 1 % notes.length
, which would also turn O(n) into O(1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that works. splice
only takes 2ms to randomly remove one item in an array of 1,000,000 items on my computer so I wasn't really concerned 😂 I'll push up the fix in a second
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahah sorry about being pedantic there, I wasn't expecting it to be that fast (should have tested!) 😂😂😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! You're just keeping me honest 😂
Make algorithm O(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thx!
@allcontributors add @MCluck90 for code |
I've put up a pull request to add @MCluck90! 🎉 |
Adds a
Foam: Open Random Note
command. It will open any random note in the knowledge base. It is guaranteed to not open the note that you're already viewingThis will implement the basic case outlined in #422 but more work will be needed for any sort of prioritization.