dgutov / bmreplace

Quickly replace an existing bookmark in Mozilla Firefox
https://addons.mozilla.org/en-US/firefox/addon/bmreplace/
MIT License
24 stars 3 forks source link

New version breaks on ESR #42

Open trlkly opened 7 years ago

trlkly commented 7 years ago

Version 1.6.4 doesn't seem to work at all on Firefox ESR 52. The button is present, but does nothing. Version 1.6.2 continues to work just fine.

Since ESR 52 will continue until mid 2018, I thought it might be a good idea to mark 1.6.4 (and 1.6.3, if it exists) as requiring Firefox 53 at minimum (or whatever the actual minimum is) and let 1.6.3 be the last version for Firefox 52. (Or, if you want, troubleshoot the problem.)

Since no one else seems to have reported problems, I presume that others either just shut off updates or aren't using ESR. So no need to retroactively fix things. However, as this will be the only way to preserve many extensions that don't have WebExtension versions after Firefox drops XUL/XPCOM support, there may be an influx of more users soon.

It'd be good if this addon continued to work on Firefox 52 until ESR 52 disappears.

dgutov commented 7 years ago

Thanks for the report.

Anything interesting in the Browser Console when that happens?

trlkly commented 7 years ago

Unfortunately not. Both the browser console (Ctrl-Shift-J) and the page console (Ctrl-Shift-K) are entirely blank when I press the button or use the shortcut key.

Just Google Firefox ESR if you want to test on it. Just be sure to start it with -P so it will ask you to pick a different profile (for testing purposes). And use -no-remote if Firefox is already running.

(You may already know that, but I'm just including it to be helpful.)

On Sat, Sep 9, 2017 at 4:50 PM, Dmitry Gutov notifications@github.com wrote:

Thanks for the report.

Anything interesting in the Browser Console when that happens?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dgutov/bmreplace/issues/42#issuecomment-328305464, or mute the thread https://github.com/notifications/unsubscribe-auth/AFNSruRp5aVbqNueCiCtCJ17ohzQLCc3ks5sgwgugaJpZM4PSIRV .

dgutov commented 7 years ago

You may already know that

Yup. And what's worse, there's nothing in the difference between the two versions that jumps out at me as a potential problem.

So maybe the users of ESR will have to simply install 1.6.2.

trlkly commented 7 years ago

Oddly, 1.6.3 also works. So it would be something that changed for 1.6.4.

If you mark 1.6.4 as requiring Firefox 53, do you know if it the addon will serve the older version to Firefox 52? Or, at the very least, will it still show up in search? Because then you could at least include a note telling people to install 1.6.3 for Firefox 52. (You can even link it in the description.)

Another idea is to release a 1.6.5 that tests the version of Firefox, and uses the old code for Firefox 52 and the new code for 56.

dgutov commented 7 years ago

If you mark 1.6.4 as requiring Firefox 53, do you know if it the addon will serve the older version to Firefox 52? Or, at the very least, will it still show up in search?

I've made that change. Try and see for yourself.

dgutov commented 7 years ago

The difference is here: https://github.com/dgutov/bmreplace/commit/396ac67ef275ea7b09c858543ab31eededd0df89, and it's kind of ridiculous that either version of the code can fail to work in any of the recent Firefox versions.

trlkly commented 7 years ago

I just see a gray button telling me that the addon doesn't work with my version of Firefox. No mention of previous versions that might.

At the very least, I would ask that you mention something like "For Firefox ESR 52, please install version 1.6.3 from the Versions page" and link the versions page.

Still, I think the ideal would be to either figure this out or just bifurcate the code with a system.version test from the system API.

I honestly don't know how these APIs work. But the thing that seems like it might have potential is that you rolled your own getDescriptionFromDocument. Maybe have that function add an if that checks system.version (from the system API) and, if it contains "52.", just import the other version and return a call to that function.

dgutov commented 7 years ago

I just see a gray button telling me that the addon doesn't work with my version of Firefox

OK, I lowered the minimum version for now.