sfsam / Itsycal

Itsycal is a tiny calendar for your Mac's menu bar. http://www.mowglii.com/itsycal
MIT License
3.32k stars 234 forks source link

Open zoom app directly #176

Closed pakerwreah closed 3 years ago

pakerwreah commented 3 years ago

Closes #175

sfsam commented 3 years ago

Wow, you are on a roll! I'm not very familiar with how Zoom works so I have some questions:

  1. After using NSWorkspace to detect the Zoom App, is there a reason you don't just use string substitution to create the app URL? Something like this:

    NSString *zoomAppURLString = [url.absoluteString stringByReplacingOccurrencesOfString:@"?" withString:@"&"];
    zoomAppURLString = [zoomAppURLString stringByReplacingOccurrencesOfString:@"/j/" withString:@"/join?confno="];
    zoomAppURLString = [zoomAppURLString stringByReplacingOccurrencesOfString:@"https://" withString:@"zoommtg://"];
    return [NSURL URLWithString:zoomAppURLString];
  2. It looks like your code requires a pwd parameter in the original URL to turn it into an app URL. Is there a reason for this? For example, the string replacement code above would turn https://us02web.zoom.us/j/89621335927 into zoommtg://us02web.zoom.us/join?confno=89621335927, but your code would not transform it because it doesn't have a pwd.

  3. It looks like your code removes sub domains. Is that required for an app link? For example, your code turns https://us04web.zoom.us/j/73968781465?pwd=VzhCZGVHZG0xQnBpY1YwTUVnU0JMdz09 into zoommtg://zoom.us/join?confno=73968781465&pwd=VzhCZGVHZG0xQnBpY1YwTUVnU0JMdz09 whereas the string method would retain the us04web subdomain in the app link.

  4. I'm wondering where is the right place to do this transformation. Is it in EventCenter when the video links are detected (as you have done), or is it in AgendaViewController when the action is assigned to the video button?

pakerwreah commented 3 years ago

1) No particular reason.

2) Nope, I was just excited that it worked and forgot to cover that case 😅

3) Not required, I've tested. (my meetings all have subdomains)

4) Looking from a MVVM point of view, this should not be done in the controller.

// TODO: I'll change it to the more straightforward way, just replacing the strings and we cover 1, 2 and 3.

sfsam commented 3 years ago

Thanks. I'm thinking it can be refactored a bit to eliminate the zoomDirectLinkFromURL block. It can all be done in GetZoomURL since it's just a simple if clause. The NSWorkspace if clause can go after info.zoomURL = result.URL in GetZoomURL's test for zoom.us/j/. I think you can use the link local var that is already there. Something like:

...
NSString *link = result.URL.absoluteString;
if ([link containsString:@"zoom.us/j/"]) {
    info.zoomURL = result.URL;
    // Test if user has the Zoom app and, if so, create a Zoom app link.
    if ([NSWorkspace.sharedWorkspace URLForApplicationToOpenURL:[NSURL URLWithString:@"zoommtg://"]]) {
        link = [link stringByReplacingOccurrencesOfString:@"https://" withString:@"zoommtg://"];
        link = [link stringByReplacingOccurrencesOfString:@"?" withString:@"&"];
        link = [link stringByReplacingOccurrencesOfString:@"/j/" withString:@"/join?confno="];
        NSURL *appLink = [NSURL URLWithString:link];
        if (appLink) info.zoomURL = appLink;
    }
}
...

Does that make sense?