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.

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

Updating saved file names of chat sessions and summary files #15

Merged
merged 4 commits into from
Mar 16, 2023

Conversation

Shaka-n
Copy link
Contributor

@Shaka-n Shaka-n commented Mar 15, 2023

This updates the file names of the Summarize and the Ask commands!

@@ -42,8 +42,8 @@ export class AskChatGPTModal extends Modal {
}

async saveToAndOpenNewNote(text: string) {
const noteRandomId = Math.floor(Math.random() * (100000 - 1) + 1);
const newNote = await this.app.vault.create(`/vaultchat-${noteRandomId}.md`, text)
const dateTime = (new Date().toDateString()).split(" ").join("-")
Copy link
Member

Choose a reason for hiding this comment

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

What does this end up looking like? Is it just the date or does it include the time? If it's just the date then you would only be able to save one chat per day without collisions.

Copy link
Member

@kristenbrann kristenbrann Mar 15, 2023

Choose a reason for hiding this comment

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

Ran in dev console and got this:

> new Date().toDateString().split(" ").join("-")
< 'Wed-Mar-15-2023'

I think we want something more like Vault Chat - 2023-03-14 09:37:21.md

Hmm, can't have colons in file names in obsidian though... Let's do:
VaultChat-03-14-2023-093721.md where it is VaultChat-<Mon>-<Day>-<Year>-<RandomNumber>.md

Comment on lines 46 to 47
const summaryTitle = this.fileName.substring(0, this.fileName.length-3)
const newNote = await this.app.vault.create(`/${summaryTitle} - Vault Chat Summary.md`, text)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Shaka-n
Copy link
Contributor Author

Shaka-n commented Mar 16, 2023

Added a change to the callback function for Summarize, per this (feedback)[https://github.com/obsidianmd/obsidian-releases/pull/1752#issuecomment-1469805304].

@@ -1,4 +1,4 @@
import { Plugin, TFile} from 'obsidian';
import { Editor, MarkdownView, Plugin, TFile} from 'obsidian';
Copy link
Member

Choose a reason for hiding this comment

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

unused imports?

Comment on lines +79 to +94
if(checking){
if(activeFile){
return true
}else{
return false
}
}else{
if(activeFile){
const fileName = activeFile.name
const fileContents = await this.app.vault.read(activeFile)
new SummarizeNoteModal(this.app, this, this.openAIHandler, fileName, fileContents).open();
this.app.vault.read(activeFile).then(f => { new SummarizeNoteModal(this.app, this, this.openAIHandler, fileName, f).open()})
return true
}else{
return false
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This could could be simplified for readability:

In general, this sort of code:

if(checking) {
    if(activeFile) {
        return  true
    } else {
        return false
    }
}

Can be simplified to just return the boolean (or return activeFile !== undefined, or whatever makes most sense to you):

if(checking) {
    return activeFile!! 
}

However, in this case where theres even more conditionals and return statements, the following might make more sense:

checkCallback: (checking: boolean) => {
	const activeFile = this.app.workspace.activeEditor?.file
	if(activeFile){
		if(!checking){
			const fileName = activeFile.name
			this.app.vault.read(activeFile).then(f => { new SummarizeNoteModal(this.app, this, this.openAIHandler, fileName, f).open()})
		}
		return true
	}
    return false
}

Copy link
Member

@kristenbrann kristenbrann left a comment

Choose a reason for hiding this comment

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

Some tiny comments, but overall functional. Merging so we can cut the next release!

@kristenbrann kristenbrann merged commit 696808a into main Mar 16, 2023
@kristenbrann kristenbrann deleted the ss-update-chat-filenames branch March 16, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants