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

fix for window not being shown with wayland in same cases by switching to the 'did-finish-load' event instead of 'ready-to-show' #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

echoabhishek
Copy link
Owner

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A npm run ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

related issues:

Description

on some setups signal is never shown due to electrons ready-to-show event is not firing.

I've found that people use did-finish-load instead due to (probably) this bug (electron/electron#25253 (comment)).

After a deep rabbit hole into electron and the chromium source code I could not determine why this event is not fired, but both events should be more or less equal from their timing in our case, so it is a workaround, but can be used permanently.

https://www.electronjs.org/docs/latest/api/browser-window

Using the ready-to-show event

This event is usually emitted after the did-finish-load event, but for pages with many remote resources, it may be emitted before the did-finish-load event.

setup with which I could reproduce the issue and verify the fix

  • latest main (git checkout v7.35.0-alpha.1)
  • electron v33.1.0
  • node v20.18.0
  • Sway Desktop (wayland/wlroots-based)
  • ELECTRON_OZONE_PLATFORM_HINT=auto environment variable

… to the 'did-finish-load' event instead of 'ready-to-show'

related issues:
signalapp#6368
signalapp#6740
signalapp#6966
electron/electron#25253 (comment)

After a deep rabbit hole into electron and the chromium source code I could not determine why this event is not fired, but both events should be more or less equal from their timing in our case:

https://www.electronjs.org/docs/latest/api/browser-window
> ### Using the `ready-to-show` event
> This event is usually emitted after the did-finish-load event, but for pages with many remote resources, it may be emitted before the did-finish-load event.
Copy link

robointern1 bot commented Jan 25, 2025

Technical Documentation: Event Timing Issue in Signal Desktop

Critical Issues

  1. Event Timing Mismatch: Switching from 'ready-to-show' to 'did-finish-load' may introduce timing issues. The 'did-finish-load' event can fire before all resources are loaded, which may lead to visual glitches or functionality problems if the window is presented too early.

  2. Inconsistent Behavior: While this change may resolve the issue for Wayland users, it could create inconsistencies for other platforms. The pull request (PR) does not address the impact of this non-Wayland environments.

Suggestions

  1. Error Handling: Introduce error handling for the 'did-finish-load' event, similar to the existing handling for 'ready-to-show'.

  2. Logging: Implement a log statement to indicate when the 'did-finish-load' event is fired, aiding in debugging. Example code snippet:

    mainWindow.webContents.once('did-finish-load', async () => {
      getLogger().info('main window did-finish-load event fired');
      // ... rest of the code
    });
  3. Configuration Option: Make this behavior configurable, allowing users to select between 'ready-to-show' and 'did-finish-load' based on their environment. Example code snippet:

    const windowReadyEvent = config.get('useDidFinishLoad') ? 'did-finish-load' : 'ready-to-show';
    mainWindow.webContents.once(windowReadyEvent, async () => {
      // ... existing code
    });

Summary

The PR addresses a critical issue where the Signal Desktop window fails to show for Wayland users, proposing a switch from 'ready-to-show'did-finish-load'. This fix could resolve the immediate problem but may lead to timing-related issues in other environments.

Recommendations Before Merging:

  1. Conduct thorough testing on non-Wayland environments to ensure no regressions are introduced.
    configurable solution to allow switching between events based on user environments.
  2. Add necessary error handling and logging for the newly introduced event.

Additional Investigation

It would be beneficial to investigate why 'ready-to-show' is not consistently firing on Wayland and potentially report this issue to the Electron project for a more robust long-term resolution.

Implementation Steps

  1. Implement the change from 'ready' to 'did-finish-load' in the appropriate files.
  2. Test the change thoroughly in both Wayland and non-Wayland environments.
  3. Verify that the window across all scenarios.
  4. Check for any potential side effects or regressions, and make adjustments as necessary.
  5. Commit the changes and push them to the repository if everything functions as expected.

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

Successfully merging this pull request may close these issues.

2 participants