Closed DanH42 closed 11 years ago
Dan, thanks for the contribution! I'm going to accept the pull request, albeit a bit reluctantly. I'm gonna offer a little bit of advice, but feel free to take it however.
Obviously this is a quick fix, but if you wanted to do it in it's entirety here's what you could have done differently: 1) the comments should have been removed before committing. 2) the code formatting does not match the surrounding code. 3) there are a dozen other settings, images, code, files, etc that were used when htmlNotifications worked. Since htmlNotifications is deprecated, putting a band aid on RE to handle it might not be the best approach. Everything we use with htmlNotifications should also be deprecated. The files should be moved to the support folder in case we want to use them in the future for something else, and the settings dialog needs to be changed in order to reflect those changes.
I'm glad to help out! Your thoughts are plenty welcome. Although this was just a quick band-aid, there are a couple things I'd like to point out:
I mostly just submitted this because I felt bad for the poor Windows and ChromeOS users who were getting no notifications, and I felt like anything would be better than nothing. In the future, it would probably be good to include Chromium's suggested replacement for HTML notifications: the new "rich" notification API. I haven't played with it much, and while it's considerably less capable than HTML notifications, it does still allow for 2 non-stylable buttons to be included, and those could give people at least play/pause and skip. However, as primarily a Linux user, I don't have access to that API yet, so I didn't add anything for them.
I'm a big fan and longtime user of RadioEnhancer (since it was PandoraEnhancer). Once HTML notifications are fully removed from all Chrome versions, if it's been a while and you or Brandon haven't added support for the new notifications, and I've got some spare time, I may take a stab at it. Either way, best of luck to you both, and keep up the good work!
Thanks Dan!
You're right, we should support it until every platform goes away. But, there might be a better way to approach the whole thing. In the future, we still want to have the functionality of that player.
We were toying around with the idea of doing something similar to what SoundControl does and put the player all the way up top with its own icon. The new rich notifications would then be able to complement the player instead of completely replacing it!
There's a handful of other features that we'd likely implement at that point, too. Over a year ago we made a working prototype that took advantage of a flaw with how Pandora actually plays songs. This allows us to do unlimited skips, seeking, going back to a previous song, etc. Those are some killer features that we've been on the fence about releasing.
Saying that, you are welcome to implement the rich notifications if you'd like. I can imagine we'll hopefully still be able to use that code once we transition into the next phase of development. Brandon and I have lost a little bit of momentum with the extension, but our active user base is still climbing. It might be time for us to jump back in!
This is a dead simple fix that will allow all the poor Windows/ChromeOS users to at least see some form of notification when songs change.