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 ability to open files from Finder in macOS #2395

Merged
merged 17 commits into from
Apr 6, 2024

Conversation

polachok
Copy link
Contributor

@polachok polachok commented Mar 3, 2024

What kind of change does this PR introduce?

  • Feature

Did this PR introduce a breaking change?

Kinda, it may change fork behaviour in some cases.

This is basically #2191 + 9d853ea, which allows to:

  1. drag file to dock icon and start neovide with the file open
  2. open files using finder context menu ("Open with -> Neovide")

Reasoning for the fork change: #2191 (comment)

Related issues:
#1682
#1259
#644

@9mm
Copy link
Contributor

9mm commented Mar 3, 2024

awesome! I will check it out

@polachok polachok marked this pull request as ready for review March 9, 2024 12:17
@polachok
Copy link
Contributor Author

polachok commented Mar 9, 2024

@9mm @falcucci did you have a chance to look at this

@9mm
Copy link
Contributor

9mm commented Mar 9, 2024

Hey @polachok I tried it briefly but had a business trip that took up most of my last week. I'll try it this weekend, or even maybe right now before I go see Dune 2

@9mm
Copy link
Contributor

9mm commented Mar 9, 2024

Dude it works!!! I didnt test all edge cases yet but Fk thats cool!!! It's so amazing to see Neovide doing that.

I will make a coffee and then read your gotchas more in detail, with different scenarios etc.

@9mm
Copy link
Contributor

9mm commented Mar 9, 2024

  • single file drag works
  • multi-file drag works
  • app open, dragging 1 file in works
  • right click > open with works

This was listed as something that doesnt work: "it doesn't work when neovide isn't already open (not sure how to fix this)"

But for me this looks like it works

@polachok
Copy link
Contributor Author

polachok commented Mar 9, 2024

Yes, it works because this branch contains “no tty = no fork” change, see this comment
#2191 (comment)

@9mm
Copy link
Contributor

9mm commented Mar 9, 2024

Got it, well I will wait for @fredizzimo to weigh in on the best approach. but overall this works awesome.

@falcucci
Copy link
Member

falcucci commented Mar 9, 2024

I'll look into it as soon as I can.

probably after #2405 gets refined

@9mm
Copy link
Contributor

9mm commented Mar 9, 2024

Also I defer to @falcucci, @fredizzimo and @polachok on how best to integrate my code which referenced in this comment:

#2191 (comment)

@MultisampledNight
Copy link
Contributor

(not familiar with macOS internals and I could not test either, but code looks good to me)

@alsa64
Copy link

alsa64 commented Mar 25, 2024

Tested it locally on an arm mac mini and it opening files works fine, there are however two minor issues:

  1. The app doesn't advertise itself as being able to open text documents, so assigning it as the default app for a format is quite cumbersome, the user needs to highlight a file, right click it, press CMD+i, then under open with in the dropdown select other, then in another dropdown select all programs to be able to select a tool that doesn't advertise document opening support, then hit change all.

potentially relevant links:

  1. The App doesn't have icons for filetypes. vscode and other Editors tend to ship with a generic branded text document icon and a few for various common file extensions, like .c, .cpp, ...

Links:

@fredizzimo
Copy link
Member

@9mm worked on that here

The TLDR; is that cargo-bundler which we use, does not support that, and the status of the project is:

Very early alpha. Expect the format of the package.metadata.bundle section to change, and there is no guarantee of stability.

Additionally, the maintainer said that he doesn't have time to work on the project, but I just checked again, and there has been some activity recently, and it also looks like the maintainer has changed some time ago already, so there might be some hope of getting the required PRs merged if we do it ourselves.

The other alternative, which @9mm was investigating was to do it completely manually, and that actually seemed to wrok.

@falcucci
Copy link
Member

I think now we can proceed reviewing this implementation.

@polachok could you pull it from upstream and fix conflicts?

Copy link
Member

Choose a reason for hiding this comment

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

Can we store this as svg, or some other format that is easier to edit?

@9mm
Copy link
Contributor

9mm commented Apr 4, 2024

Ok, I will remake it with illustrator, and export as svg

