Add new automatic append translation feature and newline toggle #12
Conversation
Implement a new command 'Translate selection: Automatically append selection' that automatically uses the predefined 'To language' to translate and append the selected text without requiring user interaction through a modal. Enhance both append functionalities with a configurable newline toggle, allowing users to decide if a newline should be added before appending the translated text.
The new line toggle should work also with the existing `Translate selection: To language and append to selection` option.
There was a problem hiding this comment.
I can understand your adjustments. I think the changes regarding new-line is great. I wonder whether the current implementation is the right one.
There are already commands that respect the source language and target language of the setting.
I have the feeling that no command needs to be removed. From my point of view, it makes sense that I can select the source language and target language.
I think it only needs the setting for the new line. This setting should then be taken into account in all "append commands".
These are my considerations, which I have made because code I have written has been changed. I am open to discussing this.
In the end it is up to @friebetill to decide.
|
|
||
| const selection = editor.getSelection(); | ||
|
|
||
| new TranslateModal( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is not removing it.. It's only adding a new option without the modal. Not changing the existing modal function.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| this.settings.toLanguage, | ||
| this.settings.fromLanguage | ||
| ); | ||
| const textToAppend = this.settings.appendNewLine ? '\n' + translation[0].text : translation[0].text; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I only changed it for the setting(s) which appended. I didn't want to change existing functions.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
| @@ -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") | |||
There was a problem hiding this comment.
It is already respected as long as you select the right command
Implement a new command 'Translate selection: Automatically append selection' that automatically uses the predefined 'To language' to translate and append the selected text without requiring user interaction through a modal. Enhance both append functionalities with a configurable newline toggle, allowing users to decide if a newline should be added before appending the translated text.
(Sorry for the messy first commit, it was my first time ever doing a pull request! 🙇♂️)