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

Improve unread messages notification format #387

Closed
TheLastZombie opened this issue Mar 30, 2021 · 35 comments
Closed

Improve unread messages notification format #387

TheLastZombie opened this issue Mar 30, 2021 · 35 comments

Comments

@TheLastZombie
Copy link

It would be great to have the unread messages notification that pops up whenever there's a new mail to:

  • actually show the subject line and either the sender or a preview of that mail (instead of "x unread messages")
  • automatically open/expand the mail when clicked (instead of just displaying the inbox)

That way one won't have to neccessarily click the notification to know what the mail is about.

@vladimiry
Copy link
Owner

If this gets implemented then I think only if the local store toggle enabled for the account. In that mode, we have all the needed data kept in memory which simplifies the implementation.

@tukusejssirs
Copy link

If this gets implemented then I think only if the local store toggle enabled for the account. In that mode, we have all the needed data kept in memory which simplifies the implementation.

When it’ll be implemented, will it be then possible to view the web UI, enable local store and show the notifications with sender and subject?

@vladimiry
Copy link
Owner

vladimiry commented Mar 26, 2022

Addition feature request point picked from #422:

  • If this app will have an option to specify for example a shell script, and execute it with the message from first option as argument, there will appear a possibility to send this arguments where you want, to other email or chat.

@vladimiry
Copy link
Owner

When it’ll be implemented, will it be then possible to view the web UI, enable local store and show the notifications with sender and subject?

A user will be able to build a fully custom notification message using any email data, not only "subject" and "sender".

@vladimiry
Copy link
Owner

The first part of the feature, showing a custom desktop notification message, is completed in wip-custom-notification-message branch.

actually show the subject line and either the sender or a preview of that mail (instead of "x unread messages")

A user will be able to build a fully custom notification message using any email data, not only "subject" and "sender".

See the respective demo:

custom-notification-demo

So, as I said before, the feature enables a user to display a fully custom message using any email message data. So code syntax is highlighted with TypeScript support. This means that the email data model is taken from https://github.com/ProtonMail/WebClients/ and injected into the code editor, so you get auto-complete support, code errors highlighting, etc.

By the way, a similar idea with a smart code editor was used in the "JavaScript-based / programmatic messages search" feature which got originally enabled about a year ago in the v4.11.0 release.

At the moment only a plain text message is supported in the notification but technically in the future, it's possible to use a custom native module for showing advanced desktop notification messages (with formatting support, additional custom buttons, etc).

If someone wants to test the feature before it gets released, let me know and I will provide a WIP build.

So the remaining part is the following:

If this app will have an option to specify for example a shell script, and execute it with the message from first option as argument, there will appear a possibility to send this arguments where you want, to other email or chat.

For this need, I think there will be another code editor which will be used for forming a string argument to send to some executable script/binary.

@vladimiry
Copy link
Owner

If this app will have an option to specify for example a shell script, and execute it with the message from first option as argument, there will appear a possibility to send this arguments where you want, to other email or chat.

Here it's:
notification-shell-exec-demo

@tukusejssirs
Copy link

@vladimiry, I tried out Custom notification content. It is nice, but not perfect.

The default code outputs sender, subject, time of all email messages in inbox from the latest to the oldest. Now, I have 1324 messages in my inbox.

I want to create one notification for every new email I receive. I don’t one notification for all emails.

I can filter the emails to have only Unread emails, but there is no way to filter out which email was read and then made unread OR which email is new of which I haven’t been notified yet.

TL;DR: Could you add a property which would be true (1) only if it haven’t triggered a notification yet? I can request this feature in a separate issue, but I wanted to know if it makes sense to you.

Thanks! 🙏

@vladimiry
Copy link
Owner

vladimiry commented Feb 1, 2023

@tukusejssirs this will be won't fix as too specific need.

Technically, you can already achieve the need. You would use Shell command execution #387 custom feature/code to process the notification data by a custom script. In this script, you would:

  • Save in some local store/database IDs of emails notification for which got displayed. You might prefer to store IDs not on showing the notification, but on clicking on its button (manual confirmation).
  • You would only show notifications for the emails IDs of which you haven't saved in your store/DB.
  • You would use some command line tool for showing "desktop notifications". There have to be some, I believe.

Regarding tracking, "read => unread" status switch. You could use a similar approach of shell custom script to save the "read" flag switching. I'm not putting this logic into the app.

@tukusejssirs
Copy link

@tukusejssirs this will be won't fix as too specific need.

Thanks for reply! 🙏 However, I am not quite sure why you think this is a too specific need.

What I want is a a notification that is usual one in any other email client on any platform: get a notification every time a new email arrives with the sender and subject.

Now, when not using the custom desktop notification code, I get a notification every time:

  • a new email arrives in my inbox (this is what I want);
  • a read email is made unread (this is what I don’t want, as it is an quite disturbing; I know I made the email unread, thus why I need to be notified of it?).

Moreover, you don’t provide the sender and subject by default due to privacy issues which might exist if you are in an untrusted place, however, IMHO most of the users most of the time are checking their emails in a trusted place (like at home). I don’t to argue about this with you, it is you decision and there exists a way to output the subject and sender.

Technically, you can already achieve the need. You would use Shell command execution #387 custom feature/code to process the notification data by a custom script.

I believe that if ElectronMail saves this as a separate property:

  1. many users would benefit from this (not all users can write JS);
  2. IMHO it is much easier and much less error-prone to note this does in ElectronMail that outside of it:
    • you already have a copy of all emails in the database, thus adding a column and a single command after creating the notification to update the database table row.

For me (as well as others), it might be better and easier to add a option what should be the notification content:

  1. [default] privacy-oriented content (no content from the emails, just the account/alias and number of unread emails);
  2. create a new notification with sender (name and address) and subject, one for each new received email.

IMHO if such option (a toggle) would be implemented, this issue would be fully solved and the much more users would be happy using ElectronMail.

Just a note: even ProtonMail client on Android displays the sender and subject.

@vladimiry
Copy link
Owner

adding a column and a single command after creating the notification to update the database table row.

Currently, in relation to the local store, there are no such terms as column or table row. Saving local store is currently relatively heavy operation (and decrypting it too), so triggering saving operation more frequently than required by each email status update is not a very good idea. But when/if #571 gets implemented, I might reconsider adding a new flag for email.

@tukusejssirs
Copy link

tukusejssirs commented Feb 1, 2023

Currently, in relation to the local store, there are no such terms as column or table row.

Oh, okay. I was under impression (based on your usage of the term database in the l10n string in the tooltip of the plug icon (Toggle online/database view mode).

Therefore, yes, it totally makes sense to move to a SQLite before adding new properties/columns. I understand that it might not happen in the near future, but I think it would make the local store much more performant, thus it might be nice to implement it ASAP (I don’t want you hurry nor tell you what should be your priorities). I am not a Go coder, thus I might not be able to help you out in this.

Anyway, I’ll try to use the custom shell command execution to build something when I have some more time for this.

Thanks for you time and ElectronMail! 🙏


Jest a question relating to shell command execution: is the output of formatNotificationShellExecArguments() function displayed as a notification? Therefore I should use only either custom notification content or shell command execution, right?

@vladimiry
Copy link
Owner

vladimiry commented Feb 1, 2023

is the output of formatNotificationShellExecArguments() function displayed as a notification? Therefore I should use only either custom notification content or shell command execution, right?

Nope. Shell script calling is an independent thing.

Also notice some points from the d9f462d commit comment (or release note of https://github.com/vladimiry/ElectronMail/releases/tag/v4.14.0):

  • If the resolved "body" is empty then a notification won't be displayed. So this is a way to suppress the notification for a specific account or mail data.
  • If the resolved "execution command" is empty then execution won't be happening. So this is a way to suppress the command execution for a specific account or mail data.

@tukusejssirs
Copy link

Nope. Shell script calling is an independent thing.

Okay, then, I need to (somehow) read/get the data of emails (or at least their IDs) to trigger a notification in the custom notification content function, right? It feels to be cumbersome.

@vladimiry
Copy link
Owner

You can see on the screenshot in #387 (comment) that mails array is coming into your formatNotificationShellExecArguments function as an argument. But, I missed pointing out, that currently only unread emails come to that function.

@tukusejssirs
Copy link

You can see on the screenshot in #387 (comment) that mails array is coming into your formatNotificationShellExecArguments function as an argument.

I’ve seen that screenshot and I have printed out an item from mails array, thus I know the structure, but I miss a property that would tell me if the email is new (thus no notification has been triggered yet).

But, I missed pointing out, that currently only unread emails come to that function.

I have no problem with that as new emails are unread emails, but not all unread emails are new emails.

@vladimiry
Copy link
Owner

but I miss a property that would tell me if the email is new (thus no notification has been triggered yet).

I've written above that maintaining this property is currently on you (using some external storage/database).

@tukusejssirs
Copy link

I get that, I didn’t want to suggest anything more.

Another question: is it possible to trigger multiple notifications from formatNotificationContent()? I want a notification per email, not one notification for all emails. There is a chance that multiple emails would be retrieved at the same time.

@vladimiry
Copy link
Owner

vladimiry commented Feb 1, 2023

I want a notification per email

Currently, the app displays one notification per mail account. But using custom shell script function should alloy you to display as many notifications as needed (if you manage to find a suitable notification showing tool that would support command line interface, a tool could be a self-written too I guess).

@tukusejssirs
Copy link

tukusejssirs commented Feb 1, 2023

@vladimiry, could you tell me why the following code fails running? It runs as expected when I remove the mails property or when I use mails[0].

formatNotificationShellExecArguments(({login, alias, mails}) => {
  const data = JSON.stringify({
    alias,
    date: new Date(),
    login,
    oneMail: mails[0],

    // None of these three values of `mails` work. When I omit then, I get the output, otherwise it raises an error, which is hard to debug, as you use some 
    mails
    // mails: mails.map(i => ({id: i.ID, sender: i.Sender, subject: i.Subject}))
    // mails: mails.reduce((a, c) => [...a, {id: c.ID, sender: c.Sender, subject: c.Subject}], [])
    
    // mails: mails.reduce((accumulator, {Subject, Sender, Time}) => [...accumulator, {Sender, Subject, Time}], [])
  }).replace(/'/g, "\\'")

  return {command: `/usr/bin/echo '${data}' > /home/ts/git/test/desktop_notifs/tmp.json`}
})
// Error when using `mails` or `mails.reduce()` or `mails.map()`:
Uncaught (in promise): Error: Failed to execute a triggered by an unread desktop notification shell exec command!
Error: Failed to execute a triggered by an unread desktop notification shell exec command!
at executeUnreadNotificationShellCommand (/opt/ElectronMail/resources/app.asar/app/electron-main/index.cjs:92077:17)

How could I debug this?

@vladimiry
Copy link
Owner

vladimiry commented Feb 1, 2023

Maybe see if limiting array size helps, using mails.slice(0,10) instead of mails. I guess there might be a weak point somewhere that is sensitive to the data portion size. There is no answer at the moment where that weak point is if you confirm data size correlation.

@tukusejssirs
Copy link

Thanks, @vladimiry, you are right that that is some max data size (maybe a contraint on the size of the used memory, IDK).

According to my tests, I could used data of max 131,011 chars size; anything greater fails. This is my test function that succeeds:

formatNotificationShellExecArguments(({login, alias, mails}) => {
  return {command: `/usr/bin/echo '${''.padStart(131011, '1')}' > /some/path/with/42/chars/in/length/including/filename/tmp.json`}
})
Test cases
// data length: okay
100000
120000
130000
131000
131010
131011  // Greatest number of chars that does not fail

// data length: fails with the error below
131012
131013
131015
131020
131030
131050
131100
131200
131300
131500
132000
133000
135000
140000
160000
180000
200000
// Error
Uncaught (in promise): Error: Failed to execute a triggered by an unread desktop notification shell exec command!
Error: Failed to execute a triggered by an unread desktop notification shell exec command!
at executeUnreadNotificationShellCommand (/opt/ElectronMail/resources/app.asar/app/electron-main/index.cjs:92077:17)

An item from mails array has approx 15,000 chars in my case, thus 8 items could be used in my case. That said, I don’t need the anything from mails but ID, sender and subject, thus the size of an item could be reduced to approx 300 chars (min: 187, max: 376), thus I can use about 436 items in my case. No chance to read all my emails (1379) at once, which I would do only to create the initial backup.

@vladimiry
Copy link
Owner

vladimiry commented Feb 2, 2023

@tukusejssirs, thanks for the detailed report. Looks like you hit the issue of maximum character length of arguments in a shell command. I executed the test and got E2BIG error (I've just enabled error code printing, see 1da058c). I think some shell script execution redesigning should be happening in the app, but there is no clear path yet.

No chance to read all my emails (1379) at once, which I would do only to create the initial backup.

Technically, there might be a chance if you put some pure JS string compression implementation into the formatNotificationShellExecArguments and apply it to the serialized mails data. You would have to decompress it then in an executed script. But I understand that this sounds too complex thing for a one-off need.

@tukusejssirs
Copy link

tukusejssirs commented Feb 2, 2023

Thanks for the investigation.

Now, I have even thought about writing to file, but node:fs is not available (as it is an Electron app). Another option would be to create a simple Fastify/Express app with REST API to POST the data to, process the data there and trigger a notification using node-notifier there, but I’ve just thought it might be too much. I also wanted the this to be done without any daemon or server running in the background.

For now, I see only one option: iterate over mails, extract the required properties only, create a temporary array into which we push email data one by one until JSON.stringify(m).length is less that max allowed char size.

formatNotificationShellExecArguments(({login, alias, mails}) => {
  /**
   * Max data size in characters in JSON string
   *
   * @remarks For some reason, `131011` chars does not work this way. Sometimes I get `83781` chars, but not always. Does this character limit counts the size (in chars) of this function?
   */
  const maxDataSize = 83499

  /**
   * Data used by the shell command
   *
   * @remarks
   *
   * `data` type is as follows:
   *
   * type data = {
   *   a: string,     // Account alias
   *   d: number,     // Timestamp when the emails where retrieved
   *   l: string,     // Account login name
   *   m: ({
   *     i: string,   // Email ID
   *     sn: string,  // Email sender name
   *     se: string,  // Email sender email
   *     s: string    // Email subject
   *   })[]
   * }
   */
  const data = {a: alias, d: new Date().getTime(), l: login, m: []}

  // Sort `mails` by received data descending
  mails.sort((a, b) => b.Time - a.Time)

  // Filter `mails` to keep only those properties that are needed by the shell command
  for (const i of mails) {
    const t = {i: i.ID, sn: i.Sender.Name, se: i.Sender.Email, s: i.Subject}

    if (JSON.stringify({...data, m: [...data.m, t]}).replace(/'/g, "\\'").length <= maxDataSize) {
      data.m.push(t)
    } else {
      break
    }
  }

  return {command: `/usr/bin/echo '${JSON.stringify(data).replace(/'/g, "\\'")}' > /home/ts/git/test/desktop_notifs/tmp.json`}
})

I have noticed that mails in unordered array of emails. At first, I’ve thought that emails I received after 28 Jan 2023 are missing from mails, but it is not the case. I need to sort the emails. Can’t you sort them by mails[].Time descending please?

A bit OT, but in the GUI code editor (both for notifications and shell command), it seems (based on the intellisense) like the function is run as module and it does not know about console and require (it knows about import), which is not a problem in itself, but what is that it seems to check the code as if it was a TypeScript code (when I define const data = {a: 1}, later I add data.b = 2 and the editor raises an error). I have no idea if it fails running the code (as it could fail due command length error). All I know that if fails when I adds types, thus it is definitely not executed nor compiled as TS script.

I've just enabled error code printing

It might be a good idea to print the errors (using try..catch), shell command exit codes in the pop-ups (or whatever you call them), as well as provide a way to log some arbitrary stuff to that pop-up (similarly how console.log() outputs stuff to the console). Consider adding a scrollbar to the pop-ups and max height. And consider also adding a copy button too, as selecting the pop-up content (apart from double and triple-clicking to select a word or line) is quite hard.

@vladimiry
Copy link
Owner

vladimiry commented Feb 2, 2023

The max cmd arg length value likely depends on the system (type, version, etc) and so I'd prefer not to constant it in the app.

The workaround I currently consider is extending formatNotificationShellExecArguments return type by an array of objects. So the app will execute a shell script for each array item, the same way it currently works for a single object.

I believe a regular case is when the notification/unread emails count is a relative small value. So returning an object, like it works now, should work well for most cases. And for specific cases like yours, it will be possible to work around the issue by returning an array and expecting multiple shell command executions. Splitting the result to an array will be on user (no hard-coded constant on the app side).

You won't be able to use node:fs or alike things, since the code being evaluated in a QuickJS runtime (connected as Wasm module). So there is no access to the file system or other resources. The code editor is enhanced by understanding the mail data format. There might be some imperfection in remaining globals typing, but it does good enough work.

I need to sort the emails. Can’t you sort them by mails[].Time descending please?

This is a specific need which can be handled by the custom code, so I'd prefer not to put it into the app code. Generally, adding code increases project maintenance burden, so the less is the better.

{i: i.ID, sn: i.Sender.Name, se: i.Sender.Email, s: i.Subject}

It can be an array of data vs object props, so it would keep you some space in the result string.

@tukusejssirs
Copy link

Looks like you hit the issue of maximum character length of arguments in a shell command. I executed the test and got E2BIG error

Well, I have just tested the following code in Node (node file.js). It has no problem to output 999,999 chars. It might be a problem with Electron which I have never used (I used Tauri, which has much better features, smaller app size and it is possibly much more performant).

const {execSync} = require('child_process')

console.log(execSync(`/usr/bin/echo '${''.padStart(999999, '1')}' > /some/path/with/42/chars/in/length/including/filename/tmp.json`).toString())

I believe a regular case is when the notification/unread emails count is a relative small value.

Yeah. I am in the middle of creating a Node script to process mails and I want to save mails each adding an n boolean property (true means that the notification was triggered, thus no new notification will be triggered). From the backup, I purge all emails I have received yesterday and older. I could purge these in formatNotificationShellExecArguments() as well, thus reduce the size of mails, but in there I can’t be sure if the email has already triggered a notification or not.

Splitting the result to an array will be on user (no hard-coded constant on the app side).

Yes, splitting the array could make it work, but the performance might get worse.

the code being evaluated in a QuickJS runtime

That is good to know.

@vladimiry
Copy link
Owner

The test code you posted gives me the same error (with code=E2BIG). If I lower the size, it finishes without error.

@tukusejssirs
Copy link

The test code you posted gives me the same error (with code=E2BIG). If I lower the size, it finishes without error.

How did you run the code from this comment? From a terminal? What is your OS? NPM and Node version?

I tested it from a terminal using node@19.5.0, npm@9.1.2 on Arch Linux and there is no issue with that code in itself.

@vladimiry
Copy link
Owner

vladimiry commented Feb 2, 2023

It doesn't really matter which OS/node I used, since the successful exec/execSync execution confirmed as environment/OS/node and cmd line length dependent.

@tukusejssirs
Copy link

Yeah, it errors out for me too, I missed the error. It does not errors out when echo is run directly in the terminal (without Node).

Now, a soulution could be writing the command to BASH/SH file file and execute it (bash -c file.sh) instead. Could it be easily done in Electron without node:fs?

@vladimiry
Copy link
Owner

vladimiry commented Feb 2, 2023

Clearly, there are two options for solving the issue like this. Putting the data to the file and then reading the data from it vs reading from cmd arg, or executing the shell script multiple times or the data split to the pieces. I prefer the latest option as a more isolated and more privacy-friendly option (so you put something to a file, and then the script fails for some reason, but the unencrypted data is still there in the file, or you missed removing that file in the script's logic doing testing/debugging, and the situations alike). I mean, I prefer extending formatNotificationShellExecArguments result type by array (side benefit is that backward compatibility is respected), as written before, vs writing something unencrypted to the file system.

Could it be easily done in Electron without node:fs?

There is no issue of using node's fs or other node modules in @electron. But the user's custom script is by design executed in an isolated environment (for this case I preferred QuickJS vs node's built-in VM module), so only pure JS-execution/computation with serializable input/output data is supported.

@tukusejssirs
Copy link

Clearly, there are two options for solving the issue like this. Putting the data to the file and then reading the data from it vs reading from cmd arg

Yeah, but unless you do it automatically, I cannot do it, as it might be a quite long string → E2BIG.

or executing the shell script multiple times or the data split to the pieces.

Yes, I can do this (echo '${data[0]}' > tmp.json && echo '${data[1]}' >> tmp.json && ...), but the performance is not ideal.

more privacy-friendly option

The most privacy-friendly and secure option would be if you added an option to enable (opt-in) individual notifications to each received email with sender and subject information directly in ElectronMail. No other way would be secure enough. That said, I understand that the notifacation server will nevertheless see the email sender and subject, but that is necessary, therefore this option should be opt-in. Yeah, I get it: it will cause additional maintenance burden, but I see no better option. It might no even require the additional database column I talked about.

vs writing something unencrypted to the file system.

As you see, even now I use echo to write the data unencrypted to the system, just because I want this feature and I miss it very much. I trust my system enough to let the notifications contain sender and subject.

There is no issue of using node's fs or other node modules in @electron.

All I wanted to say in my previous comment that you could write the command value to file and execute that file. If there is no problem to use node:fs in Electron it is an option, albeit, not the best, however, it would solve the E2BIG error.

@vladimiry
Copy link
Owner

The most privacy-friendly and secure option

I didn't talk about the most, but picked one from the two. The app is not going to persist the "notified" flag per email.

As you see, even now I use echo to write the data unencrypted to the system

Yes, and this is on you, not on the app. I don't want the app to write unencrypted data to the file system.

All I wanted to say in my previous comment that you could write the command value to file and execute that file. If there is no problem to use node:fs in Electron it is an option, albeit, not the best, however, it would solve the E2BIG error.

Same answer.

So the best option I'm ready to provide at the moment, is running exec on the array of commands. Since you know the command length limit for your system, you, technically, will be able to form the array all command items of which have allowed length.

@tukusejssirs
Copy link

Since you know the command length limit for your system

That is disputable, as during my additional tests today, I couldn’t use 131,011 chars after computing the JSON string. I could use only 83,499 chars, but even that is unstable. Currently, I am sticking to 50,000 chars. I have no idea why there is so much invariability, probably it counts some other parts of the function (or probably entire function).

Anywhere, for now I am about to ignore those messages that are outside of the 50,000 chars limit, as it is only the first run that need to parse older messages. Yes, it is not ideal algorithm, but mostly it’ll work. I’ll share it here after some tests done.

@tukusejssirs
Copy link

I have noticed an issue with my approach: if I get new emails while my computer is turned off, I would’t receive a new email notification at system startup. Could we run the function at ElectronMail startup?

@vladimiry
Copy link
Owner

The app downloads the missed emails first and after that triggers the notification if there are unread emails in the account. So if you have set up automatic login into the app and into the mail account, then there should be notification at the app startup with a normally small delay caused by downloading the missed emails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants