Cleaning up more TODOs

MeoMix · May 17, 2015 · 0b24b92 · 0b24b92
1 parent fdafe0c
commit 0b24b92
Show file tree

Hide file tree

Showing 57 changed files with 259 additions and 166 deletions.
diff --git a/Streamus Chrome Extension.csproj b/Streamus Chrome Extension.csproj
@@ -78,11 +78,14 @@
     <Content Include="src\js\common\lodashMixin.js" />
     <Content Include="src\js\contentScript\interceptor.js" />
     <Content Include="src\js\foreground\model\contextMenu\contextMenuAction.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\element\fixedMenuItem.js" />
     <Content Include="src\js\foreground\model\video\mediaSourceWrapper.js" />
     <Content Include="src\js\foreground\model\video\sourceBufferWrapper.js" />
     <Content Include="src\js\foreground\enum\keyCode.js" />
     <Content Include="src\js\foreground\model\tooltip\tooltip.js" />
+    <Content Include="src\js\foreground\view\contextMenu\contextMenuItemsView.js" />
     <Content Include="src\js\foreground\view\dialog\aboutStreamusDialogView.js" />
     <Content Include="src\js\foreground\view\dialog\aboutStreamusView.js" />
     <Content Include="src\js\foreground\view\element\fixedMenuItemView.js" />
@@ -168,7 +171,7 @@
     <Content Include="src\js\test\foreground\view\playlist\playlistsView.spec.js" />
     <Content Include="src\js\test\foreground\view\playlist\playlistView.spec.js" />
     <Content Include="src\js\test\foreground\view\saveSongs\saveSongsRegion.spec.js" />
-    <Content Include="src\js\test\foreground\view\search\searchAreaRegion.spec.js" />
+    <Content Include="src\js\test\foreground\view\search\searchRegion.spec.js" />
     <Content Include="src\js\test\foreground\view\search\searchResultsView.spec.js" />
     <Content Include="src\js\test\foreground\view\search\searchResultView.spec.js" />
     <Content Include="src\js\test\foreground\view\search\searchSpecLoader.js" />
@@ -350,7 +353,7 @@
     <Content Include="src\js\foreground\view\stream\saveStreamButtonView.js" />
     <Content Include="src\js\foreground\view\stream\shuffleButtonView.js" />
     <Content Include="src\js\foreground\view\stream\streamItemsView.js" />
-    <Content Include="src\js\foreground\view\search\searchAreaRegion.js" />
+    <Content Include="src\js\foreground\view\search\searchRegion.js" />
     <Content Include="src\js\foreground\view\listItemButton\listItemButtonView.js" />
     <Content Include="src\js\foreground\view\notification\notificationRegion.js" />
     <Content Include="src\js\foreground\view\notification\notificationView.js" />

diff --git a/src/js/background/model/song.js b/src/js/background/model/song.js
@@ -25,11 +25,6 @@
             this._setPrettyDuration(this.get('duration'));
             this._setCleanTitle(this.get('title'));
             this._setUrl(this.get('id'));
-
-            //  TODO: Why can song values even change?
-            this.on('change:duration', this._onChangeDuration);
-            this.on('change:title', this._onChangeTitle);
-            this.on('change:id', this._onChangeId);
         },
 
         copyUrl: function() {
@@ -49,18 +44,6 @@
                 message: chrome.i18n.getMessage('urlCopied')
             });
         },
-
-        _onChangeId: function(model, id) {
-            this._setUrl(id);
-        },
-
-        _onChangeTitle: function(model, title) {
-            this._setCleanTitle(title);
-        },
-
-        _onChangeDuration: function(model, duration) {
-            this._setPrettyDuration(duration);
-        },
 
         //  Calculate this value pre-emptively because when rendering I don't want to incur inefficiency
         _setPrettyDuration: function(duration) {

diff --git a/src/js/contentScript/beatport.js b/src/js/contentScript/beatport.js
@@ -18,8 +18,6 @@
             //  Prevent Beatport from playing all tracks.
             event.stopPropagation();
 
-            //  TODO: Prefer to not have to re-find tracks by passing them into the function, but
-            //  it's tricky because I'd need to use a 'bind' which gets hard with removeEventListener.
             var container = document.querySelector('.bucket[class*=tracks]');
             var streamusButtons = container.querySelectorAll('.bucket-items .bucket-item .streamusButton');
 

diff --git a/src/js/foreground/application.js b/src/js/foreground/application.js
@@ -15,7 +15,7 @@
             window: Backbone.Wreqr.radio.channel('window'),
             contextMenu: Backbone.Wreqr.radio.channel('contextMenu'),
             playlistsArea: Backbone.Wreqr.radio.channel('playlistsArea'),
-            searchArea: Backbone.Wreqr.radio.channel('searchArea'),
+            search: Backbone.Wreqr.radio.channel('search'),
             activeStreamItemArea: Backbone.Wreqr.radio.channel('activeStreamItemArea'),
             element: Backbone.Wreqr.radio.channel('element'),
             saveSongs: Backbone.Wreqr.radio.channel('saveSongs'),

diff --git a/src/js/foreground/model/contextMenu/contextMenu.js b/src/js/foreground/model/contextMenu/contextMenu.js
@@ -8,6 +8,9 @@
             return {
                 top: 0,
                 left: 0,
+                //  Used to determine whether contextMenu display should flip as to not overflow container
+                containerHeight: 0,
+                containerWidth: 0,
                 items: new ContextMenuItems()
             };
         },

