jlebon / textern

A Firefox add-on for editing text in your favourite external editor!
GNU General Public License v3.0
138 stars 21 forks source link

Use the built-in keyboard commands system #73

Open Roy-Orbison opened 3 years ago

Roy-Orbison commented 3 years ago

Using the commands manifest key simplifies registration and conflict resolution of shortcuts (the user automatically gets warnings about existing bindings). This also changes the default shortcut to one that's unused by Firefox itself. It does require an extra permission to find the current tab.

jlebon commented 2 years ago

I'll have to test this, but being able to drop shortcut.js would be really nice. Have you observed any regressions using this?

Roy-Orbison commented 2 years ago

I use Firefox Dev Edition with my modification manually installed, and it has worked well for me. Something I hadn't fully tested was a cross-domain iframe, but I got that working with the latest changes. I tested in a CodePen (without saving it) using only this HTML:

<iframe width=100% height=400 src=http://www.coconut.com.fj/contact/></iframe>
Roy-Orbison commented 2 years ago

The only issues are that any existing customised shortcut cannot be copied to to the updated version, and I set the default to one that does not conflict with the core 'bookmark all tabs'. Not sure about the best way to notify users of that.

Roy-Orbison commented 2 years ago

Could you give it a whirl?

https://github.com/Roy-Orbison/textern/releases/tag/v0.8b1

Roy-Orbison commented 1 year ago

I'm still using this patched version, regularly, without issue.

Roy-Orbison commented 1 year ago

I like your idea of phasing in the new shortcut to preserve people's preferences. Might it be possible to push an update which still has a default shortcut, but uses commands.update() to migrate any existing pref across?

Roy-Orbison commented 1 year ago

@jlebon I had an idea for how to smooth out the migration, because I don't think the web extension API provides a way to differentiate updates from new installs, and it's possible to use your extension without setting any prefs.

If you push a minor version where the only change ensures a setting is saved, such as a string containing the current version, then subsequent versions will be able to detect whether they are being updated or not, based on its presence or absence. They can then update the setting after any migration is performed.

In the context of this PR, commands.update() need only be called if updating, and you would pass the users pref or the existing combo if none is set. New installs will just use whatever is specified in the manifest.

jlebon commented 1 year ago

I like your idea of phasing in the new shortcut to preserve people's preferences. Might it be possible to push an update which still has a default shortcut, but uses commands.update() to migrate any existing pref across?

Ahh nice, hadn't realized commands.update() existed. This provides the possibility of doing a seamless migration. One concern though is that looking at the supported keys and comparing it to shortcut.js, it seems like the latter supports more things. So there's a chance here that the shortcut is simply not translatable and user action is required. I would think the majority of cases should work though.

@jlebon I had an idea for how to smooth out the migration, because I don't think the web extension API provides a way to differentiate updates from new installs, and it's possible to use your extension without setting any prefs.

Hmm, we should be able to use the presence of the current shortcut key in local storage to tell apart between new installs and updates, no?

Modifying the previous plan, how about something like this:

Roy-Orbison commented 1 year ago

Hmm, we should be able to use the presence of the current shortcut key in local storage to tell apart between new installs and updates, no?

As per my previous comment, it's possible to use your extension without setting any prefs, so that key is not guaranteed to exist (I just tested this in a fresh profile). We can't know how many users rely on the default settings. Even if it's only a few, you wouldn't want to break their migration. Best to push an update that uses the existing shortcut system but also guarantees a value in the extension storage.

That timeline seems like a good approach, as long as you do the above to differentiate new installs. I hope having both shortcut methods in the code at the same time doesn't cause conflicts.

jlebon commented 1 year ago

Just a note here before I forget, I stumbled upon https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onInstalled which looks like it'd be helpful here.

Roy-Orbison commented 1 year ago

@jlebon Well done. I looked for something like that, assuming it would be there but didn't find it.