-
Couldn't load subscription status.
- Fork 153
Apple Notes importer Improvement for One-way Sync #394
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
base: master
Are you sure you want to change the base?
Conversation
Introduced a new setting to allow users to skip importing notes that already exist in the vault. This feature helps prevent duplication during the import process.
|
Thanks for making these improvements. I see that the PR is marked as a work in progress still, so let me know when it's ready for review. |
Introduced a new setting that allows users to specify a date prefix format for filenames during the import process. The format can include placeholders for year (YYYY), month (MM), and day (DD). If left blank, no prefix will be added. This enhancement improves file organization and user customization.
…ures" This reverts commit 696ebd6.
|
You can test the pre-release version using BRAT. https://github.com/jykim/obsidian-importer/releases/tag/v0.2 |
… when saving attachments and markdown files.
Introduced an enum for duplicate handling strategies: Skip, ImportUpdated, and CreateCopy. Updated the importer to allow users to choose how to manage existing notes and attachments during the import process, improving flexibility and control over file management.
|
@tgrosinger -- I think it's ready for your review! |
|
Just a gentle nudge @tgrosinger :) |
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.
This looks like a great start. Thank you working on this. Let me know if you have questions about any of these comments.
src/formats/apple-notes.ts
Outdated
| ' Leave blank for no prefix.' | ||
| ) | ||
| .addText(t => t | ||
| .setValue('YYYY-MM-DD') |
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.
Let's please leave this empty so the default is to preserve the existing behavior.
src/formats/apple-notes.ts
Outdated
| if (this.filePrefixFormat) { | ||
| const creationDate = new Date(this.decodeTime(row.ZCREATIONDATE3 || row.ZCREATIONDATE2 || row.ZCREATIONDATE1)); | ||
| const datePrefix = this.filePrefixFormat | ||
| .replace('YYYY', creationDate.getUTCFullYear().toString()) |
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.
This is pretty rigid right now. Let's please use Moment to output the date using the provided format string. See the other importers to see how this is being done.
src/formats/apple-notes.ts
Outdated
| title = `${datePrefix} ${title}`; | ||
| } | ||
|
|
||
| const fullPath = `${folder.path}/${title}.md`; |
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.
path.join. See other uses in this file.
src/formats/apple-notes.ts
Outdated
| // Check for duplicate notes based on the selected handling option | ||
| const existingFile = this.vault.getAbstractFileByPath(fullPath) as TFile; | ||
|
|
||
| if (existingFile) { |
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 asserting above, please do if (existingFile && existingFile instanceof TFile) {
src/formats/apple-notes.ts
Outdated
| if (this.filePrefixFormat && row.ZCREATIONDATE) { | ||
| const creationDate = new Date(this.decodeTime(row.ZCREATIONDATE)); | ||
| const datePrefix = this.filePrefixFormat | ||
| .replace('YYYY', creationDate.getUTCFullYear().toString()) |
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.
Same comment as above.
|
|
||
| if (appleAttachmentModTime <= existingAttachmentModTime) { | ||
| this.ctx.reportSkipped(finalAttachmentName, 'attachment unchanged since last import'); | ||
| return existingAttachment; |
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.
Should this be returning null?
src/formats/apple-notes.ts
Outdated
| async getAvailablePathForAttachment(filename: string, existingPaths: string[]): Promise<string> { | ||
| // Always use the default behavior from FormatImporter to respect Obsidian's attachment folder settings | ||
| // This ensures attachments go to the configured attachment folder (e.g., _files_) rather than the note folder | ||
| return super.getAvailablePathForAttachment(filename, existingPaths); |
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.
If there are no behavior changes please don't override this method.
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.
removed
src/formats/apple-notes.ts
Outdated
| if (this.duplicateHandling === DuplicateHandling.Skip || this.duplicateHandling === DuplicateHandling.ImportUpdated) { | ||
| // For Skip and ImportUpdated, create the file directly without numeric suffix | ||
| const sanitizedName = sanitizeFileName(title); | ||
| const fullPath = `${folder.path}/${sanitizedName}`; |
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.
path.join
src/formats/apple-notes.ts
Outdated
| return existingFile; | ||
| } else { | ||
| // Create new file | ||
| return await this.vault.create(fullPath, content); |
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.
If the file doesn't exist then just fall through to call the super function.
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.
done
|
Thanks for comments @tgrosinger . |
…improve file path handling - Replaced manual date formatting with moment.js for better flexibility in file prefix formatting. - Updated file path construction to use path.join for consistency. - Removed unused method for attachment path handling.
|
All your comments addressed. @tgrosinger -- thanks for thoughtful comments. As for default file prefix, I still prefer to keep 'YYYY-MM-DD' as default value, although I understand this might cause disruption for existing user. Since the use case I have in mind is running the import regularly to keep the contents in sync, setting the value every time is a bit of hassle. Can we move this into plug-in setting so that it won't have to be set for every import? |
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.
A few more comments, but heading in a good direction. Thank you.
src/formats/apple-notes.ts
Outdated
| .addDropdown(d => d | ||
| .addOption(DuplicateHandling.Skip, 'Skip import') | ||
| .addOption(DuplicateHandling.ImportUpdated, 'Import only updated') | ||
| .addOption(DuplicateHandling.CreateCopy, 'Create a copy (legacy)') |
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.
| .addOption(DuplicateHandling.CreateCopy, 'Create a copy (legacy)') | |
| .addOption(DuplicateHandling.CreateCopy, 'Create a copy') |
I don't think we need to include (legacy). Most people using this plugin are first time users of it, so they don't need to know what the previous default behavior was.
src/formats/apple-notes.ts
Outdated
|
|
||
| // Check for existing attachment based on the selected handling option | ||
| const attachmentPath = await this.getAvailablePathForAttachment(`${finalAttachmentName}.${outExt}`, []); | ||
| const existingAttachment = this.vault.getAbstractFileByPath(attachmentPath) as TFile; |
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.
Looks like this comment was missed. Please don't assert here and instead check the type.
If you remove the assertion here, then below where it's checked you can add the type check too.
if (existingAttachment && existingAttachment instanceof TFile) {
| if (existingAttachment) { | ||
| if (this.duplicateHandling === DuplicateHandling.Skip) { | ||
| this.ctx.reportSkipped(finalAttachmentName, 'attachment already exists'); | ||
| return existingAttachment; |
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.
This comment was missed.
src/formats/apple-notes.ts
Outdated
| const fullPath = path.join(folder.path, `${title}.md`); | ||
|
|
||
| // Check for duplicate notes based on the selected handling option | ||
| const existingFile = this.vault.getAbstractFileByPath(fullPath) as TFile; |
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.
| const existingFile = this.vault.getAbstractFileByPath(fullPath) as TFile; | |
| const existingFile = this.vault.getAbstractFileByPath(fullPath); |
src/formats/apple-notes.ts
Outdated
| const fullPath = path.join(folder.path, sanitizedName); | ||
|
|
||
| // Check if file already exists and handle overwriting | ||
| const existingFile = this.vault.getAbstractFileByPath(fullPath) as TFile; |
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.
Another assertion. Please change this one too.
| if (existingAttachment) { | ||
| if (this.duplicateHandling === DuplicateHandling.Skip) { | ||
| this.ctx.reportSkipped(finalAttachmentName, 'attachment already exists'); | ||
| return existingAttachment; |
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.
Actually it seems like maybe the return null below should not be returning null? Because these cases are successful, they just don't need to do anything. So we don't want the caller of this function to think this was unsuccessful.
I don't think we should add it as a plugin setting because there aren't any other plugin settings so far. What about storing the last used value in local storage, and using that if there is one? Take a look at the OneNote importer where we are using |
- Introduced local storage functionality to save and retrieve the file prefix format for Apple Notes imports - Updated duplicate handling logic to return existing files instead of null when skipping duplicates - Minor adjustments to ensure consistent type handling for existing files and attachments.
|
Thanks. I applied your other comments, and I also tried to use incorporate local storage, and the stored value (YYYY-MM-DD in my case) does show up in UI once I entered, but when I run import the default value ("") is used, creating files with no prefix. I'm trying to debug this myself but any comment would be appreciated. |
|
I added getter/setter for filePrefixFormat with log to see the behavior, and here's my observation:
|
Current importer logic for Apple Note seems to be designed for one-time import. I want to use both Apple Notes (upstream) and Obsidian (downstream), so made two improvements to Apple Notes importer for people who want to perform regular one-way sync (from Apple Notes to Obsidian).
Since this changes the default behavior at the moment, comments are welcome.
Option to add date-based prefix (default: YYYY-MM-DD)
The prefix applies to both notes and attachments. I prefer default value of YYYY-MM-DD which will make imported notes follow obsidian journal file naming convention, and preserve the chronological ordering from Apple Notes.
Option to control how duplicated notes (same name in both Apple Note & Obsidian) are handled
Again, I believe the new default (2) is more sensible which fetches only notes updated in Apple Notes, but it will overwrite the changes to corresponding notes in Obsidian, in which case (1) can be useful. IMO the legacy method (3) doesn't make much sense for regular sync.
Comments welcome.
You can test the pre-release version using BRAT.
https://github.com/jykim/obsidian-importer/releases/tag/v0.2