mikehardy / thunderlink

Link to your Thunderbird emails!
Mozilla Public License 2.0
42 stars 14 forks source link

Support for mac #26

Closed MarioKusek closed 5 years ago

MarioKusek commented 5 years ago

Hi,

In this pull request is support for using thunder link on Mac OSX.

It works as following. I have created scripts ThunderlinkOpener.scpt and ThunderlinkOpenerJS.scpt which can be saved as ThunderlinkOpener.app (there is also ready app file). In integration/macosx/install.txt are instructions how to install it, or update it if it is needed. Basically you copy script in Applications folder and give right to script to control computer (send keys to other applications). This script in Info.plist has registration that runs it when thunderlink URL is clicked. Then script copies URL in clipboard, opens thunderbird, finds window and send combination of keys to thunderbird (CMD+ALT-o). In the thunderlink plugin I have registered those keys to call function that reads data from clipboard, extracts messageid and opens message.

There is one small feature for generating text from templates. You can now also put

mikehardy commented 5 years ago

This looks fantastic thank you! I'm traveling right now but will look at this in detail as soon as I can

mikehardy commented 5 years ago

I had a minute and was able to give this a medium-level look just now but have not tested it yet. Can you tell me which macOS + Thunderbird versions you have checked? Thunderlink 1.2.6 covers TB38 through TB64 now, so I want to make sure I test whatever versions you haven't

The PR is more involved than I thought but it makes sense - in order to work around thunderbird not passing command line arguments on macOS (until TB64 anyway), you're using the applescript to operate thunderbird via the clipboard and a new command-line accelerator, right? If I am misunderstanding that please let me know - but I think that's what it is doing, and I think that's a neat way around the problem.

Other questions - could you put the date template thing in a separate commit? That's an easy merge and is unrelated. I'm not sure how good your git skills are with soft-resets and force pushes and stuff (I just learned this year...). If it were me I'd do a git reset --soft HEAD~1 on your dev branch, then a git add -p on preferences.xul and thunderlink.js and use that to pick out just the date template change

Separately but more importantly - it looks like there is a lot of code duplication with existing code. Most of the code in OpenMessage looks like it is copy-pasted from somewhere else - I really don't want code duplication so where ever this code came from should be abstracted (maybe with another parameter if you need one) and re-used. That way when I fix #11 for instance, I won't have to look for folder opening style in multiple places. The same could be said for the GetPref method - I think we're getting prefs in other areas, this code should just be in one spot and re-used everywhere if possible?

I am excited to merge this and will do so as soon as we make sure we're not duplicating code, and that it works for all the versions it can

Thanks!

MarioKusek commented 5 years ago

I have to admit that I had similar change in thunderbird plugin for 3 years but I did not have time to clean it up a little bit and create PR. I am not skilled in creating extensions for TB :(

I had a minute and was able to give this a medium-level look just now but have not tested it yet. Can you tell me which macOS + Thunderbird versions you have checked? Thunderlink 1.2.6 covers TB38 through TB64 now, so I want to make sure I test whatever versions you haven't

Regarding TB version, I have used 60.4.0. I did not test on older versions.

Regarding MacOS I have this script from 2015 so I have started to use it in 10.11 El Capitan and now I have 10.13.6 High Sierra. I have not test it in 10.14 Mojave.

The PR is more involved than I thought but it makes sense - in order to work around thunderbird not passing command line arguments on macOS (until TB64 anyway), you're using the applescript to operate thunderbird via the clipboard and a new command-line accelerator, right? If I am misunderstanding that please let me know - but I think that's what it is doing, and I think that's a neat way around the problem.

Yes, that is correct.

Other questions - could you put the date template thing in a separate commit?

Sure, I can do that.

Separately but more importantly - it looks like there is a lot of code duplication with existing code. Most of the code in OpenMessage looks like it is copy-pasted from somewhere else

Yes that is true, it is copied from thunderlinkCommandLineHandler.js. There are thunderlinkCommandLineHandler_handle and getPref functions. I am not sure how can I put them in one place and use them. :( And the other thing is that I did not want to refactor your code.

I really don't want code duplication so where ever this code came from should be abstracted (maybe with another parameter if you need one) and re-used.

I totally agree with you.

That way when I fix #11 for instance, I won't have to look for folder opening style in multiple places. The same could be said for the GetPref method - I think we're getting prefs in other areas, this code should just be in one spot and re-used everywhere if possible?

If you have suggestion how to organise code and how to include it more places I can do the work.

I am excited to merge this and will do so as soon as we make sure we're not duplicating code, and that it works for all the versions it can.

Excellent! I am excited too. Thanks for feedback.

mikehardy commented 5 years ago

I never mind a good refactoring if you want to give it a try. PRs are great for that as they're just proposals and if you've got the Git Power (tm) to force push etc it's easy to mush code around. Give it a shot to remove the duplication and as long as it's not using ECMAScript 2015 stuff (which would break Thunderbird 38) it'll probably be fine...

MarioKusek commented 5 years ago

I have extracted duplicated code in thunderlinkModule.js. Please look what I did.

The other thing that I noticed is that in thunderllink.js is function ConvertToUnicode. Why is that needed?

mikehardy commented 5 years ago

I have extracted duplicated code in thunderlinkModule.js. Please look what I did.

Great! Thank you and I'll look through it

The other thing that I noticed is that in thunderllink.js is function ConvertToUnicode. Why is that needed?

Using git blame, it looks like in 2014 someone requested (and made a PR for) the ability to use multi-byte strings in the title and body and it relied on this change - so I'd think it was essential (or at least is a regression if it is removed)? https://github.com/simonthum/thunderlink/commit/7eda9689807f7752f4ba1ce7e1fa7993c7ebcfab

MarioKusek commented 5 years ago

Using git blame, it looks like in 2014 someone requested (and made a PR for) the ability to use multi-byte strings in the title and body and it relied on this change - so I'd think it was essential (or at least is a regression if it is removed)? simonthum@7eda968

I was asking because this is creating problem for me (non ascii characters in subject or in from of an email). If there are already UTF characters in string this function change them to something else e.g. 'š' to 'e'. I think that the same call is also creating problem that is stated in code (thunderlink.js):

    // FIXME This can throw exceptions? NS_ERROR_ILLEGAL_INPUT if there are Spanish accents etc

What I did in my local copy is decode MIME string (this is already used for subject) and removed call to convert to unicode before putting in clipboard. This is working for subject and author with special characters in different MIME encodings which is what you get from headers.

I have this changes in local branch. If you want I can create PR or I can merge to master to be part of this PR.

mikehardy commented 5 years ago

I was asking because this is creating problem for me (non ascii characters in subject or in from of an email). If there are already UTF characters in string this function change them to something else e.g. 'š' to 'e'. I think that the same call is also creating problem that is stated in code (thunderlink.js):

I see what you mean

What I did in my local copy is decode MIME string (this is already used for subject) and removed call to convert to unicode before putting in clipboard. This is working for subject and author with special characters in different MIME encodings which is what you get from headers.

That seems reasonable, if I understand correctly the SMTP standard(s) uses MIME in all of those places so UTF doesn't have to be an issue?

I have this changes in local branch. If you want I can create PR or I can merge to master to be part of this PR.

I would definitely like to keep these things separate, so a separate issue with a separate PR that is based on the relevant standards (i.e. those listed here? https://en.wikipedia.org/wiki/Unicode_and_email#Unicode_support_in_message_header ) would be best

That way if we cause some sort of regression we won't have a hard time picking apart what happened

MarioKusek commented 5 years ago

OK, tnx

I will create new PR

mikehardy commented 5 years ago

If all your dev work was on topic branches this might be a bit easier. I just realized that the mime stuff was probably also on master? If my changes that I PRd to you work then I can merge them in from my topic branch on my fork followed by you rebasing your follow up stuff...

MarioKusek commented 5 years ago

If all your dev work was on topic branches this might be a bit easier. I just realized that the mime stuff was probably also on master?

No they are not.

If my changes that I PRd to you work then I can merge them in from my topic branch on my fork followed by you rebasing your follow up stuff...

I don't understand this :(

mikehardy commented 5 years ago

@MarioKusek no worries I think after I merge to master it will be obvious. I'm with my family now so I'll merge it all together later tonight (Ecuador time)

MarioKusek commented 5 years ago

It is not something of high priority. Enjoy with your family.

mikehardy commented 5 years ago

@MarioKusek - for some reason this isn't working for me, and I'm not sure how to debug it. I'm on macOS 10.14.2 and after putting the script .app into /Applications and adding it to Accessibility, I can click on links and they will either open Thunderbird or bring it to the foreground (if already open), but the message itself does not pop up either in a new tab or in a new window (I tried both of those methods).

Have you tried with a clean install of the scripts from the integration directory, and a clean 1.2.7 plugin version? Maybe there's an environment difference. Or maybe you're on a different mac version and they changed security stuff. I dunno?

I was excited to try it though.

Also for what it's worth, you might try just using the regular command-line integration with Thunderbird 64 and 65 - apparently the reason this couldn't work on the mac was because Mac thunderbird didn't pass command-line arguments correctly to running thunderbird so it was impossible. But they supposedly fixed that, so maybe the clipboard link-passing style isn't needed and we can use one style for all 3 platforms (windows linux mac) ?

mikehardy commented 5 years ago

it looks like it might be an encoding issue - = to %3D ??

the error message I see:

screen region 2019-01-07 at 12 10 49

the link I was using:

screen region 2019-01-07 at 12 12 18
MarioKusek commented 5 years ago

Regarding script, MAC OSX scans Application folder for new applications and their protocols. But sometimes that doesn't happen immediately. Sometimes I have to wait for some time (maybe a minute). I don't know how much time.

Regarding error message, what are steps to generate that error?

mikehardy commented 5 years ago

Regarding script, MAC OSX scans Application folder for new applications and their protocols. But sometimes that doesn't happen immediately. Sometimes I have to wait for some time (maybe a minute). I don't know how much time.

It's definitely connecting things, so I think the application / script is present and functioning

Regarding error message, what are steps to generate that error?

I right click on a message and generate "clickable subject" hyperlink (one of the default choices) - you can see the thunderlink that is created

Then I click that link from my todo list, which tries to open the given thunderlink. I see Thunderbird move to foreground, then I get the error message I captured, and it appears to me that there is some url-encoding of the thunderlink going on because the equals sign has mutated? I'm not sure though

MarioKusek commented 5 years ago

I just read carefully your first message. So if thunderbird is opened when you click the link that means that Mac has scanned script and started it. So can you please copy link in clipboard and press Cmd+Alt+o. If it opens link then the shortcut is working. This means that problem occurs in macOS 10.14.2. I know that Apple has lots of security things added in Mojave and that needs to be configured for this script.

MarioKusek commented 5 years ago

I right click on a message and generate "clickable subject" hyperlink (one of the default choices) - you can see the thunderlink that is created

I see that URL in todo list is OK.

Then I click that link from my todo list, which tries to open the given thunderlink. I see Thunderbird move to foreground, then I get the error message I captured, and it appears to me that there is some url-encoding of the thunderlink going on because the equals sign has mutated? I'm not sure though

= is URL encoded to %3D. Script is not doing that.

I have just tried to put link in MS Word and it works there. When I paste it to Safari then it is changed. So this is depending on application.

The way to handle this it to URL decode content of clipboard or to change regex in openThunderlink function or to just replace %3D with =.

MarioKusek commented 5 years ago

Regarding TB 64 and 65, are they stable? I am highly dependent on thunderbird and this plugin so I can not waste to much time on this if it is not working good.

MarioKusek commented 5 years ago

I have found that there is decodeURIComponent build in thunderbird. So fix is to call it in openThunderlink first and then the rest will be fine.

mikehardy commented 5 years ago

I played around with variations, and I see what you saw - it's Safari doing it (I have my todo list open in a Fluid.app Safari container), and when I manually urldecode it works, or when I manually copy the source of the link (but now what Safari presents as the clickable version after editing) it works.

There are '+' signs in some of links so we have to be careful but I think urldecode is the right call.

I worry about urldecoding in thunderlink when this seems like a Mac problem though - there are no reports of it failing on other platforms and I hate to change something that isn't broken.

Can you throw the URLdecode into the Applescript? https://developer.apple.com/library/archive/documentation/LanguagesUtilities/Conceptual/MacAutomationScriptingGuide/EncodeandDecodeText.html

As for TB64/65 - they seem to work okay to me? I haven't tested calendaring but as a mail client they seem okay. That said, I don't fear reverting. I maintain separate installs and separate profiles for each version (unpacked and moved to version-specific-directory names) in order to do testing, and I use this script to run them:

#!/bin/bash
echo "Attempting to run release $1"
if [ "$2" != "" ]; then
  echo "received second argument $2"
fi
export RELEASE_HOME=/home/mike/work/thunderlink/releases/thunderbird-$1
cd $RELEASE_HOME
export LD_LIBRARY_PATH=.
./thunderbird-bin -purgecaches -P TestUser-$1 $2 &
mikehardy commented 5 years ago

Thanks for being so responsive by the way! This isn't a stop-work issue for me, I have a setup that works great already, it was just something that I noticed - needs to be fixed but not super urgent :-)

MarioKusek commented 5 years ago

I have updated ThunderlinkOpener script. Now it is getting URL and decode it. I have tried in my machine and it is working when you paste link in Safari. Where do you want me to put it? Should I open new branch and put ti there and create PR or?

mikehardy commented 5 years ago

Thank you!! I'm actually looking forward to having thunderbird on my Mac as well (right now I'm doing the inter-operating thing with messageid URLs on both, but Linux is my main platform now, so best to move).

From a different project, I have grown quite used to everything being on topic branches with PRs - seems to keep things clean - so if you can open a PR I'd appreciate it, and I'll give it a test today

mikehardy commented 5 years ago

During the whole Mail.app + GSuite + OSX10.14.4 fracaso this was super handy!