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

Verify that notificationCheckbox is non-null #2064

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

mquevill
Copy link
Collaborator

Since the notifications are currently somewhat broken, this is a temporary patch that returns false if the selector is null. I would be wary of simply toggling the "Do not disturb" mode, since it normally sets a time limit for that mode, which may require more of an overhaul of notification options. I'm open to handling this differently, but it does handle this promise/error.

The error on the current version:

(node:10656) UnhandledPromiseRejectionWarning: TypeError: Cannot read properties of null (reading 'checked')
    at ...\caprine\dist-js\browser.js:208:34
    at async EventEmitter.listener (...\caprine\node_modules\electron-better-ipc\source\renderer.js:46:34)

@dusansimic
Copy link
Collaborator

A problem is that those options don't show up when preferences modal is programmatically open. When it's opened manually by a user, both "Do not disturb" and "Notification sounds" as well as "Active status" show up. We still haven't figured out why it happens but it's the root cause of this issue.

This workaround merely avoids throwing an error when the options don't show up which is both good and bad. I'd rather have it log at least some message so we could know that it's happening. It would be something like what was suggested in #1892. I haven't still had time to finish up that patch but I'll try to work on it in following weeks and push at least something for start.

@stkrknds
Copy link
Contributor

Hello. I've been working on this. I think this should be merged since there is no notification checkbox right now in messenger's preferences.

A problem is that those options don't show up when preferences modal is programmatically open. When it's opened manually by a user, both "Do not disturb" and "Notification sounds" as well as "Active status" show up. We still haven't figured out why it happens but it's the root cause of this issue.

Yes this is true, but even when these options show up, there is simply not a notification checkbox(the sounds checkbox I think is supposedly handled by the function toggleSounds, so it's unrelated). By the way, I found a fix for this. If you wait for the menu to get displayed, then the preferences always include the missing options. You can achieve this by changing line 68 in browser.ts to:
await elementReady(`${selectors.conversationMenuSelectorNewDesign} > div > div > div> div > div:nth-child(3)[style*="display: block;"]`, {stopOnDomReady: false});
Of course by changing this, the menu becomes visible for a split second. As far as I can tell, the function responsible for hiding it is the withMenu, that adds the hide-dropdowns class specifically for this purpose, which needs to be fixed as well.

@dusansimic dusansimic merged commit 86aced7 into sindresorhus:main Sep 15, 2023
1 check passed
@mquevill mquevill deleted the NotificationCheckbox branch September 21, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants