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

add support for browser profiles #113

Merged
merged 3 commits into from
Aug 11, 2020

Conversation

bhedavivek
Copy link
Contributor

Allows selecting profile of target browser.
Adds support for Google Chrome, Brave Browser and Mozzila Firefox.

Apologies in advance for not following any Swift/Javascript conventions correctly. I don't don't have much experience in either.

You can specify which browser profile target by defining the profile property on the browser object in the config file.

Sample Config

module.exports = {
    defaultBrowser: {
        name: "com.brave.Browser",
        appType: "bundleId",
        profile: "Personal",
        openInBackground: false,
    },
    handlers: [
        {
            match: ({ sourceProcessPath }) =>
                sourceProcessPath && sourceProcessPath.startsWith("/Applications/Slack.app"),
            browser: {
                name: "com.brave.Browser",
                appType: "bundleId",
                profile: "Work",
                openInBackground: false,
            }
        },
    ]
};

@johnste
Copy link
Owner

johnste commented May 15, 2020

Hey,

Thanks for for working on this! There is another pull request adding support for Google Chrome profiles which has a lot of similarity to your approach, though it doesn't attempt to add profile support for Brave or Firefox at this point.

I haven't been able to test your branch yet, but unless I am mistaken you would have to refer Google Chrome profiles by the folder name instead of the actual profile name here (see this comment.

The JavaScript file Finicky/Finicky/finickyConfigAPI.js is generated from the TypeScript config-api project in the repo root so any changes made need to be added there. The profile field would need to be added to the TypeScript types as well as the schema validation.

The pull requests are both pretty similar and I think we can find a good solution that uses parts of both. There haven't been any updates in the last couple of weeks to the other one so I'm not sure it's still being worked on.

I can probably have a look this weekend at combining these efforts.

@bhedavivek
Copy link
Contributor Author

bhedavivek commented May 15, 2020

Hey,

Thanks for for working on this! There is another pull request adding support for Google Chrome profiles which has a lot of similarity to your approach, though it doesn't attempt to add profile support for Brave or Firefox at this point.

Yes, I've been keeping an eye on that for a while hoping it would get more traction, in the end got impatient and started experimenting for a fix myself. I think my solution provides a more generalized approach. It is inspired by the original PR. I should have probably linked to it or used it as my fork.

I haven't been able to test your branch yet, but unless I am mistaken you would have to refer Google Chrome profiles by the folder name instead of the actual profile name here (see this comment.

I think with all Chromium browsers that haven't messed with the default CLI options (cough Firefox) --profile-directory=<profile name> points to the profile directory. However when a user creates a new profile, the directory and the profile have the same name. The directory isn't renamed when the user renames the profile.

If the --profile-directory=<profile name> directory doesn't exist, the browser will just create a directory with that name and generate a profile. However names the profile with the default name 🤦

I personally being slightly annoyed by that, first created the directory with my intended names and then renamed my profiles. Keeping things consistent.

With Firefox, you can either provide the profile name with -P or provide the profile directory path with -profile.

I chose call the config property in Finicky profile since you were generally referring to the profile name (or at least what they were originally called)

The JavaScript file Finicky/Finicky/finickyConfigAPI.js is generated from the TypeScript config-api project in the repo root so any changes made need to be added there. The profile field would need to be added to the TypeScript types as well as the schema validation.

Thanks, for some reason the folder was hidden by XCode and AppCode IDEs, I failed to realize that's where those files were coming from. Naively just updated the JS and experimented locally. I'll update the TS scripts.

Do I just need to run npm run build to regenerate the JS files from the TS files?

The pull requests are both pretty similar and I think we can find a good solution that uses parts of both. There haven't been any updates in the last couple of weeks to the other one so I'm not sure it's still being worked on.

I can probably have a look this weekend at combining these efforts.

Thanks for creating this project, has been really useful for me and my personal workflows. Look forward to any suggestions you have to improving this PR.

@johnste
Copy link
Owner

johnste commented May 15, 2020 via email

@bhedavivek
Copy link
Contributor Author

Hi, sorry I've not been able to devote time to this PR for the last few weeks. I'll be updating it this week.

@bhedavivek bhedavivek force-pushed the browser-profile-support branch 2 times, most recently from 8749d0f to c0135c4 Compare June 15, 2020 04:48
@bhedavivek
Copy link
Contributor Author

Hi @johnste

I've added changes as you requested to the .ts files. I see you ended up removing the autogenerated file.

I am wondering if you require any additional changes/unit tests for this to be mergeable. Let me know, I should have more bandwidth available this week to address any required changes.

Thanks,
Vivek

@bhedavivek
Copy link
Contributor Author

bhedavivek commented Jun 15, 2020

I am having trouble running this locally now after the changes made to add coding signing as the development team and ENABLE_HARDENED_RUNTIME. How should I go ahead and test?

Figured my way around Xcode. Still seems to work for me. Let me know if this needs any additional changes.

@johnste
Copy link
Owner

johnste commented Jun 15, 2020

Thanks, I'll check this out.

Did you have to disable Hardened runtime to be able to test? I'm sorry for making it harder but I needed to enable it to be able to noterize the app.

@bhedavivek
Copy link
Contributor Author

I don't think it was the hardened runtime itself, I was just having a hard time building because of the code signing. Since I wasn't using Xcode it was difficult for me to understand how to turn it off.

I eventually gave in and opened Xcode and found the option to build for local testing. (I don't have experience working with Apple app development)

Now that code signing is part of the default build configuration probably should update the command in the README/Wiki to build locally?

xcodebuild clean build CODE_SIGN_IDENTITY="" CODE_SIGNING_REQUIRED=NO

@johnste
Copy link
Owner

johnste commented Jun 15, 2020 via email

@johnste
Copy link
Owner

johnste commented Jun 23, 2020

Hey I had some time to test things out a bit. I didn't have much success so far. I was only able to test Firefox tonight.

I added a handler like so:

    {
      match: /example3/,
      browser: {
        name: "Firefox",
        profile: "simple",          
      },
    },

I ran open http://example.com/example3 in iTerm to test the handler out. So far, all that happens when running command just opens up a new window with the default profile selected and no url set. So that's weird.

I added a debug output to show the command Finicky generated which is
open -a /Applications/Firefox.app -g -n --args -P "simple" http://example.com/example3. Running that command in iTerm seems to work just fine. So I'm not sure what's going on here.

I tried commenting out task.waitUntilExit() like was tested in the other PR, but it didn't seem to have any effect.

I will try to dig more when I have the time again. I could test the Chromium based and see if that works better for me.

@bhedavivek
Copy link
Contributor Author

I've been using a build using this branch for my Brave browser profiles. I think Chromium based browsers will function fine.

With respect to the open command behaving differently, I am assuming its because of its running in a subshell.

If we use native AppKit Classes that control the open command (see: https://developer.apple.com/documentation/appkit/nsworkspace/3172702-open) we would have better luck supporting Firefox I think. I could try and hack something together this weekend, but that would mean finicky would only be able to support macos 10.15+

@bhedavivek
Copy link
Contributor Author

bhedavivek commented Jul 14, 2020

Hi, sorry for the lack of updates on this. I found the issue with my changes.
In essence the strategy works fine as we noticed when running commands on the CLI through the terminal.

The issue was two parted:

  1. I have the a typo in my bundleId
    org.mozilla.firefox needs to be org.mozilla.Firefox (At some point in my local testing I updated this to the wrong version I don't know why). This caused finicky to always send a blank profile option when trying to launch Firefox.

  2. When I am returning the profileOption I needed to return a String array and not a string literal (kudos my lack of knowledge of swift). I am assuming swift escapes the args passed to process. This is what caused firefox to always open in the default profile
    So, this needs to be updated from :

private func getProfileOption(bundleId: String, profile: String) -> String {
    var profileOption: String {
        switch bundleId {
        case Browser.Firefox.rawValue: return "-P \(profile)"

to

private func getProfileOption(bundleId: String, profile: String) -> [String] {
    var profileOption: [String] {
        switch bundleId {
        case Browser.Firefox.rawValue: return ["-P", profile]

I tested this locally and should support firefox profiles correctly now.

@bhedavivek
Copy link
Contributor Author

I guess, just needed some fresh eyes.

@bhedavivek
Copy link
Contributor Author

Let me know once this is mergeable, I can go ahead and squash my commits.

@arefz
Copy link

arefz commented Jul 15, 2020

Thank you so much for working on this @bhedavivek 🥇

@moshevayner
Copy link

This is great! Looking forward to see this merged, it'll be super useful.
Thanks for working on this! 😄

@johnste
Copy link
Owner

johnste commented Jul 23, 2020

Hey, I haven't been able to get the time necessary to test this out yet, will do when my schedule opens up. If anyone else wants to help out, building it and trying it out would help a ton!

@superbrothers
Copy link

I tried this PR with the following settings and found that it works well.

module.exports = {
  defaultBrowser: "Google Chrome",
  handlers: [
    {
      match: (a) => {
        finicky.log(JSON.stringify(a, null, 2));
      },
      browser: "Google Chrome",
    },
    {
      match: ({ keys }) => {
        return keys.command;
      },
      browser: {
        name: "Google Chrome",
        appType: "appName",
        profile: "Default"
      }
    },
    {
      match: ({ keys }) => {
        return keys.shift;
      },
      browser: {
        name: "Google Chrome",
        appType: "appName",
        profile: "Profile 1"
      }
    }
  ]
};

@bhedavivek Thank you for a great job!

@arefz
Copy link

arefz commented Aug 4, 2020

Could we please have this merged? 🙏

@johnste
Copy link
Owner

johnste commented Aug 4, 2020

Will try to get to this soon. I haven't been able to test this yet and I don't want to merge without some thorough testing.

@bhedavivek
Copy link
Contributor Author

bhedavivek commented Aug 4, 2020

Been using this as my local build for the past 3 months with Brave Browser, incase that helps instill some confidence with support for Chromium based browser profiles.

@johnste
Copy link
Owner

johnste commented Aug 10, 2020

I have my development setup up and running again, should be able to test this during the next few days.

@johnste
Copy link
Owner

johnste commented Aug 11, 2020

After testing a bit this seems to work well with Chrome (and, like you said, assuming other Chromium based browsers).

I noted a couple of things:

  • If the profile doesn't exist, Chrome will create a new profile
  • If the profile name uses a space character, Chrome doesn't appear to be able to match it, and instead creates a new profile

I didn't have much success getting Firefox profiles to work, it appears to work intermittently.

Overall, I'm happy to merge this and start preparing a new release. I think until we have more ability to test Firefox profiles (or other browsers), it makes sense to only officially support Chromium profiles for now.

If you want to look into any other issues or prepare this pull request for merging, feel free to do that and I will merge when you're done. Thank you so much for you work @bhedavivek and sorry for me taking such a long time to get back to you.

@bhedavivek
Copy link
Contributor Author

After testing a bit this seems to work well with Chrome (and, like you said, assuming other Chromium based browsers).

I noted a couple of things:

  • If the profile doesn't exist, Chrome will create a new profile
  • If the profile name uses a space character, Chrome doesn't appear to be able to match it, and instead creates a new profile

I didn't have much success getting Firefox profiles to work, it appears to work intermittently.

Overall, I'm happy to merge this and start preparing a new release. I think until we have more ability to test Firefox profiles (or other browsers), it makes sense to only officially support Chromium profiles for now.

If you want to look into any other issues or prepare this pull request for merging, feel free to do that and I will merge when you're done. Thank you so much for you work @bhedavivek and sorry for me taking such a long time to get back to you.

Fixing the chromium profile names with spaces should be easy, I'll escape the profile name for the chromium browsers.

For disabling firefox support, would you like me to comment out the code or remove it completely?

@johnste
Copy link
Owner

johnste commented Aug 11, 2020 via email

Allows selecting profile of target browser.
Adds support for Google Chrome, Brave Browser and Mozzila Firefox.
bhedavivek added a commit to bhedavivek/finicky that referenced this pull request Aug 11, 2020
Disabling support for Firefox browsers till there is better testing
methodology and tooling for Firefox browsers. Performs unreliably when
targeting specific profiles for Firefox.

Github PR Comment:

johnste#113 (comment)
@bhedavivek
Copy link
Contributor Author

bhedavivek commented Aug 11, 2020

I updated the method to only modify the browser command for supported browsers in 9054810

Disabling support for Firefox browsers till there is better testing
methodology and tooling for Firefox browsers. Performs unreliably when
targeting specific profiles for Firefox.

Github PR Comment:

johnste#113 (comment)
Updates to `getProfileOption` method now only returns a valid profile
option string array for supported browsers, returns nil otherwise. This
will keep the browser command practically the same when trying to launch
with unsupported browsers.
@johnste johnste merged commit e47de82 into johnste:master Aug 11, 2020
johnste pushed a commit that referenced this pull request Aug 11, 2020
Disabling support for Firefox browsers till there is better testing
methodology and tooling for Firefox browsers. Performs unreliably when
targeting specific profiles for Firefox.

Github PR Comment:

#113 (comment)
@johnste
Copy link
Owner

johnste commented Aug 11, 2020

Thank you @bhedavivek for you great work!

@arefz
Copy link

arefz commented Aug 19, 2020

@johnste 👋 Will you consider pushing out a new release with this anytime soon? 🙏

@johnste
Copy link
Owner

johnste commented Aug 19, 2020

Yes, I'll try to get a release out later tonight.

Edit: More likely tomorrow. Writing release notes takes a bit of time.

@johnste
Copy link
Owner

johnste commented Aug 21, 2020

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