@fredizzimo
Copy link
Member

Thanks, Gimp for example can open the PSD files, but for example the text can't be edited, so a more supported format is better if other people need to modify it later.

@9mm
Copy link
Contributor

9mm commented Apr 4, 2024

@fredizzimo can you try one of these?

You will need the font from here: https://github.com/ryanoasis/nerd-fonts/releases/download/v3.2.0/JetBrainsMono.zip

I made .ai / .eps. ai is for illustrator specifically but i converted everything to vectors so maybe it will edit fonts now (hopefully this works in gimp)

i also made eps which i think is another vector format that might work

Archive.zip

@fredizzimo
Copy link
Member

Inkscape can import both, but it can only save eps, and ignores the illustrator specific data, so I don't think .ai is a good option.

The best option might actually be pdf, since eps does not support transparency. And should be editable by a lot of programs https://www.graphicdesignforum.com/t/eps-vs-pdf/7424/4

But I'm not an expert on these, so, if someone else knows better, feel free to recommend.

@9mm
Copy link
Contributor

9mm commented Apr 5, 2024

Ok here is also a PDF. i will let you choose which seems best.

The only thing i have is illustrator so i cant check how it works with open source apps.

background.pdf

@9mm
Copy link
Contributor

9mm commented Apr 5, 2024

BTW i removed all raster content and everything is 100% vector, so no transparency shouldn't be an issue with the EPS if thats what you end up going with. PDF seems to work well too though

@fredizzimo
Copy link
Member

The PDF seem to work perfectly. All the paths and groups are showing and are editable. The eps had a lot of bitmap images.

@9mm
Copy link
Contributor

9mm commented Apr 5, 2024

nice! maybe @polachok can just replace my psd with that then? you could also include the zip of the font if you want to be specific. thanks guys

@polachok
Copy link
Contributor Author

polachok commented Apr 6, 2024

@9mm you can push it to your branch and I will rebase mine on top

@9mm
Copy link
Contributor

9mm commented Apr 6, 2024

Ok, I ...

  1. removed the psd
  2. added pdf
  3. added instructions on how to export the tiff and run make-icns

9mm@ed768b4

@fredizzimo fredizzimo merged commit fcf5e87 into neovide:main Apr 6, 2024
2 checks passed
@fredizzimo
Copy link
Member

Thanks, let's add CI support for this in another PR.

@9mm
Copy link
Contributor

9mm commented Apr 6, 2024

one thing is... someone please check my shell scripts, ie... i dont write many bash scripts and im using rm -R "${ICONSET_DIR}" near the bottom. want to make sure this is idiomatic bash

@fredizzimo
Copy link
Member

@9mm, good catch, recursive deletion of things stored in environment variables is generally a very dangerous thing to do. This happened just a couple of weeks ago https://www.bleepingcomputer.com/news/linux/kde-advises-extreme-caution-after-theme-wipes-linux-users-files/

And this has also happened https://hackaday.com/2024/01/20/how-a-steam-bug-once-deleted-all-of-someones-user-data/

In this case, I think it should be fine, you don't use force, and the variable is local to the script. But we can check it closer in the integration PR.

@polachok polachok deleted the open-files branch April 6, 2024 16:23
@9mm
Copy link
Contributor

9mm commented Apr 6, 2024

Btw not sure if this is possible but i noticed if you drag a file, maybe the working directory for vim should be set to that file? that seems to be what macvim does

However, i noticed a lot of changes ot neovide anyway though so ... i cant really pinpoint right now how this is related to the general binary getting passed a path... for example:

NEOVIDE_FORK=1 /Users/zesty/.cargo/bin/neovide

this i would expect to open neovide to the current path im in but i have to build my own launcher script which does -- -c "cd $path" otherwise it always opens ~ home folder

@fredizzimo
Copy link
Member

The working directory support should have been added by @falcucci here:

@9mm
Copy link
Contributor

9mm commented Apr 6, 2024

hmm interesting... ok thanks

@fredizzimo
Copy link
Member

@9mm, I also noticed that it does not work as it should, for me it always sets the working directory to home. I will check what's going on a bit later today.

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.

6 participants