search

will-stone / browserosaurus

🦖 The browser prompter for macOS
https://browserosaurus.com
GNU General Public License v3.0
1.69k stars 160 forks source link

refactor: use app names for detection #593

Closed will-stone closed 1 year ago

will-stone commented 1 year ago

This should be much faster but currently relies on people not renaming apps. Will possibly look into users providing their own config...

will-stone commented 1 year ago

@mreid-tt Here's a first look at app-name based detection if you fancy taking a look?

mreid-tt commented 1 year ago

Hey @will-stone, thanks for sharing. I've taken and look and tried to run for myself (had to fix a few minor linter issues). The app seemed to run nicely with npm start however I was noticing that my Zoom app doesn't show up. I then checked your new app list and noticed this:

https://github.com/will-stone/browserosaurus/blob/313b66b954fb3ff10f83536acf609b2e8683e5ac/src/config/apps.ts#L127

This would be incorrect since the actual app name is 'zoom.us.app'. I then added 'zoom.us' to the list for myself and Zoom showed up in my detected app list on relaunch but without its proper name and icon...

Screenshot 2022-12-04 at 7 19 35 AM

Digging deeper into the logs streaming in Terminal I noted this line:

Error reading zoom.us
06:56:17.498 › Error: Command failed: /Development/browserosaurus-test/node_modules/file-icon/file-icon [{"appOrPID":"zoom.us","size":64}]
Couldn't find: zoom.us

    at ChildProcess.exithandler (node:child_process:407:12)
    at ChildProcess.emit (node:events:527:28)
    at maybeClose (node:internal/child_process:1092:16)
    at ChildProcess._handle.onexit (node:internal/child_process:302:5)

I then went to the source repository at https://github.com/sindresorhus/file-icon/ and noted the following line in the code:

input.contains(".") ?

Based on this, it seems to be evaluating the input as a 'BundleIdentifier' if it contains a period. Unfortunately the actual name for Zoom from a fresh install is 'zoom.us' so this would not work for most users. I don't know if there would be a way to fix this with additional arguments to force the input to only be treated as app names? (don't know Swift code that well).

EDIT: Perhaps another way would be to have some sort of alternate name for 'zoom.us' named 'Zoom' which would be used for the icon search.

will-stone commented 1 year ago

Ooo, interesting. This is definitely a bug report for Sindre's tool.

mreid-tt commented 1 year ago

Ooo, interesting. This is definitely a bug report for Sindre's tool.

Have you tried out the ChatGPT from OpenAI which came out recently? I fed it the code for main.swift and asked it about the function and it even came up with a proposal to fix it! See below:

Question 1:

Screenshot 2022-12-04 at 10 21 40 AM

etc...

Screenshot 2022-12-04 at 10 21 52 AM

Question 2:

Screenshot 2022-12-04 at 10 22 14 AM Screenshot 2022-12-04 at 10 30 16 AM

Amazing isn't it?

will-stone commented 1 year ago

Crikey. I think Sindre’s been tweeting about that app so he may find it amusing it can write his code for him 😊

mreid-tt commented 1 year ago

I've done a bit more refining to simplify the change and submitted a PR for him. We may get a fix soon for your app 😊

will-stone commented 1 year ago

Awesome! Thanks 🙂

mreid-tt commented 1 year ago

Awesome! Thanks 🙂

This new release should have the changes included: https://github.com/sindresorhus/file-icon/releases/tag/v5.1.1

will-stone commented 1 year ago

Nice! Thanks @mreid-tt Appreciate all your help there 😊 I'll aim to look at it this weekend.

mreid-tt commented 1 year ago

seems to be working!

Screenshot 2022-12-09 at 1 39 03 PM
will-stone commented 1 year ago

Thank you @mreid-tt for your help. I've merged in your changes and rebased. Now I just need to check the names for all the apps. You don't happen to know a quick way to do that do you, other than installing them all? 😬 My plan was to use brew names as a guide, still involves going through them one-by-one though.

will-stone commented 1 year ago

The brew-lookup technique doesn't work for dev/canary build names 😞

will-stone commented 1 year ago

To be honest, they all look right. I think I'll just release it with the caveat that some may be wrong and people need to raise issues/PRs; this release is already going to wipe-out app config (no way to convert ids to names without keeping them in the code base). Okay, I'll have a final shake-down...

mreid-tt commented 1 year ago

You're welcome! I'm glad that this is now published and I have been able to resolve #572 since initial testing shows all my browsers showing up within a minute from boot. I think you made the right call on the app list, the community should be able to help resolve any naming issues for missing apps.

One thing you may need to consider is updating your documentation. As we have moved from Spotlight searching, neither apps in different locations nor apps which have been manually renamed will be picked up. One discussion like this one: https://github.com/will-stone/browserosaurus/discussions/479 could be updated to say that it will only pick up apps in the /Applications folder from v20.

will-stone commented 1 year ago

Ah yes! I always forget the docs 😖 I’ll add that to my todo list. Thanks 😊