search

msfeldstein / chrome-media-keys

Chrome extension that adds media keys to most web music players
GNU Affero General Public License v3.0
89 stars 33 forks source link

New Pandora and ES6 conversion #103

Closed Ustice closed 7 years ago

Ustice commented 7 years ago

Changes included

This pull request makes the following changes:

ES6-style refactor

The BasicController.js file has been refactored to use ES6-style javascript which is now supported by Chrome. This improves the code readability and maintainability.

Multiple configurations

Multiple configurations are supported now in a similar manner to previous. The configuration that is passed to the BasicController constructor can now be an array of configuration objects. Before the configuration is loaded, the constructor now runs the test method to determine if that configuration object should be used, and the first successful test is the only one used. All others are ignored (so that they can be put in priority order). This is to support different versions of pages running on the same host. To support backward compatibility, if a configuration does not have a test method, then it is used.

Simplified method override

Method overrides no longer necessitate the override method, and can simply be added onto the configuration object. Note that if the method needs access to the controller from inside the method, it needs to be a regular function, and not an arrow function. The BasicTemplate.js file has been updated to reflect the changes. The override method is kept for backward compatibility, but should likely be deprecated.

Pandora's new site

Support is added for Pandora's new site.

msfeldstein commented 7 years ago

I might be missing something, but i don't see where we're using the new "test()" method here to decide which version of the controller to use

msfeldstein commented 7 years ago

nevermind, i didn't realize github collapsed the diff for BasicController (why would they do that?)

Ustice commented 7 years ago

It's just too damned big. 😅

msfeldstein commented 7 years ago

Overall this looks good. Want to wait to merge until i have a little bit of time to test a few things, but i like it. Makes me think we should get rid of coffeescript as well :)

Ustice commented 7 years ago

I'm glad you think so. Maybe do some work to the build process. Adding some integration tests would be good too to detect when a controller no longer works for a page.

msfeldstein commented 7 years ago

Yeah if you have ideas for how to add that kidn of test im all ears. I think we could get to a point where we can at least verify that the controller is detected, which i think would catch 90% of the changes (usually things break after big site redesigns)

On Sat, Dec 3, 2016 at 8:48 PM Jason Kleinberg notifications@github.com wrote:

I'm glad you think so. Maybe do some work to the build process. Adding some integration tests would be good too to detect when a controller no longer works for a page.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/103#issuecomment-264684278, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ1b8VYO0aakP2x_YZM3J7sD1qltIDlks5rEkYggaJpZM4LCqUa .

Ustice commented 7 years ago

Stack Overflow to the rescue! We should be able to open tabs automatically and test whether we can access all of the elements on the page at least.

Ustice commented 7 years ago

Rebased.

nemchik commented 7 years ago

This gave me an idea.

Would it be possible (in the future, not in this PR) to use test() or something else to determine if a media key is pressed and does not work, and then show an alert to the user, something to the effect of "Oops! The media keys on this site are not working! Please visit GitHub to let us know!" ? I would imagine only showing that message once (flag that it's been shown) until the next time that specific controller is loaded and then again fails to work when a media key is pressed.

Just kicking ideas around. This PR is way over my head, but looking through it got me thinking about the above.

msfeldstein commented 7 years ago

I would rather just log an error automatically but yes

On Sun, Dec 4, 2016, 4:54 AM Eric Nemchik notifications@github.com wrote:

This gave me an idea.

Would it be possible (in the future, not in this PR) to use test() or something else to determine if a media key is pressed and does not work, and then show an alert to the user, something to the effect of "Oops! The media keys on this site are not working! Please visit GitHub to let us know!" ? I would imagine only showing that message once (flag that it's been shown) until the next time that specific controller is loaded and then again fails to work when a media key is pressed.

Just kicking ideas around. This PR is way over my head, but looking through it got me thinking about the above.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/103#issuecomment-264702331, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ1bxS46VIIYLFavMWwQEBZm9bn4pYhks5rErf-gaJpZM4LCqUa .

nemchik commented 7 years ago

Would it make more sense then to use analytics to record hits to a URL when something fails?

Maybe something like this? failure.html?controller=Pandora&button=play Any time the extension detects a failure with a certain site or button it would send an HTTP request a URL such as this. The html file would pretty much just be for analytics, but it should capture the query strings in the URL. This would allow you to fairly quickly determine if a certain site suddenly went through a redesign that requires updates to a controller. The end user wouldn't see anything other than the controller not working.

msfeldstein commented 7 years ago

Yes this would be fantastic

On Sun, Dec 4, 2016, 4:37 PM Eric Nemchik notifications@github.com wrote:

Would it make more sense then to use analytics to record hits to a URL when something fails?

Maybe something like this? failure.html?controller=Pandora&button=play Any time the extension detects a failure with a certain site or button it would send an HTTP request a URL such as this. The html file would pretty much just be for analytics, but it should capture the query strings in the URL. This would allow you to fairly quickly determine if a certain site suddenly went through a redesign that requires updates to a controller. The end user wouldn't see anything other than the controller not working.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/103#issuecomment-264744845, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ1b5TXRFa8igEvYLdcLasPVSEH2jS_ks5rE1zJgaJpZM4LCqUa .

Ustice commented 7 years ago

Is there anything missing in this PR?