search

tjhrulz / WebNowPlaying-BrowserExtension

The extension to go along with the WebNowPlaying plugin for Rainmeter
74 stars 35 forks source link

Suggestion of code reorganization #3

Closed alexesprit closed 6 years ago

alexesprit commented 7 years ago

Hi Trevor!

I like the idea of this extension and the Rainmeter plugin, and I want to contibute this project somehow.

I noticed all scripts, which gather track information, duplicate common code (WebSocket setup, event dispatching, clicking on elements etc). I suggest to extract all the code to a single module (I named it 'connector') and share it between all scripts. I pushed changes that describe the idea better: https://github.com/alexesprit/WebNowPlaying-ChromeExtension/tree/example.

The possible future improvement is to add a background script that will inject content scripts into a page, so you don't need to include 'connector.js' for every website.

Should I continue this work, and is there a chance these changes will be accepted?

tjhrulz commented 7 years ago

I see no reason to not continue this, I am not a big javacript person so both the extensions were hacked together over the course of a weekend a few months back and still worked so I just stuck with it. I was already considering doing some refactoring anyways.

The only slight concern I have is running a script on every website (Although on second glance it seems I may have misunderstood what you meant by every website). While I know the code is not malicious it seems an unneeded permission request that some people may dislike. However though I have though I may have to make some changes though that way the extension can do things like pickup embeded youtube videos and if it does serve a better purpose I have no issue running a basic script on every page.

tjhrulz commented 6 years ago

I have rewritten the extension to make it easier to add support for new players in the future. While I did not add something like an enum to make things like repeat states more readable like in your example I did make a generic player that tries to dynamically add support for websites.

I have not committed it yet but I do plan to tonight.

tjhrulz commented 6 years ago

Commited with https://github.com/tjhrulz/WebNowPlaying-BrowserExtension/commit/dfc2d4f500d6c07d3f18e64bef482a97b37fc305