search

gurumukhi / youtube-screenshot

Cross browser addon for youtube video screenshots
https://addons.mozilla.org/en-US/firefox/addon/youtube-screenshot-button/
Mozilla Public License 2.0
43 stars 9 forks source link

Call once guard in initialization? #19

Closed joggee-fr closed 2 years ago

joggee-fr commented 2 years ago

Do we really need this call once guard: https://github.com/gurumukhi/youtube-screenshot/blob/8b1e32eae89cdb7a44d924e079207e5e9c388d91/content_script.js#L69 ? I am very new to WebExtension dev but trying to improve this useful one. I am in the good way to fix an issue when the add-on is loaded opening the YouTube homepage, not directly a video page. However, as the patch is done in the initialization phase, I wonder if it is useful to keep this guard.

Thanks.

gurumukhi commented 2 years ago

This guard makes sure duplicate button is not added if the init method is called again on same page where button is already added (there could be various scenarios when this might happen).

On Fri, 16 Sep, 2022, 13:32 Jonathan GUILLOT, @.***> wrote:

Do we really need this call once guard:

https://github.com/gurumukhi/youtube-screenshot/blob/8b1e32eae89cdb7a44d924e079207e5e9c388d91/content_script.js#L69 ? I am very new to WebExtension dev but trying to improve this useful one. I am in the good way to fix an issue when the add-on is loaded opening the YouTube homepage, not directly a video page. However, as the patch is done in the initialization phase, I wonder if it is useful to keep this guard.

Thanks.

— Reply to this email directly, view it on GitHub https://github.com/gurumukhi/youtube-screenshot/issues/19, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU7ELIAYVTZ5HFMYMOFKB3V6QSQRANCNFSM6AAAAAAQOCZKWI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

joggee-fr commented 2 years ago

Thanks @gurumukhi. I tried to reproduce such a case but was not able to do in "normal" cases. I encountered such a behavior when reloading the add-on in about:debugging of Firefox, but it is surely not a typical use case. Furthermore, the guard is not working in such a case. IMHO, if the whole script is reloaded, the initCalled variable is set to falseagain and screenshot button is consequently duplicated. If you can give me a way to reproduce, I would be happy to try.

gurumukhi commented 2 years ago

You might be right. I think we should get rid of this.

Also, see if you can look into issues #15 & #8 too. Then I will merge all PRs together and republish the add-on.

On Sat, 17 Sep, 2022, 14:35 Jonathan GUILLOT, @.***> wrote:

Thanks @gurumukhi https://github.com/gurumukhi. I tried to reproduce such a case but was not able to do in "normal" cases. I encountered such a behavior when reloading the add-on in about:debugging of Firefox, but it is surely not a typical use case. Furthermore, the guard is not working in such a case. IMHO, if the whole script is reloaded, the initCalled variable is set to falseagain and screenshot button is consequently duplicated. If you can give me a way to reproduce, I would be happy to try.

— Reply to this email directly, view it on GitHub https://github.com/gurumukhi/youtube-screenshot/issues/19#issuecomment-1250033674, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU7ELIJPXPSFD3XZTWFJWTV6WCUFANCNFSM6AAAAAAQOCZKWI . You are receiving this because you were mentioned.Message ID: @.***>

joggee-fr commented 2 years ago

I am preparing a new PR to get rid of this unneeded guard and fix incorrect behavior when starting at YouTube homepage. I am not very sure I can take a look at issues #15 and #8 soon. You can already review my PRs #17 and #18 😄, for different topics.

joggee-fr commented 2 years ago

@gurumukhi, is the project still alive?

gurumukhi commented 2 years ago

Yes it is.

Should I merge your PRs now? Sorry for delay.

On Mon, 26 Sep, 2022, 13:14 Jonathan GUILLOT, @.***> wrote:

@gurumukhi https://github.com/gurumukhi, is the project still alive?

— Reply to this email directly, view it on GitHub https://github.com/gurumukhi/youtube-screenshot/issues/19#issuecomment-1257628423, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU7ELPOH246TAABNWVHFM3WAFH5HANCNFSM6AAAAAAQOCZKWI . You are receiving this because you were mentioned.Message ID: @.***>

gurumukhi commented 2 years ago

Merged two PRs, can you please help me resolve this conflict https://github.com/gurumukhi/youtube-screenshot/pull/18/conflicts ?

joggee-fr commented 2 years ago

Thanks for the merges @gurumukhi. I have just updated the PR #18 to fix the conflicts. Let me know if you wish me to change something.

By the way, this issue #19 is now fixed for me.