diff --git a/src/js/foreground/model/dialog/createPlaylist.js b/src/js/foreground/model/dialog/createPlaylist.js
@@ -0,0 +1,28 @@
+define(function() {
+    'use strict';
+
+    var CreatePlaylist = Backbone.Model.extend({
+        defaults: {
+            valid: false,
+            dataSourceValid: true,
+            titleValid: false
+        },
+
+        initialize: function() {
+            this.on('change:dataSourceValid', this._onChangeDataSourceValid);
+            this.on('change:titleValid', this._onChangeTitleValid);
+        },
+
+        _onChangeDataSourceValid: function(model, dataSourceValid) {
+            var valid = dataSourceValid && this.get('titleValid');
+            this.set('valid', valid);
+        },
+
+        _onChangeTitleValid: function(model, titleValid) {
+            var valid = titleValid && this.get('dataSourceValid');
+            this.set('valid', valid);
+        }
+    });
+
+    return CreatePlaylist;
+});
diff --git a/src/js/foreground/model/dialog/editPlaylist.js b/src/js/foreground/model/dialog/editPlaylist.js
@@ -0,0 +1,12 @@
+define(function() {
+    'use strict';
+
+    var EditPlaylist = Backbone.Model.extend({
+        defaults: {
+            playlist: null,
+            valid: true
+        }
+    });
+
+    return EditPlaylist;
+});
diff --git a/src/js/foreground/model/element/checkbox.js b/src/js/foreground/model/element/checkbox.js
@@ -6,6 +6,7 @@
             //  Primary checkboxes are colored more boldly than secondary.
             primary: true,
             checked: false,
+            checking: false,
             labelText: '',
             //  Often times a checkbox has a 1:1 relationship with a model property.
             //  Linking that property to its checkbox allows working with a collection of checkboxes more easily.

diff --git a/src/js/foreground/view/appBar/appBarView.js b/src/js/foreground/view/appBar/appBarView.js
@@ -77,8 +77,8 @@
 
             this.listenTo(Streamus.channels.playlistsArea.vent, 'hiding', this._onPlaylistsAreaHiding);
             this.listenTo(Streamus.channels.playlistsArea.vent, 'showing', this._onPlaylistsAreaShowing);
-            this.listenTo(Streamus.channels.searchArea.vent, 'hiding', this._onSearchAreaHiding);
-            this.listenTo(Streamus.channels.searchArea.vent, 'showing', this._onSearchAreaShowing);
+            this.listenTo(Streamus.channels.search.vent, 'hiding', this._onSearchHiding);
+            this.listenTo(Streamus.channels.search.vent, 'showing', this._onSearchShowing);
 
             var signedInUser = this.signInManager.get('signedInUser');
             if (signedInUser !== null) {
@@ -148,16 +148,16 @@
         },
 
         _onClickShowSearchButton: function() {
-            Streamus.channels.searchArea.commands.trigger('show:search');
+            Streamus.channels.search.commands.trigger('show:search');
             Streamus.channels.playlistsArea.commands.trigger('hide:playlistsArea');
         },
 
         _onClickHideSearchButton: function() {
-            Streamus.channels.searchArea.commands.trigger('hide:search');
+            Streamus.channels.search.commands.trigger('hide:search');
         },
 
         _onInputSearchInput: function() {
-            Streamus.channels.searchArea.commands.trigger('search', {
+            Streamus.channels.search.commands.trigger('search', {
                 query: this.ui.searchInput.val()
             });
         },
@@ -167,7 +167,7 @@
 
             if (isButtonEnabled) {
                 Streamus.channels.playlistsArea.commands.trigger('show:playlistsArea');
-                Streamus.channels.searchArea.commands.trigger('hide:search');
+                Streamus.channels.search.commands.trigger('hide:search');
             }
         },
 
@@ -185,7 +185,7 @@
             this.ui.hidePlaylistsAreaButton.addClass('is-hidden');
         },
 
