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

[DO NOT MERGE] This is what we rsync into the Apple Cordova project #1517

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fortuna
Copy link
Collaborator

@fortuna fortuna commented Dec 15, 2022

This PR is for documentation purposes. It's not supposed to be merged at any time.

@fortuna fortuna requested review from a team as code owners December 15, 2022 21:32
@fortuna fortuna marked this pull request as draft December 15, 2022 21:32
@daniellacosse
Copy link
Contributor

You mind adding a description of what you're attempting to achieve here?

@fortuna fortuna changed the base branch from master to fortuna-hooks December 15, 2022 21:44
This commit shows the edits we make to the standard Cordova project.
We obtain the diff by reversing the rsync call.
@fortuna fortuna force-pushed the fortuna-cordova-diff branch from a9318f5 to cbb283f Compare December 15, 2022 22:18
@fortuna fortuna changed the base branch from fortuna-hooks to master December 15, 2022 22:19
@fortuna fortuna changed the base branch from master to fortuna-config December 15, 2022 22:19
@fortuna fortuna changed the base branch from fortuna-config to master December 15, 2022 22:19
<dict/>
<key>CFBundleIcons~ipad</key>
<dict/>
<string>$(EXECUTABLE_NAME)</string>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eacrh What's the difference between ${} and $()?

Copy link
Contributor

@elenadoty elenadoty Dec 16, 2022

Choose a reason for hiding this comment

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

They're both valid, I believe ${} is a legacy way of referencing that Apple moved away from in favor of $()

@@ -2,70 +2,53 @@
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The "old" file is what we currently use. The "new" file is what Cordova generates before our edits.

andSelector:@selector(handleURLEvent:withReplyEvent:)
forEventClass:kInternetEventClass
andEventID:kAEGetURL];
// Don't ever show the default Cordova window, as we will display its content view in a popover.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should show the app window. Users have complained that they run the app and "nothing happens". The tray icon is not super visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, this would be in addition to the tray?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know, maybe we need UX input.
Perhaps a simpler approach would be just to open the popup window on start.
i'm curious about what other VPN apps do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think they do just that, show the open window under the tray icon on first load so it grabs the user's attention.

#pragma mark - Launcher

// Enables or disables the embedded app launcher as a login item.
- (void)setAppLauncherEnabled:(bool)enabled {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have logic to restart on login if the user was connected to the VPN.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this leads to two options for multi-server config:

  1. still connect to the previous connected IP:port
  2. rerun the entire selection process and might connect to a new server

[self.eventMonitor stop];
}

- (void)showPopover {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We added logic to show the system tray.

@@ -288,49 +313,6 @@
</items>
</menu>
</menuItem>
<menuItem title="Window" id="Wrb-DK-0VT">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why we need these menu items, since we don't have a menu bar.

<feature name="LocalStorage">
<param name="osx-package" value="CDVLocalStorage" />
</feature>
<feature name="Device">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need this file because there's a bug where cordova-osx fails to restore the plugins: apache/cordova-osx#106

@@ -4,54 +4,8 @@
"uninstalled": []
},
"config_munge": {
"files": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -8,6 +8,14 @@ cordova.define('cordova/plugin_list', function(require, exports, module) {
"cordova.plugins.clipboard"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why we rsync this file.
TODO: remove it from our code.

// execSync(`rsync -avc src/cordova/apple/xcode/${outlinePlatform}/ platforms/${platform}/`, {
// stdio: 'inherit',
// });
// Hack to find out what we edit in the standard platform that is created.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the trick to generate the diff.

@fortuna fortuna removed request for a team December 16, 2022 00:05
@daniellacosse daniellacosse changed the title [DRAFT] Cordova edits [DO NOT MERGE] Cordova Exploration & Annotations Dec 16, 2022
@daniellacosse daniellacosse added the do not merge Do not merge the pull request for now. label Dec 16, 2022
@daniellacosse daniellacosse changed the title [DO NOT MERGE] Cordova Exploration & Annotations [DO NOT MERGE] This is what we rsync into the Apple Cordova project Dec 16, 2022
@jyyi1 jyyi1 added the question label Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge the pull request for now. question size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants