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 "Open in new window" context menu action in release builds #2873

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

absidue
Copy link
Member

@absidue absidue commented Nov 18, 2022

Fix "Open in new window" context menu action in release builds

Pull Request Type

  • Bugfix

Related issue

Partially addresses #2832

Description

In the release builds, all files are accessed using the file: protocol, including the index.html file. Interestingly how the new URL() class handles file URLs differs depending on the implementation. In node the origin field is null, in Firefox it's a string with null in it "null" and in Chromium it's file:///. In release builds, the current implementation was checking if the linkURL contained null, which it never did, so it always hid the context menu entry.

In dev builds the files are served from the webpack dev server (http://localhost:9080) so the previous implementation worked fine there.

Testing

  1. yarn build
  2. run the built release build
  3. right click on links and check that the Open in New Window context menu entry shows up

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.18.0

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 18, 2022
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 18, 2022 15:21
@@ -31,7 +31,7 @@ function runApp() {
},
{
label: 'Open in a New Window',
visible: parameters.linkURL.includes((new URL(browserWindow.webContents.getURL())).origin),
visible: parameters.linkURL.startsWith(browserWindow.webContents.getURL()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should add code comment to document this pitfall

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment now.

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (i.e. yarn build:arm64)

@FreeTubeBot FreeTubeBot merged commit c8d37cd into FreeTubeApp:development Nov 21, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 21, 2022
@absidue absidue deleted the fix-new-window-context branch November 21, 2022 22:33
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.

5 participants