Fix preferences opening on startup

[meszarosdezso]

This PR adds a boolean flag for the updateDoNotDisturb and openHiddenPreferences functions, to prevent the preferences window from opening on startup. I know its not the best solution, but gets the job done for now(:

fixes #1547

[dusansimic]

Hi @meszarosdezso. I think this is not a good way to go about fixing this. In this case you stop preferences window being open on startup which is needed in order to set some settings (mute notifications). By doing this, the user defined settings are not respected since we don't set them, we just avoid opening preference window all together.

I'd rather resolve the root of the problem which i think is that preferences window is not being closed at all. Check openHiddenPreferences function in browser.ts

caprine/source/browser.ts

Line 280 in 0e5175b

async function openHiddenPreferences(isNewDesign: boolean): Promise<boolean> {

It seems that in the old design, preferences window would be closed automatically or some thing like that since we don't call closePreferences function there. This would probably get fixed by explicitly calling closePreferences if the new design is used. Also, all the work regarding styles is pretty much unnecessary for new design so it would be good to avoid it on new design. That would mean checking weather new design is false when we want to set styles in the document and checking weather new design is true when we want to close preferences.

[meszarosdezso]

Thanks, I'll take a look later today!

[meszarosdezso]

Okay, so after some professional console.log-ing I found that isNewDesign is sometimes undefined, which is falsy. it happens that the callback parameter for toggle-mute-notifications (which is typed to IToggleMuteNotifications) is false, so destructuring isNewDesign from it results in undefined (browser.ts, line 281).

I couldn't find where the props for IToggleMuteNotifications passed (isNewDesign and defaultStatus), any idea, and could this be the root cause for the issue?

[dusansimic]

Okay, so after some professional console.log-ing I found that isNewDesign is sometimes undefined, which is falsy. it happens that the callback parameter for toggle-mute-notifications (which is typed to IToggleMuteNotifications) is false, so destructuring isNewDesign from it results in undefined (browser.ts, line 281).

This seems to happen because when toggle-mute-notifications is called from index.ts on lines 397 and 493. On line 397, nothing is passed as a parameter which is fine on old design but now we need to pass an object

{ isNewDesign }

where isNewDesing holds a boolean value. On line 493, config value is passed as a parameter when an object should be passed:

{ isNewDesign, defaultStatus: config.get('notificationsMuted') }

isNewDesign could be got with a call to check-new-ui:

const isNewDesign = await ipcMain.callRenderer<undefined, boolean>(mainWindow, 'check-new-ui');

and it should be added on line 382 (after createMainWindow call) and updateAppMenu call should be moved below isNewDesign since the real value could be passed then. The final result should look something like this:

await Promise.all([ensureOnline(), app.whenReady()]);
mainWindow = createMainWindow();
const isNewDesign = await ipcMain.callRenderer<undefined, boolean>(mainWindow, 'check-new-ui');
await updateAppMenu({isNewDesign});

In that case I think there is no need to have the same code on 445 since we could use the variable from outside of dom-ready context.

The thing that actually makes all of this work look quite pointless (but I hope you'll go through with this pr) is that the toggle-mute-notifications doesn't do anything because notifications are not implemented on new ui. This means that we should avoid displaying it as an option at all since it doesn't exist in Messenger website.

could this be the root cause for the issue?

toggle-mute-notifications issue is a different issue which you just found on accident 😅. The real cause of preferences window being shown for a second i think is it not being hidden by styles. Also, I've just realized that preferences are closed in toggle-mute-notifications so there is no need for closing them in openHiddenPreferences. You'll just need to add the style to hide preferences window.

To sum everything up, wrong parameters are being passed in index.ts which can be fixed by getting information about new design earlier in the code and passing it to renderer calls. Options for muting notifications should not be visible on new design. Preferences window can be fixed by adding a utility class for hiding the window if class is added to body tag #1552 (comment). Also there is no need to execute the logic in toggle-mute-notifications since none of it will work under new design, so a single if to exclude any

This is quite a long comment so feel free to ask for clarifications if there is any need.

Edit:
In toggle-mute-notifications the notificationCheckbox.click() call probably throws and error which because is not handled, stops the function and preferences window is never closed. For now we could just wrap the whole notificationCheckbox logic in an if (!isNewDesign) so nothing should happen. I think there would be less refactoring if that feature gets implemented if we just ignore all the logic if the design is not new.

[meszarosdezso]

The final result should look something like this:

await Promise.all([ensureOnline(), app.whenReady()]);
mainWindow = createMainWindow();
const isNewDesign = await ipcMain.callRenderer<undefined, boolean>(mainWindow, 'check-new-ui');
await updateAppMenu({isNewDesign});

Calling 'check-new-ui' there never resolves the promise, making the app stuck before even opening the window. /:

Putting that call right before calling setNotificationsMute seems to work, the parameters are now the expected boolean values, now the problem is that in the updateDoNotDisturb function soundCheckbox is null, so soundCheckbox.checked throws an error. I guess this is because that checkbox is not even there in the preferences window on startup... is it added asyncronously? (see attached screenshots)

After opening it manually from the menu the checkbox is there.

Also, I've just realized that preferences are closed in toggle-mute-notifications so there is no need for closing them in openHiddenPreferences.

true, lol.

notificationCheckbox.click() call probably throws an error

I don't see any errors related to notificationCheckbox.click(), athough notificationCheckbox is null (I guess it never gets to that line). I added the if (!isNewDesign) for safety.

Preferences window can be fixed by adding a utility class for hiding the window if class is added to body tag #1552 (comment).

The preferences window is now hidden automatically, would this make it not even flash for a second? Where should I add/remove the class to/from the body?

EDIT: I guess I misunderstood your comment, I see now that the soundCheckbox should not event be there at all in the new design, so I guess that whole call at the end of updateDoNotDisturb could go in a if (!isNewDesign) block, wyt?

updated updateDoNotDisturb would look like:

async function updateDoNotDisturb(isNewDesign: boolean): Promise<void> {
	const shouldClosePreferences = await openHiddenPreferences(isNewDesign);
	
	if (shouldClosePreferences) {
		closePreferences(isNewDesign);
	}
	
	if (!isNewDesign) {
		const soundsCheckbox = document.querySelector<HTMLInputElement>(messengerSoundsSelector)!;
		toggleSounds(await ipc.callMain('update-dnd-mode', soundsCheckbox.checked));
	}
}

[dusansimic]

Putting that call right before calling setNotificationsMute seems to work

Okay, now I think we've got the best solution. We should leave the check-new-ui call in dom-ready and let it be used for that context. There should also be another check-new-ui call in click handler for macOS dock menu item.

caprine/source/index.ts

Line 396 in 0e5175b

async click() {

This way we will check for new ui on each button click which is not ideal but it is the cleanest solution. If this is what you meant by adding check-new-ui right before setNotificationsMute then we're good on that.

now the problem is that in the updateDoNotDisturb function soundCheckbox is null, so soundCheckbox.checked throws an error. I guess this is because that checkbox is not even there in the preferences window on startup... is it added asyncronously? (see attached screenshots)

Here we have two problems.
First one is that those options and toggles are as you've said, added asynchronously. I'm not sure why there are not added when we open preferences with JS code and they're added when we manually open the preferences but this will need to be researched. I think that is probably another issue which we could leave for another PR, currently the main problem is hiding preferences window on startup.

I guess I misunderstood your comment, I see now that the soundCheckbox should not event be there at all in the new design, so I guess that whole call at the end of updateDoNotDisturb could go in a if (!isNewDesign) block, wyt?

That is basically it, except in order for this to work on old design, the logic must be between opening and closing of preferences window. Just move if (!isNewDesign) block above if (shouldClosePreferences) block. That way, logic will be executed before preferences window is closed which is intended behavior.

The preferences window is now hidden automatically, would this make it not even flash for a second? Where should I add/remove the class to/from the body?

I'm not really sure what you mean by preferences window is now hidden automatically.

You'll need to first add that utility class to browser.css. It should look like this.

/* A utility class for temporarily hiding preferences window */
html.hide-preferences-window [data-pagelet=root] > div > div:last-child {
	display: none;
}

This will hide the preferences window and overlay when hide-preferences-window class is added to the html element. To activate this class, just add classList.add('hide-preferences-window'); before preferences window is opened in openHiddenPreferences and remove it classList.remove('hide-preferences-window'); after closing in closePreferences. This should only be done on new design. By doing this you'll add and remove class for hiding preferences window so when a user opens preferences manually, window will be visible.

[meszarosdezso]

We should leave the check-new-ui call in dom-ready and let it be used for that context.

This still does not work, as I mentioned above, that call for some reason never resolves, and the app doesn't start (the window is not appearing, terminal is stuck at tsc && electron ., any idea why is that?

Just move if (!isNewDesign) block above if (shouldClosePreferences) block

Oh, of course, that makes sense 😅

I'm not really sure what you mean by preferences window is now hidden automatically.

It opens for a split second, and disappears.

The fix with the utility class works, tho I still see only the opaque white overlay flashing for a bit.

[dusansimic]

This still does not work, as I mentioned above, that call for some reason never resolves, and the app doesn't start (the window is not appearing, terminal is stuck at tsc && electron ., any idea why is that?

I'd just like to clarify this. I meant to revert to what is being done on main branch regarding index.ts, except with passing correct parameters to setMuteNotifications. Caprine should open when calling check-new-ui in dom-ready and in click handler which is currently done.
If npm run start hangs at tsc && electron ., that would mean Caprine is started but it's not shown. To show it you can just run Caprine like you normally would (the installed version, not development version). That would make Caprine window visible because it will automatically register that Caprine is already running. Try that out.

It opens for a split second, and disappears.

The fix with the utility class works, tho I still see only the opaque white overlay flashing for a bit.

I guess that's not what we want since preference window should not be shown at all. I'll check that out later today.

[meszarosdezso]

I guess we need to target the element's parent element.

afaik, there's no way to do that with css, I guess something like

document.querySelector('[data-pagelet=root] > div > div:last-child').parentElement.style.display = 'none';

[dusansimic]

I guess we need to target the element's parent element.

afaik, there's no way to do that with css, I guess something like

document.querySelector('[data-pagelet=root] > div > div:last-child').parentElement.style.display = 'none';

That line actually hides all elements in the app since parent element of overlay is the wrapper around the whole ui. The utility class should be doing all the work but for some reason it showing the overlay for a split second.

I've added a review on that par since I've just now realized that order of the calls was wrong. You should first close the preferences and only then remove the utility class.

What might also make a problem is that once close preferences action is done (clicking the close button) there is some time between click() function returning and preferences actually closing. You'll need to track if preference window is actually closed and only then remove the utility class. Here is how it is done with a mutation observer on another element.

caprine/source/browser.ts

Line 176 in 0e5175b

const sidebarObserver = new MutationObserver(records => {

Instead of observing the sidebar, you should observe the wrapper for overlay [data-pagelet=root] > div > div:last-child and once an element is removed from it, callback function will be triggered which will check if div element was removed and if it was, it will remove the utility class and stop observing.

[meszarosdezso]

You'll need to track if preference window is actually closed and only then remove the utility class

That was it! Now it seems to work just fine, thanks!

[dusansimic]

This look good to me. I've just refactored a few things. Thanks for the fix @meszarosdezso! Sorry for all the back and forth about tiny bugs 😅.

// @sindresorhus

[meszarosdezso]

No probs, thanks for your help, I way too underestimated the problem, but it was fun to work on this and get to know the internals better(:

Sorry for the unnecessary Boolean castings I saw it somewhere in the code, thought it's for typescript :p