Finished evaluating all TODOs and moved to GitHub or deleted

Streamus Chrome Extension.csproj

@@ -80,6 +80,7 @@
     <Content Include="src\js\contentScript\interceptor.js" />
     <Content Include="src\js\foreground\model\dialog\createPlaylist.js" />
     <Content Include="src\js\foreground\model\dialog\editPlaylist.js" />
+    <Content Include="src\js\foreground\model\listItemButton\listItemButton.js" />
     <Content Include="src\js\foreground\model\song\songActions.js" />
     <Content Include="src\js\foreground\model\video\mediaSourceWrapper.js" />
     <Content Include="src\js\foreground\model\video\sourceBufferWrapper.js" />
@@ -95,14 +96,15 @@
     <Content Include="src\js\foreground\view\dialog\googleSignInView.js" />
     <Content Include="src\js\foreground\view\dialog\linkUserIdView.js" />
     <Content Include="src\js\foreground\view\dialog\updateStreamusView.js" />
+    <Content Include="src\js\foreground\view\listItemButton\playlistOptionsButtonView.js" />
     <Content Include="src\js\foreground\view\simpleMenu\simpleMenuItemsView.js" />
     <Content Include="src\js\foreground\view\leftPane\activePlaylistAreaView.js" />
     <Content Include="src\js\foreground\view\leftPane\leftPaneRegion.js" />
     <Content Include="src\js\foreground\view\leftPane\leftPaneView.js" />
     <Content Include="src\js\foreground\view\leftPane\playlistItemsView.js" />
     <Content Include="src\js\foreground\view\leftPane\playlistItemView.js" />
     <Content Include="src\js\foreground\view\leftPane\signInView.js" />
-    <Content Include="src\js\foreground\view\listItemButton\moreActionsButtonView.js" />
+    <Content Include="src\js\foreground\view\listItemButton\songOptionsButtonView.js" />
     <Content Include="src\js\foreground\view\tooltip\tooltipRegion.js" />
     <Content Include="src\js\foreground\view\tooltip\tooltipView.js" />
     <Content Include="src\js\foreground\view\video\videoRegion.js" />
@@ -130,6 +132,7 @@
     <Content Include="src\js\test\foreground\view\behavior\tooltipable.spec.js" />
     <Content Include="src\js\test\foreground\foregroundSpecLoader.js" />
     <Content Include="src\js\test\foreground\view\behavior\viewModelContainer.spec.js" />
+    <Content Include="src\js\test\foreground\view\listItemButton\playlistOptionsButtonView.spec.js" />
     <Content Include="src\js\test\foreground\view\simpleMenu\simpleMenuRegion.spec.js" />
     <Content Include="src\js\test\foreground\view\simpleMenu\simpleMenuSpecLoader.js" />
     <Content Include="src\js\test\foreground\view\dialog\aboutStreamusDialogView.spec.js" />
@@ -166,7 +169,7 @@
     <Content Include="src\js\test\foreground\view\listItemButton\deleteListItemButtonView.spec.js" />
     <Content Include="src\js\test\foreground\view\listItemButton\listItemButtonSpecLoader.js" />
     <Content Include="src\js\test\foreground\view\listItemButton\listItemButtonsView.spec.js" />
-    <Content Include="src\js\test\foreground\view\listItemButton\moreActionsButtonView.spec.js" />
+    <Content Include="src\js\test\foreground\view\listItemButton\songOptionsButtonView.spec.js" />
     <Content Include="src\js\test\foreground\view\listItemButton\playPauseSongButtonView.spec.js" />
     <Content Include="src\js\test\foreground\view\listItemButton\playPlaylistButtonView.spec.js" />
     <Content Include="src\js\test\foreground\view\listItemButton\saveSongButtonView.spec.js" />
@@ -214,7 +217,7 @@
     <Content Include="src\template\icon\closeIcon_24.svg" />
     <Content Include="src\template\icon\deleteIcon_18.svg" />
     <Content Include="src\template\icon\menuIcon_24.svg" />
