-
Notifications
You must be signed in to change notification settings - Fork 32
thread summary #597
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: v3.8.0
Are you sure you want to change the base?
thread summary #597
Conversation
| export const contextMenuIconsPath = { | ||
| [contextMenuID_AddTags]: 'moz-extension:images/autotags.png', | ||
| [contextMenuID_Spamfilter]: 'moz-extension:images/spamfilter.png', | ||
| // [contextMenuID_Summarize]: 'moz-extension:images/summarize.png', |
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.
no icon yet. If you have a good idea, go ahead and add one.
|
Thank you Guido for your PR! I'm currently working on version 3.8.0 in this branch that simplifies a lot how special prompts work in regard of the different integration APIs and the default settings. May you rebase this PR to that branch? |
I think the automatic summary inside of the email (#579/#580) sounds very cool. Both options can be useful, and the user should be able to use both. I am personally more interested in the ability to summarize threads and ask questions about them, so the ability to select more than one email is important to me, but this does not mean that won't appreciate automatic summaries when opening emails. Regarding #152 : You can already get this by "context menu" -> "open" -> "open message in conversation". There you can just collapse the thread and then use "context menu" -> "summarize" from my PR. It is a couple of more clicks but also gives a bit more control in case you want to exclude an email.
👍 |
You are right, this gives a workaround waiting for that feature in the Thunderbird API. |
b057c31 to
81c626a
Compare
|
You should also change the PR base to be the branch v3.8.0. |
|
Question: Currently I have separators between emails in both the prompt as well as in concatenation. Where should they go? Translations are auto generated, right? I think you can review the PR now. |
|
You need to prepare also the final prompt, to handle placeholders. For the concatenation, I'd like to have an option in the special prompt page to define it. The default value should be a translated string and not something hardcoded, so that the user can define it. For the translation, do not worry, there is no need to add it to the PR, I manage the translation on weblate. |
I was thinking about using a parameter, but a dedicated prompt might be more flexible. May you handle the enable and disable of the button in the option page, like the tag and spamfilter ones?
With the "Placeholders: use default value" option selected it should use an empty string instead of the placeholder tag. do you have that option active?
This is my fault. I fixed it in aa0fd97, thank you! |
|
Could you also add descriptions for the three prompts on the management page to explain their purpose? |
f0773d8 to
51c8141
Compare
Rebased onto v3.8.0, error is gone. 👍
Let me know if you want me to change it, see also comment at the end. Folding this one into the other two prompts is possible and decreases the amount of code.
Ahh, didn't notice this one before, thanks! I only require
Option was unchecked. Works as intended, thanks!
Done, please let me know if they are not clear enough. Also:
|
|
|
||
| // Add Context menu: Summarize | ||
| if(prefs_init.summarize && prefs_init.summarize_context_menu && checkAPIIntegration(prefs_init.connection_type && prefs_init.summarize_use_specific_integration, prefs_init.summarize_connection_type)) { | ||
| console.log("adding summarize to context menu") |
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.
To be removed before merging.
| summarize_el.addEventListener('click', (event) => { | ||
| async function _summarize_el_change() { | ||
| if (event.target.checked) { | ||
| let granted = await messenger.permissions.request({ permissions: ["messagesRead"] }); |
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.
messageRead is not an optional permission, so you can't ask for it here. The permission is mandatory to install ThunderAI. See the mainfest.json file.
In the logs there is this error: "Uncaught (in promise) Error: Cannot request permission messagesRead since it was not declared in optional_permissions"
| spamfilter_context_menu: true, | ||
| spamfilter_enabled_accounts: [], | ||
| summarize: false, | ||
| summarize_context_menu: true, |
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.
You have added this option that is used in the mzta-background script, but there is no way for the user to change it. I use a similar option for add_tags and spamfilter, but I'm thiking about removing it and always show the menuitem when the user has activated the feature. I think you could remove it and always show the context menu when the summurize feature is activated.
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.
I filed issue #609
|
You need to add these to the en locale file:
You are using |
| let getting = await browser.storage.sync.get(prefs_default); | ||
|
|
||
| let specialPrompts = await getSpecialPrompts(); | ||
| let addtags_prompt = specialPrompts.find(prompt => prompt.id === 'prompt_add_tags'); |
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.
you need to update for summarize this code pasted from add_tags.
I think we can keep it this way.
In your actual code at that line there is no call that uses
The problem is in the You need to change line 53 of fullPrompt = curr_prompt.text + (String(curr_prompt.need_signature) == "1" ? " " + await taPromptUtils.getDefaultSignature():"") + " " + chatgpt_lang + ((selection_text!='' || body_text != '')? " \"" + (selection_text=='' ? body_text : selection_text) + "\" " : '');This has to be tested against all the other prompts if they continue to work.
I think this happens when the mail body is converted from html to text.
Find a generic prompt that for you is working ok and we can use it as standard.
I checked it in the past, but there is no standard about it, so it's not possibile to be sure to strip the correct part of the email. Thank you for all the work you are putting in adding this feature!! |





Adds an option to the context menu to summarize single emails and email threads and allows the user to chat with them.
Context menu:
Summary:
Asking questions: