-
-
Notifications
You must be signed in to change notification settings - Fork 560
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
Add support for replying via notifications on macOS #726
Conversation
Alright, I'm testing this change, here's what I found:
I realize that number 4 is a large task. Let's say that after 1-3 are fixed, we'll just try it out and see if the current code is reliable enough. Or maybe doing the 4 will be necessary to fix previous issues… 🤷♂️😅 |
Hi @CvX Thanks for looking into it, I'll fix 1-3 and then look into 4 again, I really struggled with nr. 4. I wanted to use the callback but couldn't get it to work without doing hacky stuff, so I implemented the least hacky solution that would work, but I'll look into it again. I should be able to get it to work because as you said, edit: const facebookLocales = require('facebook-locales');
const userLocale = facebookLocales.bestFacebookLocaleFor(app.getLocale().replace('-', '_')); or should I move the above two lines to |
I just tested these again and they seem to work on my end, with "the window closed", you mean when you press the (x) button to just close the window right? Does the text field contain the message you intended to send? |
Hm, can't reproduce it now either. Maybe because of the emoji issue I mistook sent message for an earlier one… |
Okay so the emoji thing is a though one. document.querySelector('div[contenteditable="true"]').focus();
document.execCommand('insertText', false, 'message 😂😂'); but then it would display:
So I used: const event = document.createEvent('TextEvent');
event.initTextEvent('textInput', true, true, window, message, 0, locale); instead. However, this is causing issues with the emoji being pasted (and this function is deprecated, so shouldn't be used anyways, I figured that out yesterday). I've seen the error message on the internet on numerous forums, but it's most of the times related to messenger itself or an extension. I has also been mentioned in #316. Does anyone else have an idea? I'll keep looking into it. edit: edit 2: |
I have a solution. It's a bit of a hack, but after a couple of hours of research I couldn't find anything better. What I did is a combination of both your attempts. 😃 const inputField = document.querySelector('[contenteditable="true"]');
if (inputField) {
inputField.focus();
const event = document.createEvent('TextEvent');
event.initTextEvent('textInput', true, true, window, 'placeholder', 0, locale);
inputField.dispatchEvent(event);
document.execCommand('selectAll', false, null);
document.execCommand('insertText', false, message);
} The clues were all there! 😄 Btw, while working on a solution, I thought of an edge case that should be handled. If you have an unsent message typed in, the app should save that message, do the reply logic, and then restore that message. |
Thanks for looking into it! That would certainly work and is a great workaround! Another solution to this that I thought of would be to quickly switch between your previous convo and back, because then the error goes away and the inputted text is still present and ready to be sent, this solution is equally hacky but doesn't make use of the deprecated API (and I haven't tested how reliable it is yet). I was still looking for another way but so far didn't have any luck (even posted a stackoverflow question).
I agree, that's a good idea, if it's possible to implement it 😅 @sindresorhus Any thoughts on this issue? Should we just implement a somewhat hacky solution? |
The conversation-switcheroo is a nice solution. It is certainly much cleaner and more straightforward than using deprecated events and a couple of text-manipulating commands. There are two downsides that come to mind, though:
Restoring the previous message is possible, just did a quick test. The bottom line is: const messageInput = document.querySelector('div[contenteditable="true"]');
let previousMessage = messageInput.textContent;
messageInput.focus();
if (previousMessage) {
document.execCommand('selectAll', false, null);
document.execCommand('delete', false, null);
}
document.execCommand('insertText', true, message);
nextConversation();
previousConversation();
sendMessage();
if (previousMessage) {
document.execCommand('insertText', true, previousMessage);
nextConversation();
previousConversation();
} |
Yeah, I mean it's not the most elegant solution to use a deprecated API but I don't see any other way to currently get it done otherwise. So should I just implement it with your workaround then? |
Yeah, I suppose so. We can always change it if we come up with a better one. 🙂 |
Alright, I'll take a stab at implementing it tomorrow, thanks for the help with this. |
Hi @CvX ipc.on('notification-reply', (event, data) => {
const previousConversation = getIndex();
window.postMessage({type: 'notification-reply', data}, '*');
window.addEventListener('message', async ({data: {type}}) => {
if (type === 'reply-callback') {
await sendReply(data.reply);
if (previousConversation) {
selectConversation(previousConversation);
}
}
});
}); inside if (type === 'notification-reply') {
window.postMessage({type: 'reply-callback'}, '*');
} like that right? Or am I missing something? |
Originally implemented by @JoniVR in 97f7e3d (PR sindresorhus#726).
Yeah, but you'll have to move the |
It's probably going to be an issue to switch to the previous conversation then, unless we send it back and forth through |
I think it's about ready now as far as I can tell, could you do some testing of your own? I'm asking some people to send me messages so I can try replying when notifications from multiple people are coming in. Might also want to check Windows and Linux again to see if anything is broken on that end. edit: Yeah, I can't find any issues on my end, messages are sent to the correct people, emojis work, autoswitching back works, saving and restoring previous messages in the field work,... let me know if there's anything else you want me to look into. |
I'm currently testing this. I've found some issues, but they're not directly caused by this PR:
|
I'm not having this issue, probably because I've disabled my active status by default. But it makes sense. I'll look into it.
I don't think we can do much about that, but I also don't think people use bots that much (or at least won't reply to them through notifications that much, at least I don't)
I had noticed both of these issues during my testing with the notifications on several occasions during this PR, but I just thought it was due to me triggering a lot of notifications and Apple somehow limiting it to prevent spam, but what you're saying makes a lot of sense. I'll look into fixing the first issue, nr 3, 4 are something I'll research too, but again, I'm not an expert at electron, so my experience with it is pretty limited. |
Update on issue 4: It's a bug in Electron. Turns out Notification objects are being prematurely garbage collected… so in many cases clicking, replying, or closing a notification doesn't trigger anything because the underlying Notification object (and its callbacks) is gone. I've pushed a workaround to this branch. About other issues: 1: Gotta fix that, will look into that next. |
I had accidentally removed it while merging
Hi @CvX Any updates on this? Did you create an issue on electron (so I can track it myself)? |
@JoniVR Sorry, I've been away for a while. Here's the issue on the Electron repo: electron/electron#16922 About the issue "1.", the best way to reproduce it is to chat with a bot (I've created one to test this PR, but I'm waiting for it to go through FB's review)
|
@CvX |
Yeah, Meme Generator either went down or it blocked us 😉 That's what prompted me to do my own bot (review still pending) |
Alright, Messenger Ping Bot is live! 😄 Just search for it in the app and send anything. |
I've pushed a fix: apparently the "Send" button lost one of its CSS classes recently. 😉 I was also investigating the issue with max 3 notifications. Turns out it's a "feature". 😉 Messenger tracks how many unread messages are in each thread and show notification only for the first 3 messages in each one. I haven't found any reliable way to work around it, so I'd say there's no reason to try to fight it and just accept it. 😉 There's a minor edge case that also comes to mind that I don't think I've mentioned before. Let's say you have unread messages in the currently active conversation. You reply via notification to a different conversation. But the first one is marked as seen/read even though you haven't. And with that, I think this PR is almost ready to be merged? 🙂 |
Alright, makes sense, it was really weird that it did that anyways, couldn't really find a reason why it would happen.
Would it perhaps be possible to block the "seen indicator" in this case? The app already does this as a setting so perhaps that could be a workaround? I'm just guessing here. Anyways, thanks a lot for all the help and feedback with this, I've been extremely busy lately so it's very helpful! |
Yeah, that might be a good solution. 🙂 I'll try it later! |
Sounds good to me, would be great to have this feature! Looking forward 👌🏻 |
Alright, no issues on Windows 10! 🚀 |
Works great for me 👌 Thanks for implementing this, @JoniVR 🙌 |
Is it possible that you forgot to add the other assets to the 2.29.0 release? Or is it normal that they don't show up yet? Thanks for everything. |
Sometimes |
Hi,
First of all thanks for this great application.
Fixes #298.
Some things:
Also, I made this the default, not configurable yet (since I thought it didn't make much difference in the user experience), but if you want me to I can add it as an extra option.