firenvim icon indicating copy to clipboard operation
firenvim copied to clipboard

Handle firefox flatpak installation

Open elken opened this issue 1 year ago • 5 comments

Summary

This should take care of a number of issues w.r.t installing this within a flatpak sandboxed browser.

At present, this is only tested against firefox but there's little reason it shouldn't work the same for other browsers.

There are also a few more paths that need to be overridden in that flatpak, namely:

  • stdpath("config")
  • stdpath("data")
  • ~/.local/share/firenvim
  • The path to nvim if it's installed via something like homebrew
  • /run/user/1000/firenvim

Which will be included in the documentation, as well as added to troubleshooting steps.

What this PR changes

If a browser configuration has a key denoting its flatpak name, then we also check if there is a flatpak installed. If not, we move on. If there is no flatpak installed, we also move on.

The interesting thing we now do here is creating symbolic links from your config inside the flatpak's config folder and your data path inside the flatpak's data folder. Doing that is the only check I needed to get this working (as I write this issue now inside firenvim)

image image

What's here has been tested locally by me insofar as I can, but I would be much more comfortable if others tested this as well. Others testing this might also demonstrate how simply this can be applied to other browsers like Chrome; which I don't use but I can spend some time testing those if you'd prefer every browser to be in this PR.

If it's not also evident from the code, I haven't touched VimScript in nearly a decade ... so feel free to correct my code as you see fit.

I know there's also no documentation; to save me constantly updating it I'll wait until you're happy with this PR & add some in then. I think there's a few more troubleshooting steps I can add in too.

Thanks for a fantastic tool!

elken avatar Mar 26 '24 15:03 elken

Hi, thanks a lot for taking the time to add Flatpak support to Firenvim! Really appreciated :). I think your pull request should work as is, but I wonder if it might be better to treat a firefox flatpak installation as a separate browser, i.e. instead of modifying the definition of the existing "firefox" installation, we would have:

diff --git a/autoload/firenvim.vim b/autoload/firenvim.vim
index 7311c50..fe9a9e3 100644
--- a/autoload/firenvim.vim
+++ b/autoload/firenvim.vim
@@ -770,7 +770,12 @@ function! s:get_browser_configuration() abort
                         \ 'manifest_content': function('s:get_firefox_manifest'),
                         \ 'manifest_dir_path': function('s:get_firefox_manifest_dir_path'),
                         \ 'registry_key': 'HKCU:\Software\Mozilla\NativeMessagingHosts\firenvim',
-                        \ 'flatpak_name': 'org.mozilla.firefox',
+                \},
+                \'firefox_flatpak': {
+                        \ 'has_config': s:firefox_flatpak_config_exists(),
+                        \ 'manifest_content': function('s:get_firefox_manifest'),
+                        \ 'manifest_dir_path': function('s:get_firefox_flatpak_manifest_dir_path'),
+                        \ 'registry_key': 'HKCU:\Software\Mozilla\NativeMessagingHosts\firenvim',
                 \},
                 \'librewolf': {
                         \ 'has_config': s:librewolf_config_exists(),

The reason I'm thinking this might be better is that as far as I can tell, a "regular" firefox installation and a flatpak one can coexist on a same computer. I'm also under the impression that it might make implementing flatpak-specific behavior easier.

What do you think about this? If you think this might indeed be better, do you want to work on it or would you prefer that I do it? :)

glacambre avatar Mar 28 '24 06:03 glacambre

So the reason I went with the approach I did was it makes it easier to later add support for flatpaks for the other browsers without having to add a new browser type for each one (also snap, etc) but I don't have any inherent problem with it being implemented that way :smile:

It sounds like you're otherwise happy with the rest, so I'll make that change & add some documentation/troubleshooting info and then this should be good to go if you're happy

elken avatar Mar 28 '24 06:03 elken

Apologies for not getting to this, I've had a pretty busy week and I've got another one but I've planned some time at the weekend to look at getting this done :smile:

elken avatar Apr 01 '24 08:04 elken

No problem! I can get quite busy too at times, take all the time you need :)

glacambre avatar Apr 01 '24 09:04 glacambre

I think that looks fine docs wise, is there anything else you think I should add? :smile:

elken avatar Apr 04 '24 13:04 elken