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

Add new automatic append translation feature and newline toggle #12

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add new automatic append translation feature and newline toggle #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf28983
Add new automatic append translation feature and newline toggle
arcticpinecone May 13, 2024
f8bd5ba
Update main.ts
arcticpinecone May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 68 additions & 48 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,35 @@ export default class DeepLPlugin extends Plugin {
},
});

this.addCommand({
id: "translate-and-append-selection-automatically",
name: "Translate selection: Automatically append selection",
editorCallback: async (editor: Editor) => {
const selection = editor.getSelection();
if (selection === "") {
new Notice("No text selected.");
return;
}

try {
const translation = await this.deeplService.translate(
selection,
this.settings.toLanguage,
this.settings.fromLanguage
);
const textToAppend = this.settings.appendNewLine ? '\n' + translation[0].text : translation[0].text;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good if the setting for the new line is respected in all commands that keep the original text.

For example: deepl-translate-selection-append

Copy link
Author

Choose a reason for hiding this comment

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

I only changed it for the setting(s) which appended. I didn't want to change existing functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand that. But it would make sense to me if all functions that append would also respect the setting of the new line. With your change, not all would be the same. If the default for the setting is false, it does not change anything for existing functions, unless you set it to true. From a user's perspective, it would irritate me if one command made a new line and another did not. I would therefore adapt it everywhere for the sake of consistency.

But that's just my opinion, I'm not the owner. Not meant to sound offensive, sorry if it was :)

Copy link
Author

Choose a reason for hiding this comment

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

No no! I am new to this, and I'm not immune from mistakes or criticisms. Also I appreciate you taking the time to reply!
I'm of course a bit excited since this is really my first pull request, and I shouldn't expect it to be perfect or even accepted.

Regarding the new line, you are right. I was focused on the append parts, and I didn't take too much care of the other functions either. I just translate a lot of things, and I didn't want the existing stuff to be overwritten, and I needed a fast way to compare translations as well. Thus this 'stacked' method and a focus on the appending.

Copy link
Contributor

Choose a reason for hiding this comment

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

It takes a lot of conviction to change someone else's code, so a certain amount of conviction is necessary. The same with commenting and reviewing. We are all not free from errors. As long as we can talk about it, it's good, we learn something new every day.

And I can understand that from your perspective. I tried to take a different perspective. If we're of the same opinion now, I'd say the discussion was worthwhile :)

editor.replaceSelection(selection + textToAppend);
} catch (error) {
if (error instanceof DeepLException) {
new Notice(error.message);
} else {
console.error(error, error.stack);
new Notice("An unknown error occurred. See console for details.");
}
}
}
});

this.addCommand({
id: "deepl-translate-selection-to",
name: "Translate selection: to language",
Expand Down Expand Up @@ -135,54 +164,45 @@ export default class DeepLPlugin extends Plugin {
});

this.addCommand({
id: "deepl-translate-selection-from-to",
name: "Translate selection: From a language to another",
editorCallback: async (editor: Editor) => {
if (editor.getSelection() === "") {
return;
}

new TranslateModal(
app,
"From",
Object.entries(fromLanguages).map(([code, name]) => ({
code,
name,
})),
async (from) => {
new TranslateModal(
app,
"To",
Object.entries(toLanguages).map(([code, name]) => ({
code,
name,
})),
async (to) => {
try {
const translation =
await this.deeplService.translate(
editor.getSelection(),
to.code,
from.code
);
editor.replaceSelection(
translation[0].text
);
} catch (error) {
if (error instanceof DeepLException) {
new Notice(error.message);
} else {
console.error(error, error.stack);
new Notice(
"An unknown error occured. See console for details."
);
}
}
}
).open();
}
).open();
},
id: "deepl-translate-selection-append",
name: "Translate selection: To language and append to selection",
editorCallback: async (editor: Editor) => {
if (editor.getSelection() === "") {
return;
}

const selection = editor.getSelection();

new TranslateModal(
Copy link
Contributor

Choose a reason for hiding this comment

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

I can only specify one language in the settings. What if I want to translate from another language that is not set? That was the reason I added this functionality back then. I would miss this, if there are better solutions, I am very much in favour of them.

The command deepl-translate-selection-to use the language from the settings. The original command was explicitly meant to be able to choose a language that does not come from the settings.

Copy link
Author

Choose a reason for hiding this comment

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

It is not removing it.. It's only adding a new option without the modal. Not changing the existing modal function.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my GitHub diff view, I see that the deepl-translate-selection-from-to command has been removed. More precisely, it has been replaced with deepl-translate-selection-append. On the right side of the diff, the command deepl-translate-selection-from-to is no longer present. I can no longer see the from modal implementation either. But I only looked at it in the GitHub diff, I will check out the code, maybe an error in the diff view.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, now that I'm at the computer and can review this.. you're right actually. This is a mistake, and I appreciate you caught this! It's also super embarrassing.. 🤦‍♂️

I did not mean to remove this function. This would not be an improvement, and as you say I would miss this function also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, then we're talking about the same thing now :)
When I checked out the code, I was wondering if you had pushed everything and if we were actually talking about the same code-base. Because I still haven't seen it.

No worries, these things happen. Glad if I could help and it no longer seems like random input on my part.

app,
"To",
Object.entries(toLanguages).map(([code, name]) => ({
code,
name,
})),
async (language) => {
try {
const translation = await this.deeplService.translate(
selection,
language.code,
this.settings.fromLanguage
);
// Append a newline if the setting is enabled
const textToAppend = this.settings.appendNewLine ? `\n${translation[0].text}` : translation[0].text;
editor.replaceSelection(`${selection}${textToAppend}`);
} catch (error) {
if (error instanceof DeepLException) {
new Notice(error.message);
} else {
console.error(error, error.stack);
new Notice(
"An unknown error occurred. See console for details."
);
}
}
}
).open();
},
});
}

Expand Down
2 changes: 2 additions & 0 deletions src/settings/pluginSettings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export interface DeepLPluginSettings {
showStatusBar: boolean;
useProAPI: boolean;
formality: string;
appendNewLine: boolean;
}

export const formalities: Record<string, string> = {
Expand All @@ -20,4 +21,5 @@ export const defaultSettings: Partial<DeepLPluginSettings> = {
showStatusBar: true,
useProAPI: false,
formality: "default",
appendNewLine: false,
};
16 changes: 16 additions & 0 deletions src/settings/settingTab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ export class SettingTab extends PluginSettingTab {
"The target language can be selected by suggestion modal. The translation will be appended to the selection."
);

new Setting(containerEl)
.setName("Translate selection: Automatically append selection")
Copy link
Contributor

Choose a reason for hiding this comment

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

It is already respected as long as you select the right command

For example: https://github.com/friebetill/obsidian-deepl/pull/12/files#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dR38-R39

.setDesc("The translation will be appended to the selection using the 'To language' setting automatically, without the suggestion modal.");

containerEl.createEl("h4", {
text: "Language settings",
});
Expand Down Expand Up @@ -106,6 +110,18 @@ export class SettingTab extends PluginSettingTab {
})
);

new Setting(containerEl)
.setName("When appending text, add a new line before appending the translation?")
.setDesc("If enabled, adds a new line before appending the translated text.")
.addToggle((toggle) =>
toggle
.setValue(this.plugin.settings.appendNewLine)
.onChange(async (value) => {
this.plugin.settings.appendNewLine = value;
await this.plugin.saveSettings();
})
);

containerEl.createEl("h4", {
text: "Authentication settings",
});
Expand Down