Fix notifications workaround

[stkrknds]

Since Messenger does not send notifications, the use of this workaround is necessary. This pull request fixes/updates the workaround in order to work with the new design. I've been testing it for a few weeks and it works fine. However, I would be grateful if someone else could test it as well and report back. Thanks in advance!

Fixes #1562
Fixes #1825

[Alex313031]

@stkrknds I wanna add this to my fork,

[dusansimic]

Sorry, I haven't yet had time to thoroughly test it but the code seems okay. One problem that exists is that only the chats that are loaded in the sidebar would be monitored for new messages. I'm not sure how it would behave when a new message is received but I'll check it out.

The ultimate solution would be intercepting the websocket packets that are sent to Messenger website and from it so we could both filter out seen signals and catch new messages as they arrive with much less overhead (compared to monitoring the ui). I started some work in #1688 but it requires completely reverse engineering the websocket protocol they designed so it will definitely take time.

[stkrknds]

Hello @Alex313031 . Sure, go ahead. Let me know if you need anything else from me.

[Alex313031]

@stkrknds I added it to my fork, and used the lower case y as recommended by @dusansimic

[dusansimic]

Once this gets merged, I'll publish a new release.

[dusansimic]

If I'm testing this correctly, this only shows a notification once a first new message is received. Once the second one comes in but the first one is not yet read, the notification won't be shown.

[Alex313031]

@dusansimic Did you merge this (in modified form) here > Alex313031/caprine-ng@main...sindresorhus:caprine:main

I merged @stkrknds commits into my fork, but now mine is diverged too much. Should I remove his, and add the commits of yours, or try to combine them together?

[dusansimic]

Did you merge this (in modified form) here

No, I've added my own commit for lowercase (y) (c0b49cd) and resolved a conflict with the main branch (c7eb882) but I'll need to look at the code better before finally merging into main. Right now we just need to have everything working and we'll clean up the redundant code before merge.

I merged stkrknds commits into my fork, but now mine is diverged too much. Should I remove his, and add the commits of yours, or try to combine them together?

IMHO, you could just leave it as is for now and merge the final code when we merge it into main. Then you could just revert commit you've made and merge the main branch from upstream. That would be the ideal process for the future, however that would mean the fork will have to wait for changes to land in main on upstream, or at least a green light for merging.

[Alex313031]

@stkrknds This patch appears to break on Linux. Whenever someone sends a message, it dings, but then the viewport freezes. The menubar still works, but clicking anywhere in the window does nothing. The only way to resolve it is to relaunch caprine.

On Windows it seems to work OK

[dusansimic]

@Alex313031

This patch appears to break on Linux. Whenever someone sends a message, it dings, but then the viewport freezes. The menubar still works, but clicking anywhere in the window does nothing. The only way to resolve it is to relaunch caprine.

I think this is related to #2033. That issue already exists without this patch. I still haven't had enough time to properly investigate the cause of that bug. It could be any number of things that are in Caprine but most likely one is some observer enters a loop because some element gets modified repeatedly.

[stkrknds]

Sorry for the late response.

@stkrknds This patch appears to break on Linux. Whenever someone sends a message, it dings, but then the viewport freezes. The menubar still works, but clicking anywhere in the window does nothing. The only way to resolve it is to relaunch caprine.

@Alex313031 I don't face this issue with this patch, are you sure that's caused specifically by it?

If I'm testing this correctly, this only shows a notification once a first new message is received. Once the second one comes in but the first one is not yet read, the notification won't be shown.

@dusansimic Yes you are right. I'm trying to make it send notifications for every new message, but it is quite difficult because, taking the worst case scenario(1 emoji & new message identical to the previous one), the only mutation that is observed is the time(next to the message). The issue is that, I don't know when such a mutation can be classified as unread, considering that the time changes even if no new message arrives.

[Alex313031]

@stkrknds @dusansimic Reverting the patch makes Caprine work normally again. I made two clones of my repo, and in one, I reverted the commits. I then use npm ci to make sure that the package-lock.json was used, to remove variables. Running the one with the patch leads to caprine freezing.

Also, @dusansimic I am aware of #2033 I had this issue too, however, it was resolved until now.

[dusansimic]

@stkrknds

Yes you are right. I'm trying to make it send notifications for every new message, but it is quite difficult because, taking the worst case scenario(1 emoji & new message identical to the previous one), the only mutation that is observed is the time(next to the message). The issue is that, I don't know when such a mutation can be classified as unread, considering that the time changes even if no new message arrives.

That is indeed problematic. We could use two things to check if a message is new. If the chat gets promoted up the chat list, it means that it's a new message. And if the text of the message is changed, it's a new message. With those two cases, we could fairly reliably determine if some change in the ui is a new message. There is an extreme case when the new message is the same as the old one and the chat is at the top of the chat list already but I'm guessing we could overlook that for now since that is quite a special case.

[Alex313031]

@stkrknds @dusansimic I'm gonna try to bisect and see if I can narrow down the line(s) that are causing this.

[Alex313031]

@stkrknds @dusansimic It is definitely something you guys did
I narrowed it down to one file.

https://github.com/sindresorhus/caprine/blob/main/source/browser/conversation-list.ts the conversation-list.ts

This commit broke caprine > 0d38a71

The other two files are fine.

I made two clones of the repo. They were both identical except one had the conversation-list.ts file from the previous revision. That one works fine. The one with the new conversation-list.ts file causes caprine to freeze. IDK how to fix it, but now that you know what it is, maybe yall can figure it out.