-    <Content Include="src\template\icon\moreActionsIcon_18.svg" />
+    <Content Include="src\template\icon\optionsIcon_18.svg" />
     <Content Include="src\template\icon\pauseIcon_18.svg" />
     <Content Include="src\template\icon\settingsIcon_24.svg" />
     <Content Include="src\template\icon\pauseIcon_30.svg" />
@@ -484,7 +487,7 @@
     <Content Include="src\template\foregroundArea.html" />
     <Content Include="src\template\leftPane\playlistItems.html" />
     <Content Include="src\template\listItemButton\addListItemButton.html" />
-    <Content Include="src\template\listItemButton\moreActionsListItemButton.html" />
+    <Content Include="src\template\listItemButton\optionsListItemButton.html" />
     <Content Include="src\template\listItemButton\playPauseSongButton.html" />
     <Content Include="src\template\playlist\playlists.html" />
     <Content Include="src\template\dialog\browserSettings.html" />

src/js/background/collection/playlists.js

@@ -150,7 +150,6 @@
 
                                     playlistItems.addSongs(song, {
                                         success: function() {
-                                            //  TODO: Prefer showing notification via playlist
                                             Streamus.channels.backgroundNotification.commands.trigger('show:notification', {
                                                 title: chrome.i18n.getMessage('songAdded'),
                                                 message: song.get('title')

src/js/background/collection/streamItems.js

@@ -79,13 +79,11 @@
         },
 
         addSongs: function(songs, options) {
-            //  TODO: Reduce cyclomatic complexity.
             /* jshint ignore:start */
             options = _.isUndefined(options) ? {} : options;
             songs = songs instanceof Backbone.Collection ? songs.models : _.isArray(songs) ? songs : [songs];
 
             if (options.playOnAdd) {
-                //  TODO: Rethink playOnActivate to not have to use a channel since this is basically just a glorified global.
                 Streamus.channels.player.commands.trigger('playOnActivate', true);
             }
 
@@ -230,9 +228,7 @@
         //  Take each streamItem's array of related songs, pluck them all out into a collection of arrays
         //  then flatten the arrays into a single array of songs.
         _getRelatedSongs: function() {
-            //  TODO: I don't think this should be OK.
-            //  Some might not have information. This is OK. Either YouTube hasn't responded yet or responded with no information. Skip these.
-            //  Create a list of all the related songs from all of the information of stream items.
+            //  Some items may not have related information due to lack of response from YouTube or simply a lack of information.
             var relatedSongs = _.flatten(this.map(function(streamItem) {
                 return streamItem.get('relatedSongs').models;
             }));

src/js/background/model/dataSource.js

@@ -16,7 +16,6 @@
             url: ''
         },
 
-        //  TODO: Reduce cyclomatic complexity
         //  Take the URL given to the dataSource and parse it for relevant information.
         //  If the URL is for a Playlist -- just get the title and set the ID. If it's a Channel,
         //  need to fetch the Channel's Uploads playlist first.

src/js/background/model/nextButton.js

@@ -14,7 +14,6 @@
         },
 
         initialize: function() {
-            //  TODO: There's a LOT of things to listen to here. Once Stream can tell me its nextItem I should be able to clean this up easier.
             this.listenTo(this.get('stream').get('items'), 'add remove reset change:active sort', this._setEnabled);
             this.listenTo(this.get('radioButton'), 'change:enabled', this._onRadioButtonChangeEnabled);
             this.listenTo(this.get('shuffleButton'), 'change:enabled', this._onShuffleButtonChangeEnabled);

src/js/background/model/player.js

@@ -20,7 +20,6 @@ define(function(require) {
                 ready: false,
                 loading: false,
                 currentLoadAttempt: 1,
-                //  TODO: maxLoadAttempts isn't DRY with YouTubePlayer.
                 maxLoadAttempts: 10,
                 previousState: PlayerState.Unstarted,
                 state: PlayerState.Unstarted,
@@ -233,7 +232,6 @@ define(function(require) {
         _ensureInitialState: function() {
             this.set('ready', this.get('youTubePlayer').get('ready'));
             this.set('loading', this.get('youTubePlayer').get('loading'));
-            //  TODO: How will I handle currentLoadAttempt w/ 2+ APIs? If both are loading they could be on separate attempts...?
             this.set('currentLoadAttempt', this.get('youTubePlayer').get('currentLoadAttempt'));
         },
 
@@ -446,7 +444,7 @@ define(function(require) {
                     playerState = PlayerState.Buffering;
                     break;
                 case YouTubePlayerState.SongCued:
-                    //  TODO: I'm mapping SongCued to 'paused', but it shouldn't ever happen in the wild now. Remove this entirely in v0.175+ once confirmed.
+                    //  This should not occur in the wild. Remove in v0.175+ once confirmed.
                     playerState = PlayerState.Paused;
                     Streamus.channels.error.commands.trigger('log:error', new Error('Unexpected PlayerState.SongCued event.'));
                     break;

src/js/background/model/previousButton.js

@@ -13,7 +13,6 @@
         },
 
         initialize: function() {
-            //  TODO: There's a LOT of things to listen to here. Once Stream can tell me its previousItem I should be able to clean this up easier.
             this.listenTo(this.get('stream').get('items'), 'add remove reset change:active sort', this._toggleEnabled);
             this.listenTo(this.get('player'), 'change:currentTime', this._onPlayerChangeCurrentTime);
             this.listenTo(this.get('shuffleButton'), 'change:enabled', this._onShuffleButtonChangeState);

src/js/background/model/search.js

@@ -89,9 +89,6 @@
         //  Handle the actual search functionality inside of a debounced function.
         //  This is so I can tell when the user starts typing, but not actually run the search logic until they pause.
         _doDebounceSearch: _.debounce(function(trimmedQuery) {
-            //  TODO: What happens between now and when parseUrl is running? It will look like no search is being performed?
-            this.set('searchQueued', false);
-
             //  If the user typed 'a' and then hit backspace, debounce search will still be trying to run with 'a'
             //  because no future search query arrived. Prevent this.
             if (this._getTrimmedQuery() === trimmedQuery) {
@@ -112,8 +109,13 @@
                         } else {
                             this._setResultsByText(trimmedQuery);
                         }
+
+                        //  Set to false only after setting a new pendingRequest to ensure that the 'isSearching' doesn't flicker to false.
+                        this.set('searchQueued', false);
                     }.bind(this)
                 });
+            } else {
+                this.set('searchQueued', false);
             }
         }, 350),
 
@@ -128,7 +130,7 @@
         },
 
         _setResultsByPlaylist: function(playlistId) {
-            //  TODO: This is not DRY with how a Playlist loads its songs internally, how can I share the logic?
+            //  https://github.com/MeoMix/StreamusChromeExtension/issues/567
             var pendingRequest = YouTubeV3API.getPlaylistSongs({
                 playlistId: playlistId,
                 success: this._onGetPlaylistSongsSuccess.bind(this, playlistId),
@@ -216,7 +218,9 @@
         },
 
         _isSearching: function(searchQueued, pendingRequest) {
-            return searchQueued || pendingRequest !== null;
+            var isSearching = searchQueued || pendingRequest !== null;
+            console.log('isSearching:', isSearching);
+            return isSearching;
         },
 
         _onForegroundEndUnload: function() {

src/js/background/model/signInManager.js

@@ -24,7 +24,7 @@
         initialize: function() {
             this.on('change:signedInUser', this._onChangeSignedInUser);
             this.on('change:signInFailed', this._onChangeSignInFailed);
-            this.listenTo(Streamus.channels.backgroundArea.vent, 'rendered', this._onBackgroundAreaRendered);
+            //this.listenTo(Streamus.channels.backgroundArea.vent, 'rendered', this._onBackgroundAreaRendered);
             chrome.runtime.onMessage.addListener(this._onChromeRuntimeMessage.bind(this));
             chrome.runtime.onMessageExternal.addListener(this._onChromeRuntimeMessageExternal.bind(this));
             chrome.identity.onSignInChanged.addListener(this._onChromeIdentitySignInChanged.bind(this));
@@ -147,6 +147,7 @@
         },
 
         _onChangeSignInFailed: function(model, signInFailed) {
+            console.log('signInFailed');
             if (signInFailed) {
                 this._onSignInFailed();
             }
@@ -244,10 +245,21 @@
             switch (request.method) {
                 case 'copyPlaylist':
                     if (this._canSignIn()) {
-                        //  TODO: What if sign in fails?
-                        this.once('change:signedInUser', function() {
+                        var onSignInSuccess = function() {
                             this._handleCopyPlaylistRequest(request, sendResponse);
-                        });
+                            this.off('change:signInFailed', onSignInFailed);
+                        };
+
+                        var onSignInFailed = function() {
+                            this.off('change:signedInUser', onSignInSuccess);
+                            sendResponse({
+                                result: 'error',
+                                error: 'Failed to sign in'
+                            });
+                        };
+
+                        this.once('change:signedInUser', onSignInSuccess);
+                        this.once('change:signInFailed', onSignInFailed);
 
                         this.signInWithGoogle();
                     } else {
@@ -257,7 +269,6 @@
                     sendAsynchronousResponse = true;
                     break;
                 case 'isUserLoaded':
-                    //  TODO: Maybe this should also be able to sign in a user since I'm going to poll isUserLoaded and if nobody is signing in then why continue to poll?
                     sendResponse({
                         isUserLoaded: this.get('signedInUser') !== null
                     });
@@ -269,7 +280,7 @@
         },
 
         _handleCopyPlaylistRequest: function(request, sendResponse) {
-            //  TODO: Probably house this logic on signedInUser or on playlists?
+            //  It would be nice to handle this at a lower level, but I need the ability to sign a user in first.
             this.get('signedInUser').get('playlists').copyPlaylist({
                 playlistId: request.playlistId,
                 success: function() {
@@ -297,7 +308,7 @@
                 callback(false);
             }
         },
-        //  TODO: Just set this properties manually instead of via function.
+
         _needLinkUserId: function() {
             this.set('needLinkUserId', true);
         },

src/js/background/model/stream.js

@@ -42,7 +42,6 @@
 
             var activeItem = this.get('items').getActiveItem();
             if (!_.isUndefined(activeItem)) {
-                //  TODO: Perhaps I could write activeItem to localStorage and convert it back from JSON to a model on initialize, but doing this for now.
                 this.set('activeItem', activeItem);
                 this.get('player').activateSong(activeItem.get('song'));
             }
@@ -159,7 +158,6 @@
 
         //  Return the previous item or null without affecting the history.
         getPrevious: function() {
-            //  TODO: Reduce cyclomatic complexity.
             /* jshint ignore:start */
             var previousStreamItem = null;
             var history = this.get('history');

src/js/background/model/youTubeV3API.js

@@ -236,7 +236,6 @@
                         }
                     } else {
                         //  Filter out videos which have marked themselves as not able to be embedded since they won't be able to be played in Streamus.
-                        //  TODO: Notify the user that this has happened.
                         var embeddableItems = _.filter(response.items, function(item) {
                             //  Check for PT0S due to an issue in YouTube's API: https://code.google.com/p/gdata-issues/issues/detail?id=7172
                             //  Songs with 0s duration are unable to be played.
@@ -297,6 +296,5 @@
         }
     });
 
-    //  TODO: Don't return new instance even if its not stateful.
     return new YouTubeV3API();
 });

src/js/foreground/model/listItemButton/listItemButton.js

@@ -0,0 +1,11 @@
+define(function() {
+    'use strict';
+
+    var ListItemButton = Backbone.Model.extend({
+        defaults: {
+            enabled: true
+        }
+    });
+
+    return ListItemButton;
+});

src/js/foreground/view/appBar/appBarView.js

@@ -44,8 +44,8 @@
             hideSearchButton: '[data-ui~=hideSearchButton]',
             showPlaylistsAreaButton: '[data-ui~=showPlaylistsAreaButton]',
             hidePlaylistsAreaButton: '[data-ui~=hidePlaylistsAreaButton]',
-            //  TODO: I don't like regions being manipulated in UI they should be stateless.
-            //  To achieve this, I'll need to make playlistTitleRegion and searchInputRegion the same region and swap views out.
+            //  I don't like regions being manipulated in UI they should be stateless.
+            //  However, this area is going to change pretty soon once the new UI gets added. So, no need to fix right now.
             playlistTitleRegion: '[data-ui~=playlistTitleRegion]',
             searchInputRegion: '[data-ui~=searchInputRegion]'
         },

src/js/foreground/view/behavior/listItemButton.js

@@ -34,8 +34,7 @@
         },
 
         _announceClick: function(event) {
-            //  TODO: Prefer not to check hasClass and leverage state of a model.
-            if (!this.$el.hasClass('is-disabled')) {
+            if (this.view.model.get('enabled')) {
                 this.view.triggerMethod('Click');
             }
             //  Since returning false, need to announce the event happened here since root level won't know about it.

src/js/foreground/view/behavior/slidingRender.js

@@ -127,7 +127,6 @@
         },
 
         _setRenderedElements: function(scrollTop) {
-            //  TODO: Reduce cyclomatic complexity
             /* jshint ignore:start */
             //  Figure out the range of items currently rendered:
             var currentMinRenderIndex = this.minRenderIndex;
@@ -354,7 +353,6 @@
         },
 
         _onCollectionRemove: function(item, collection, options) {
-            //  TODO: It would be nice to find a way to not have to leverage _.defer here.
             //  Use _.defer to wait for the view to remove the element corresponding to item.
             //  _renderElementAtIndex has an off-by-one error if executed immediately.
             _.defer(function() {

src/js/foreground/view/behavior/sortable.js

@@ -163,9 +163,9 @@
                     index: placeholderIndex + response.minRenderIndex
                 });
 
-                //  TODO: Since I provided the index that the item would be inserted at in the collection, the collection does not resort.
-                //  But, the index in the collection does not correspond to the index in the CollectionView -- that's simply the placeholderIndex. Not sure how to handle that.
-                //  I'd need to intercept the _onCollectionAdd event of Marionette, calculate the proper index, and pass bypass: true in, but not going to do that for now.
+                //  The collection does not resort because the model's index was provided when calling addSongs 
+                //  The CollectionView rendering the model is now incorrect because the collection's index does not correspond to the CollectionView's index.
+                //  Simply triggering a sort is the simplest solution as it forces the CollectionView to re-render its children.
                 this.view.collection.sort();
             }.bind(this));
             this.view.triggerMethod('GetMinRenderIndex');

src/js/foreground/view/behavior/tooltipable.js

@@ -18,7 +18,6 @@
         events: {
             'mouseenter': '_onMouseEnter',
             'mouseleave': '_onMouseLeave',
-            //  TODO: Technically these event names are incorrect.
             'mouseenter @ui.tooltipable': '_onMouseEnter',
             'mouseleave @ui.tooltipable': '_onMouseLeave',
             'mouseenter @ui.textTooltipable': '_onMouseEnter',

src/js/foreground/view/behavior/viewModelContainer.js

@@ -76,7 +76,6 @@
             var viewModel = this.view[viewModelName];
             viewModel.stopListening();
             delete viewModel._attachedToView;
-            //  TODO: Should the model itself be deleted since any event handlers it setup are non-functional? Could be misleading if still available for use.
         }
     });