obsidian-releases icon indicating copy to clipboard operation
obsidian-releases copied to clipboard

Added Simple Note Review plugin.

Open dartungar opened this issue 2 years ago • 5 comments

I am submitting a new Community Plugin

Repo URL

Link to my plugin: https://github.com/dartungar/obsidian-simple-note-review

Release Checklist

  • [x] I have tested the plugin on
    • [x] Windows
    • [ ] macOS
    • [ ] Linux
    • [ ] Android (if applicable)
    • [ ] iOS (if applicable)
  • [x] My GitHub release contains all required files
    • [x] main.js
    • [x] manifest.json
    • [x] styles.css (optional)
  • [x] GitHub release name matches the exact version number specified in my manifest.json (Note: Use the exact version number, don't include a prefix v)
  • [x] The id in my manifest.json matches the id in the community-plugins.json file.
  • [x] My README.md describes the plugin's purpose and provides clear usage instructions.
  • [x] I have read the tips in https://github.com/obsidianmd/obsidian-releases/blob/master/plugin-review.md and have self-reviewed my plugin to avoid these common pitfalls.
  • [x] I have added a license in the LICENSE file.
  • [x] My project respects and is compatible with the original license of any code from other plugins that I'm using. I have given proper attribution to these other projects in my README.md.

dartungar avatar Jul 17 '22 18:07 dartungar

this.app.workspace.on("file-menu", (menu, file) => { before using it in your reviewNote´ function, check that file instanceof TFIle`, as it also can be a folder.

note as TFile do check before casting, see above.

Your plugins requires Dataview to be installed, but you should still check if its available & show a notice if its not.

joethei avatar Jul 21 '22 17:07 joethei

@joethei thanks for the review! I've made some amends:

  • I check for Dataview in the beginning of onLoad() in main.ts (https://github.com/dartungar/obsidian-simple-note-review/blob/master/main.ts) and show notice (and stop loading plugin) if it is not found. I don't know how to "not-load" plugin correctly, though, so any advice is appreciated!
  • I check the type of note parameter in reviewNote method (https://github.com/dartungar/obsidian-simple-note-review/blob/master/src/queue/queueService.ts#L25)

dartungar avatar Jul 22 '22 10:07 dartungar

@joethei I've made further improvements - now Simple Note Review will load, but show notices every time some Dataview-related function is called. See https://github.com/dartungar/obsidian-simple-note-review/blob/master/src/dataview/dataviewFacade.ts, https://github.com/dartungar/obsidian-simple-note-review/blob/18605dc673270c11c8be42407a7719279f528df5/src/queue/queueService.ts#L34, and https://github.com/dartungar/obsidian-simple-note-review/blob/18605dc673270c11c8be42407a7719279f528df5/src/queue/selectQueueModal.ts#L36.

I've tried it and it's quite smooth experience.

dartungar avatar Jul 25 '22 06:07 dartungar

  • if (dv == null) throw new Error(); doesn't look like you need a try/catch here. Should be able to just return !!getAPI()
  • simple-note-review-open-modal Not a big deal, but you don't need to prefix your command IDs with the plugin name. Under the hood, the commands get prepended with <your-plugin-id:> so it will be guaranteed to be unique anyway.
  • dataviewIsInstalled You don't want to do this onload since Obsidian doesn't guarantee a plugin load order. Your plugin might load before dataview, in which case this will fire as a false positive. It's better to wrap this in onLayoutReady.
  • note as TFile I don't think you need as Note since you have the type check above.
  • const firstNoteIndex = 0; this variable looks unnecessary. Just pass in 0 or 1
  • getNextFilePath Is this supposed to be removing items from the queue? Or is that all handled by re-running the dataview query?

liamcain avatar Aug 05 '22 00:08 liamcain

@liamcain Thanks for the thorough review! I've corrected these.

  • getNextFilePath Is this supposed to be removing items from the queue? Or is that all handled by re-running the dataview query?

The idea is to not store notes in queue, only "search terms" aka queue parameters. This reduces complexity dramatically (no need to track anything, and the results are always accurate), and the speed of Dataview ensures no serious overhead (I tested it on vault with 10000 notes - opening next note from queue that has all the notes takes ~1 second).

dartungar avatar Aug 05 '22 07:08 dartungar

@liamcain hello there, I've corrected some minor bugs that became apparent during the last week in which I used the plugin myself. I've also tested it on mobile. It would be great if you've found time to get back to this review :)

dartungar avatar Aug 12 '22 11:08 dartungar

Changes look good! One of the screenshots in your README is rotated though, you might want to fix that.

liamcain avatar Aug 12 '22 18:08 liamcain