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

Fixed opening of links in an external app. #1982

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

mikeller
Copy link
Member

Fixes #1980.

Open the app Window as a NW.js window, which is what NW.js was designed for.
This will re-enable the new-win-policy NW.js event handler that is required to open links in an external app.

Also moves the close handler into main.js, so that eventPage.js does not need to be used in NW.js any more.

@mikeller mikeller added this to the 10.7.0 milestone Apr 25, 2020
@mikeller mikeller mentioned this pull request Apr 25, 2020
@mikeller mikeller force-pushed the fix_link_opening branch 3 times, most recently from bb97ce0 to 410fa45 Compare April 25, 2020 09:08
McGiverGim
McGiverGim previously approved these changes Apr 25, 2020
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Is good to me, good fix @mikeller! I tried to do something similar yesterday but I suppose I did something wrong 🤪

@mikeller
Copy link
Member Author

@McGiverGim: It was a lot of trial and error. 😅

@Docteh
Copy link
Contributor

Docteh commented Apr 25, 2020

What about the minimum size of the window? I like the dark task bar on windows 10, its probably respecting dark mode now.

image

image

  "window": {
    "show": false,
    "icon": "images/bf_icon_128.png",
    "id": "main-window",
    "min_height": 550,
    "min_width": 1024
  },

@McGiverGim
Copy link
Member

True, we have a lot of tabs and css working for this minimum sizes. Maybe when merged the Cordova PR it adjust better, but for this release I think is better to have it.

package.json Outdated
"innerBounds": {
"minWidth": 1024,
"minHeight": 550
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@McGiverGim This innerBounds section not being effective is what I'm talking about with the min_width/min_height suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we must try to open the window from JavaScript and pass them as parameters, similar to the Chrome version.
My PR opened it from JavaScript but I have not passed parameters.

@mikeller
Copy link
Member Author

@Docteh: Sorry, my bad. I simply copied the window settings from eventPage.js without verifying that they are actually in the correct format for NW. Fixed now - seems to be working fine.

Docteh
Docteh previously approved these changes Apr 26, 2020
@TheIsotopes
Copy link
Contributor

TheIsotopes commented Apr 26, 2020

@mikeller i tested this and found a bug
in cli tab the buttons load from file and save to file does not work.

EDIT:
in firmware flasher tab the button load firmware (local) also not working.

EDIT again:
all buttons with local load and save function does not working.

@McGiverGim
Copy link
Member

Maybe we are using chrome load/save features and not nw features?

@McGiverGim
Copy link
Member

I confirm:

chrome.fileSystem.chooseEntry({type: 'saveFile', suggestedName: filename, accepts: accepts}, function(entry) {

and I suppose we must use: https://nwjs.readthedocs.io/en/nw13/References/File%20Dialogs/

Maybe we must make a wrapper similar to the one of Clipboard.js

@McGiverGim
Copy link
Member

It seems that since nw2, as we suspected, the window opened from the background page is treated as a background page. This is the error when trying to open a file:
image

So the problem starts here...

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
1.0% 1.0% Duplication

@mikeller
Copy link
Member Author

From the look of it this goes deeper than what we are doing with the clipboard - without the Chrome filesystem API we'll have to use the node filesystem module, and with this we'll have to move to requiring modules.
I have disabled NW2 for now - the scope of moving to NW2 seems to be too big and risky to do before the upcoming release.

Confirmed working:

  • loading links in an external browser;
  • opening files.

@TheIsotopes
Copy link
Contributor

confirmed, working again 👍

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Yes, it has a lot of sense to disable nw2 for this release.

@mikeller
Copy link
Member Author

@MaximeAmadasi: You should probably be aware of this, as it changes some of the underlying code that you are working on as well, so you can rebase on this as soon as it has been merged.

@WalcoFPV
Copy link
Contributor

True, we have a lot of tabs and css working for this minimum sizes. Maybe when merged the Cordova PR it adjust better, but for this release I think is better to have it.

The cordova PR will make the headerbar a flex container. It's design to work with a miminum window width of 1024px. And when the window width is below 575px, it switchs to the mobile headerbar (which is the same header bar but more compact)

@mikeller mikeller merged commit 90e198d into betaflight:master Apr 28, 2020
@mikeller mikeller deleted the fix_link_opening branch April 28, 2020 12:18
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.

Hiperlinks are broken
5 participants