-        _onSearchAreaShowing: function() {
+        _onSearchShowing: function() {
             this.ui.showSearchButton.addClass('is-hidden');
             this.ui.hideSearchButton.removeClass('is-hidden');
             this.ui.playlistTitleRegion.addClass('is-hidden');
@@ -194,7 +194,7 @@
             this._focusSearchInput();
         },
 
-        _onSearchAreaHiding: function() {
+        _onSearchHiding: function() {
             this.ui.showSearchButton.removeClass('is-hidden');
             this.ui.hideSearchButton.addClass('is-hidden');
             this.ui.playlistTitleRegion.removeClass('is-hidden');

diff --git a/src/js/foreground/view/behavior/itemViewMultiSelect.js b/src/js/foreground/view/behavior/itemViewMultiSelect.js
@@ -25,7 +25,7 @@
 
         initialize: function() {
             this.checkbox = new Checkbox({
-                //  TODO: I cannot access this.view.model in initialize from a behavior. https://github.com/marionettejs/backbone.marionette/issues/1579
+                // this.view.model can't be accessed in initialize from a behavior. https://github.com/marionettejs/backbone.marionette/issues/1579
                 checked: this.view.options.model.get('selected')
             });
         },
@@ -68,9 +68,7 @@
             if (!this.view.model.get('selected')) {
                 this.ui.leftContent.removeClass('is-showingCheckbox');
                 this.ui.leftContent.addClass('is-showingThumbnail');
-
-                //  TODO: Change to getChildView
-                this.view.checkbox.empty();
+                this.view.getRegion('checkbox').empty();
             }
         },
 

diff --git a/src/js/foreground/view/behavior/slidingRender.js b/src/js/foreground/view/behavior/slidingRender.js
@@ -314,7 +314,6 @@
             return isInRange;
         },
 
-        //  TODO: An animation on this would be nice.
         //  Ensure that the active item is visible by setting the container's scrollTop to a position which allows it to be seen.
         _scrollToItem: function(item) {
             var itemIndex = this.view.collection.indexOf(item);

diff --git a/src/js/foreground/view/behavior/sortable.js b/src/js/foreground/view/behavior/sortable.js
@@ -127,7 +127,6 @@
         _stop: function(event, ui) {
             var isParentNodeLost = ui.item[0].parentNode === null;
 
-            //  TODO: Check collection isImmutable instead of ListItemType.
             //  The SearchResult view is not able to be moved so disable move logic for it.
             //  If the mouse dropped the items not over the given list don't run move logic.
             var allowMove = ui.item.data('type') !== ListItemType.SearchResult && this.ui.listItems.is(':hover');
@@ -284,6 +283,7 @@
             //  If the item being sorted has been unloaded by slidingRender behavior then sortableItem will be unavailable.
             //  In this scenario, fall back to the more expensive query of getting a reference to the sortable instance via its parent's ID.
             if (_.isUndefined(sortableItem)) {
+                debugger;
                 sortableItem = $('#' + ui.item.data('parentid')).sortable('instance');
             }
 

diff --git a/src/js/foreground/view/contextMenu/contextMenuItemsView.js b/src/js/foreground/view/contextMenu/contextMenuItemsView.js
@@ -0,0 +1,12 @@
+define(function(require) {
+    'use strict';
+
+    var ContextMenuItemView = require('foreground/view/contextMenu/contextMenuItemView');
+
+    var ContextMenuItemsView = Marionette.CollectionView.extend({
+        tagName: 'ul',
+        childView: ContextMenuItemView
+    });
+
+    return ContextMenuItemsView;
+});
diff --git a/src/js/foreground/view/contextMenu/contextMenuRegion.js b/src/js/foreground/view/contextMenu/contextMenuRegion.js
@@ -35,19 +35,18 @@
         },
 
         _showContextMenu: function(top, left) {
-            //  TODO: A bug in Marionette causes this.$el.height/width to be null on first use, https://github.com/marionettejs/backbone.marionette/issues/1971.
+            //  this.$el.height/width is null on first use, https://github.com/marionettejs/backbone.marionette/issues/1971.
             var $this = $(this.el);
 
             this.contextMenu.set({
                 top: top,
-                left: left
+                left: left,
+                containerHeight: $this.height(),
+                containerWidth: $this.width()
             });
 
             this.show(new ContextMenuView({
-                collection: this.contextMenu.get('items'),
-                model: this.contextMenu,
-                containerHeight: $this.height(),
-                containerWidth: $this.width()
+                model: this.contextMenu
             }));
         },
 

diff --git a/src/js/foreground/view/contextMenu/contextMenuView.js b/src/js/foreground/view/contextMenu/contextMenuView.js
@@ -1,32 +1,27 @@
 define(function(require) {
     'use strict';
 
-    var ContextMenuItemView = require('foreground/view/contextMenu/contextMenuItemView');
     var ContextMenuTemplate = require('text!template/contextMenu/contextMenu.html');
+    var ContextMenuItemsView = require('foreground/view/contextMenu/contextMenuItemsView');
 
-    //  TODO: Refactor this to be a LayoutView with child CollectionView instead of a CompositeView.
-    var ContextMenuView = Marionette.CompositeView.extend({
+    var ContextMenuView = Marionette.LayoutView.extend({
         id: 'contextMenu',
         className: 'menu panel',
-        childView: ContextMenuItemView,
-        childViewContainer: '@ui.contextMenuItems',
         template: _.template(ContextMenuTemplate),
-        //  Used to determine whether contextMenu display should flip as to not overflow container
-        containerHeight: 0,
-        containerWidth: 0,
 
-        ui: {
-            contextMenuItems: '[data-ui~=contextMenuItems]'
+        regions: {
+            contextMenuItems: '[data-region=contextMenuItems]'
         },
 
-        initialize: function(options) {
-            this.containerHeight = options.containerHeight;
-            this.containerWidth = options.containerWidth;
+        onRender: function() {
+            this.showChildView('contextMenuItems', new ContextMenuItemsView({
+                collection: this.model.get('items')
+            }));
         },
 
         onAttach: function() {
-            var offsetTop = this._ensureOffset(this.model.get('top'), this.$el.outerHeight(), this.containerHeight);
-            var offsetLeft = this._ensureOffset(this.model.get('left'), this.$el.outerWidth(), this.containerWidth);
+            var offsetTop = this._ensureOffset(this.model.get('top'), this.$el.outerHeight(), this.model.get('containerHeight'));
+            var offsetLeft = this._ensureOffset(this.model.get('left'), this.$el.outerWidth(), this.model.get('containerWidth'));
 
             this.$el.offset({
                 top: offsetTop,
@@ -35,7 +30,7 @@
 
             this.$el.addClass('is-visible');
         },
-        //  TODO: Move this logic to the ContextMenuRegion and use the same logic as in NotificationRegion.
+
         hide: function() {
             this.$el.off('webkitTransitionEnd').one('webkitTransitionEnd', this._onTransitionOutComplete.bind(this));
             this.$el.removeClass('is-visible');

diff --git a/src/js/foreground/view/dialog/browserSettingsView.js b/src/js/foreground/view/dialog/browserSettingsView.js
@@ -52,8 +52,7 @@
                 property: propertyName
             });
 
-            //  TODO: Change to getChildView
-            this[propertyName].show(new CheckboxView({
+            this.showChildView(propertyName, new CheckboxView({
                 model: checkbox
             }));
         }

diff --git a/src/js/foreground/view/dialog/clearStreamDialogView.js b/src/js/foreground/view/dialog/clearStreamDialogView.js
@@ -5,7 +5,6 @@
     var DialogContentView = require('foreground/view/dialog/dialogContentView');
     var DialogView = require('foreground/view/dialog/dialogView');
 
-    //  TODO: Rename this to ClearStreamItems
     var ClearStreamDialogView = DialogView.extend({
         id: 'clearStreamDialog',
         streamItems: null,

diff --git a/src/js/foreground/view/dialog/createPlaylistDialogView.js b/src/js/foreground/view/dialog/createPlaylistDialogView.js
@@ -3,6 +3,7 @@
 
     var Dialog = require('foreground/model/dialog/dialog');
     var CreatePlaylistView = require('foreground/view/dialog/createPlaylistView');
+    var CreatePlaylist = require('foreground/model/dialog/createPlaylist');
     var DialogView = require('foreground/view/dialog/dialogView');
 
     var CreatePlaylistDialogView = DialogView.extend({
@@ -14,6 +15,7 @@
             });
 
             this.contentView = new CreatePlaylistView({
+                model: new CreatePlaylist(),
                 dataSourceManager: Streamus.backgroundPage.dataSourceManager,
                 playlists: Streamus.backgroundPage.signInManager.get('signedInUser').get('playlists'),
                 songs: options && options.songs ? options.songs : []