To rule out environment causing the problem, I did all my testing on a vanilla Ubuntu VM.

[stkrknds]

Made some changes. Needs some more testing. The code is a bit messy, but will clean it later.

@Alex313031 I can't reproduce the issue. Don't know if it happens only on specific distros(?). I'm running arch and it's working fine.

[Alex313031]

@stkrknds You are running Arch with what DE and WM?

And OK I will try it out.

[Alex313031]

@stkrknds @dusansimic This seems to fix it! Cool.

[stkrknds]

@Alex313031 I'm using the awesomeWM. It's good that it seems to be working fine. I will test it some more over the weekend and hopefully clean the code a bit.

[Alex313031]

@stkrknds Yup, once again I can verify that the last commit is what broke it, cloning and building main breaks it, but cloning, git checking out the notifications branch you made and building works fine again. I think it was some sort of race condition, based on a quick glance at the changes you made. Is that a correct assumption? Im not very good at .js, and typescript is even harder for me, so it was hard to decipher some of the changes. Not to mention that we have to use selectors with strange names that result from Facebook mangling/minifying their code.

Waiting for the code cleanup, and then will rebase my fork on it and make some releases. My fork is mostly identical, except I enable all features (including "dev only" ones), I made a new "versions" window in the "About" menu item that shows versions of Electron, Chromium, Node, V8, and dependency only node_modules. And my fork is still using Electron 22 - for Windows 7/8/8.1 and Ubuntu 16.04/Debian 8 support. Since Electron 22 is still supported upstream, I feel that most electron apps should keep using it unless there is a specific need for a newer version. (Which there usually isn't since there hasn't been any API changes except for the Mac window button position.)

Another minor change that I think I will make a pull request for, is setting the icon explicitly on linux instead of relying only on the packaged version's .desktop file. What I mean is that for Windows and Mac, the icon is set in the code, and will be used even when running npm run start. On linux, it is not set at all, and will show a blank icon when running in dev mode. Only when making a .deb or .appimage is the icon set, but again, it is not actually set at the electron level and only via the .desktop file. Directly running /opt/Caprine/caprine leads to no icon. I fixed this by making a 64x64 .png and placing it in the //static dir, and then making some constants in the 'constants.ts' file for the icon's location(s), with the value depending on platform, and then setting icon: iconPath, in the BrowserWindow webPreferences.

[Alex313031]

@stkrknds Also, it DOES appear to be DE/WM specific. Installing Arch and awesomeWM, openBox, or XFCE works fine (I tested in a VM, just to investigate your experience with it.) However, KDE and GNOME (which Ubuntu uses a modified version of) breaks it. Maybe because the former DE's just use libappindicator or xdg-notify, whereas KDE and GNOME have their own DE-specific notifications system.

[Alex313031]

@stkrknds @dusansimic How goes the testing on this? Did you guys read me above comments?

Will it be merged? I've been using it on Windows and Linux with no problems.

[stkrknds]

@Alex313031 Sorry, I'm going to clean the code this weekend.

[stkrknds]

Okay, I think it's pretty good now, tested it and it works fine.

[dusansimic]

Okay, I think it's pretty good now, tested it and it works fine.

It doesn't seem to work for receiving multiple messages. Also, the contents of the messages is not shown but it might be just on my machine (I run GNOME, but the weird thing is that it worked when I tried it last time).

[Alex313031]

@stkrknds Can confirm, seems broken. I hate this cat and mouse chasing with facebook everytime they decide to update their stuff.

[stkrknds]

An update: Right now the only case where notifications are not working is when the same emoji is being sent multiple times continuously. I can't find a way to fix this but I think it is not really that important. I would appreciate it if you could confirm that, with the exception of this particular case, notifications are working as intended. Thanks.

[dusansimic]

Could we move all the query selectors to the selectors.ts file and add comments about what exactly they are meant for. It would be far easier to maintain this feature and fix them in case they break.

Other than this, pr looks good.

[stkrknds]

@dusansimic Made some changes. Created variables for the selectors and fixed another bug that I found.

The freeze issue(#2053) might be caused by the currently broken countUnread function. By fixing this workaround we may indirectly solve the issue. I can't say for sure, since I can't reproduce it.

[dusansimic]

@stkrknds I just wanted to push this out quickly so I made a few changes myself. I've moved the selectors to the selectors.ts file. Also, I've changed the any type of the alreadyChecked array to string[] which caused some type errors and I've fixed them by skipping an iteration in case href is null or undefined (https://github.com/sindresorhus/caprine/pull/2051/files#diff-07559914018c1e28768b2f3ce54fd49cc8071201423637fad35b372c318ebed7R224). Could you double check that this doesn't affect negatively this feature.

If everything looks good, we're merging this and I'll push out a release today.

[stkrknds]

Works fine.

[dusansimic]

Thanks for the help!

[mzso]

I had no notifications so far. Does it look for english text or something? Facebook appears localized in Caprine for me.

[stkrknds]

@mzso No, the language shouldn't matter. Right now there are some selectors that need to be updated, so notifications may not work. Did you use the version 2.59 a week ago? I think notifications worked then.

[mzso]

@stkrknds I think I used it as soon as it came out.

[dusansimic]

@stkrknds

I think notifications worked then.

They did, the selectors seem to be broken.

[mzso]

It seems to be working now. At least I saw the tray icon turn purple a couple times.