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

fixes installing fire nvim on brave #778

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

polarmutex
Copy link
Contributor

@polarmutex polarmutex commented Nov 11, 2020

fixes #773

confirmed linux and mac paths (do not have a machine to test windows)

@glacambre
Copy link
Owner

Thanks for taking the time to fix this! I'm curious, did you find these paths somewhere in Brave's documentation? I'm wondering because as said in #773 (comment) , brave has worked for some people before, so either the paths changed or they're only wrong on some OSes and not others.

@polarmutex
Copy link
Contributor Author

It was using the chrome data directory to write the manifest for the brave browser. this should have never worked unless you copied the file manually like Prime and I did

I could not find official documentation, just posts stating these paths

@glacambre
Copy link
Owner

According to brave contributors, using Chrome's directory is the correct thing to do (at least on OSX?). Since #144 used Firenvim on Brave on OSX without reporting any issues I'd prefer it if your PR only changed the path for Linux. Do you want me to perform this change or would you rather do it yourself?

@glacambre
Copy link
Owner

Ugh, the references to the native manifest I found in Brave's source code indicate that Chrome's directory should also be used on linux:

https://github.com/brave/brave-core/blob/bbbf3b2e65b440a732f4199e598461a65775ff8f/app/brave_main_delegate.cc#L134-L161

But this is obviously wrong since it didn't work for you or ThePrimeagen… I'm wondering, how did you install Brave? Through your distro's package manager?

@polarmutex
Copy link
Contributor Author

i installed through the brave download page (using Ubuntu 20.04)

I can do some more tracing tomorrow to see what is going on, something must have changed since the original code

@glacambre
Copy link
Owner

glacambre commented Nov 11, 2020

Ok, so on OSX it's chrome's directory but on Linux it's regular XDG: https://github.com/brave/brave-core/blob/6a02303906a4a5d7c3d57f4cef29204593f9e40b/chromium_src/chrome/common/chrome_paths_linux.cc#L16-L28 . Please edit your commit to make sure it only changes paths for Linux and I'll be happy to merge it :)

@glacambre glacambre merged commit 076c1a4 into glacambre:master Nov 11, 2020
@glacambre
Copy link
Owner

Again, thank you for taking the time to fix this! :)

@glacambre glacambre added this to the next milestone Nov 11, 2020
@tristan957
Copy link
Contributor

tristan957 commented Nov 12, 2020

I don't think the solution is totally right for Linux.

If you look at the chromium source code linked above it is checking for the existence of the env var XDG_CONFIG_HOME which isn't necessarily ~/.config. If XDG_CONFIG_HOME isn't set, I would bet the function just defaults to ~/.config which is following the XDG spec to a tee.

XDG spec says if XDG_CONFIG_HOME isn't set default to ~/.config.

I am no viml god, but what you need to do is something like

if $XDG_CONFIG_HOME set:
  build_path([$XDG_CONFIG_HOME, 'BraveSoftware', ...])
else
  build_path[WHAT YOU HAVE CURRENTLY])

@glacambre
Copy link
Owner

Thanks for pointing this out. I assumed that brave was "lying" about the path actually following XDG and that it'd only return the default path, as Chrome's documentation explictly states that ~/.config should be used.
But I dug a little deeper and it looks like GetXDGDirectory does lookup XDG variables.
@tristan957 your code suggestion looks correct to me - do you want to submit a PR?

@tristan957
Copy link
Contributor

Ha. Yeah I'll see if I can figure out some viml.

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.

Making Brave Work
3 participants