search

ryanb / letter_opener

Preview mail in the browser instead of sending.
MIT License
3.73k stars 237 forks source link

Allow configuration of the 'file///' part in Launchy.open path #185

Closed Kulgar closed 2 years ago

Kulgar commented 3 years ago

Hello,

I would like to be able to modify the 'file:///' part in the Launchy.open path. Indeed, I'm actually using WSL 2 and letter opener - as is - doesn't work because the absolute path is /home/... but WSL 2 is a VM. Its files are accessible on the "local network" of the computer using //wsl$/VM-distrib.

I just need to replace 'file:///' with 'file://///wsl$/Ubuntu-18.04'

As I think WSL 2 may not be the only environment where someone can have that problem, I was thinking I could do a little PR to be able to modify that "file:///" directly from Letter opener configurations.

But... I don't know if you are interested in that PR. And if you do, where should I put the unit tests to test that new configuration actually works?

Or would you think of any better way to make letter opener works within that specific environment?

Thanks a lot for your guidance!

Kulgar commented 3 years ago

I just saw that my PR is actually adressing the same problem as https://github.com/ryanb/letter_opener/pull/183 but in a different and more flexible way. :-)

Kulgar commented 3 years ago

@ryanb @nashby : do you need help maintaining the gem? I'd gladly join the maintainers if possible.

Kulgar commented 3 years ago

Anyone?! 🤔

nashby commented 3 years ago

@Kulgar hey, sorry for late response! Could you please add a spec for that? Otherwise looks good. I think it's the only solution we can have to make it work for WSL right now without complicated WSL detections and stuff

Kulgar commented 3 years ago

Sorry I still haven't found time to create the specs... I didn't forget this pull request though ^^'

harmolipi commented 3 years ago

This would resolve the issue with letter_opener not working on ChromeOS too, where Linux files are likewise stored in a VM. Launchy.open(Rails.root) works and pulls in the proper path to the directory within the VM, but Launchy.open("file:///#{Rails.root}") does not - so it would be great to be able to remove that prefix.

Kulgar commented 3 years ago

Yeah, they asked me to do some specs, but I really don't have time to do them right now. So... if anyone has that time, please, fork my branch and add them :-) I'll be happy to add them to the PR.

harmolipi commented 3 years ago

Sounds great, I'll take a swing at adding some then.

yuricampolongo commented 2 years ago

Waiting for this PR to be merged.

nashby commented 2 years ago

@Kulgar thanks! I merged it as it is for now. Will add specs a bit later

Kulgar commented 2 years ago

@nashby sure thing, sorry I couldn't take the time to do the specs :-/

harmolipi commented 2 years ago

Glad to see this merged in! @Kulgar I submitted a PR to your fork with some specs a little while back - they might at least help jumpstart that process, if they're any good.

Kulgar commented 2 years ago

@harmolipi you did? I didn't receive any notifications about that :-(

Kulgar commented 2 years ago

I created a new PR with your specs @harmolipi - thanks a lot! https://github.com/ryanb/letter_opener/pull/193

It seems that some tests are failing for recent rubies, I'll have a look

harmolipi commented 2 years ago

Sounds good @Kulgar, thanks! Let me know if you want me to take a look at any of that too.

Kulgar commented 2 years ago

@harmolipi : I found the bug, it was a random bug due to specs running in a random order. You can have a look if you are curious: https://github.com/ryanb/letter_opener/pull/193

Kulgar commented 2 years ago

@nashby : So, you can have a look to the new PR which is now ready to be merged and adding some specs for this new feature.

Thanks!

harmolipi commented 2 years ago

@Kulgar interesting, checking it out now - thanks for figuring that out!