From 9747d2851411be6105d708a58db761b6ebc535ef Mon Sep 17 00:00:00 2001 From: Feross Aboukhadijeh Date: Thu, 19 May 2016 18:53:53 -0700 Subject: [PATCH 1/7] Fix bugs in LocationHistory - Handles more than 2 pages in the history robustly now! - When self._pending is true, all navigations are ignored. - No more bug with back() being called twice too quickly. - Remove "leaky abstraction" methods like clearPending() and pending() - Add backToFirst() that properly unloads each page as it goes back to the first one. - Enhance clearForward() to support removing a specific page from the forward stack, instead of nuking the whole thing. --- renderer/lib/location-history.js | 143 +++++++++++++++++++------------ 1 file changed, 90 insertions(+), 53 deletions(-) diff --git a/renderer/lib/location-history.js b/renderer/lib/location-history.js index 96efdf0b..fbbaf7aa 100644 --- a/renderer/lib/location-history.js +++ b/renderer/lib/location-history.js @@ -4,81 +4,118 @@ function LocationHistory () { if (!new.target) return new LocationHistory() this._history = [] this._forward = [] - this._pending = null + this._pending = false +} +LocationHistory.prototype.current = function () { + return this._history[this._history.length - 1] } LocationHistory.prototype.go = function (page, cb) { + if (!cb) cb = noop + if (this._pending) return cb(null) + console.log('go', page) + this.clearForward() this._go(page, cb) } -LocationHistory.prototype._go = function (page, cb) { - if (this._pending) return - if (page.onbeforeload) { - this._pending = page - page.onbeforeload((err) => { - if (this._pending !== page) return /* navigation was cancelled */ - this._pending = null - if (err) { - if (cb) cb(err) - return - } - this._history.push(page) - if (cb) cb() - }) - } else { - this._history.push(page) - if (cb) cb() - } -} - LocationHistory.prototype.back = function (cb) { - if (this._history.length <= 1) return + var self = this + if (!cb) cb = noop + if (self._history.length <= 1 || self._pending) return cb(null) - var page = this._history.pop() + var page = self._history.pop() + self._unload(page, done) - if (page.onbeforeunload) { - // TODO: this is buggy. If the user clicks back twice, then those pages - // may end up in _forward in the wrong order depending on which onbeforeunload - // call finishes first. - page.onbeforeunload(() => { - this._forward.push(page) - if (cb) cb() - }) - } else { - this._forward.push(page) - if (cb) cb() + function done (err) { + if (err) return cb(err) + self._forward.push(page) + self._load(self.current(), cb) } } -LocationHistory.prototype.forward = function (cb) { - if (this._forward.length === 0) return - - var page = this._forward.pop() - this._go(page, cb) -} - -LocationHistory.prototype.clearForward = function () { - this._forward = [] -} - -LocationHistory.prototype.current = function () { - return this._history[this._history.length - 1] -} - LocationHistory.prototype.hasBack = function () { return this._history.length > 1 } +LocationHistory.prototype.forward = function (cb) { + if (!cb) cb = noop + if (this._forward.length === 0 || this._pending) return cb(null) + + var page = this._forward.pop() + this._go(page, cb) +} + LocationHistory.prototype.hasForward = function () { return this._forward.length > 0 } -LocationHistory.prototype.pending = function () { - return this._pending +LocationHistory.prototype.clearForward = function (url) { + if (url == null) { + this._forward = [] + } else { + console.log(this._forward) + console.log(url) + this._forward = this._forward.filter(function (page) { + return page.url !== url + }) + } } -LocationHistory.prototype.clearPending = function () { - this._pending = null +LocationHistory.prototype.backToFirst = function (cb) { + var self = this + if (!cb) cb = noop + if (self._history.length <= 1) return cb(null) + + self.back(function (err) { + if (err) return cb(err) + self.backToFirst(cb) + }) } + +LocationHistory.prototype._go = function (page, cb) { + var self = this + if (!cb) cb = noop + + self._unload(self.current(), done1) + + function done1 (err) { + if (err) return cb(err) + self._load(page, done2) + } + + function done2 (err) { + if (err) return cb(err) + self._history.push(page) + cb(null) + } +} + +LocationHistory.prototype._load = function (page, cb) { + var self = this + self._pending = true + + if (page && page.onbeforeload) page.onbeforeload(done) + else done(null) + + function done (err) { + self._pending = false + cb(err) + } +} + +LocationHistory.prototype._unload = function (page, cb) { + var self = this + self._pending = true + + if (page && page.onbeforeunload) page.onbeforeunload(done) + else done(null) + + function done (err) { + self._pending = false + cb(err) + } +} + +function noop () {} From 60a8969abc7d8b2fbbdad64bcd722e580f3b43f1 Mon Sep 17 00:00:00 2001 From: Feross Aboukhadijeh Date: Thu, 19 May 2016 18:54:44 -0700 Subject: [PATCH 2/7] Add location.url() shorthand location.url() === location.current().url --- renderer/index.js | 6 +++--- renderer/lib/location-history.js | 5 +++++ renderer/views/app.js | 8 ++++---- renderer/views/header.js | 2 +- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/renderer/index.js b/renderer/index.js index 601bc0a7..67a367c2 100644 --- a/renderer/index.js +++ b/renderer/index.js @@ -306,7 +306,7 @@ function dispatch (action, ...args) { state.playing.isStalled = true } if (action === 'mediaError') { - if (state.location.current().url === 'player') { + if (state.location.url() === 'player') { state.playing.location = 'error' ipcRenderer.send('checkForVLC') ipcRenderer.once('checkForVLC', function (e, isInstalled) { @@ -386,7 +386,7 @@ function pause () { } function playPause () { - if (state.location.current().url !== 'player') return + if (state.location.url() !== 'player') return if (state.playing.isPaused) { play() } else { @@ -1236,7 +1236,7 @@ function showDoneNotification (torrent) { // * The video is paused // * The video is playing remotely on Chromecast or Airplay function showOrHidePlayerControls () { - var hideControls = state.location.current().url === 'player' && + var hideControls = state.location.url() === 'player' && state.playing.mouseStationarySince !== 0 && new Date().getTime() - state.playing.mouseStationarySince > 2000 && !state.playing.isPaused && diff --git a/renderer/lib/location-history.js b/renderer/lib/location-history.js index fbbaf7aa..2c5d88f3 100644 --- a/renderer/lib/location-history.js +++ b/renderer/lib/location-history.js @@ -6,6 +6,11 @@ function LocationHistory () { this._forward = [] this._pending = false } + +LocationHistory.prototype.url = function () { + return this.current() && this.current().url +} + LocationHistory.prototype.current = function () { return this._history[this._history.length - 1] } diff --git a/renderer/views/app.js b/renderer/views/app.js index a834c15b..863fcc60 100644 --- a/renderer/views/app.js +++ b/renderer/views/app.js @@ -22,7 +22,7 @@ function App (state) { // * The mouse is over the controls or we're scrubbing (see CSS) // * The video is paused // * The video is playing remotely on Chromecast or Airplay - var hideControls = state.location.current().url === 'player' && + var hideControls = state.location.url() === 'player' && state.playing.mouseStationarySince !== 0 && new Date().getTime() - state.playing.mouseStationarySince > 2000 && !state.playing.isPaused && @@ -30,10 +30,10 @@ function App (state) { // Hide the header on Windows/Linux when in the player // On OSX, the header appears as part of the title bar - var hideHeader = process.platform !== 'darwin' && state.location.current().url === 'player' + var hideHeader = process.platform !== 'darwin' && state.location.url() === 'player' var cls = [ - 'view-' + state.location.current().url, /* e.g. view-home, view-player */ + 'view-' + state.location.url(), /* e.g. view-home, view-player */ 'is-' + process.platform /* e.g. is-darwin, is-win32, is-linux */ ] if (state.window.isFullScreen) cls.push('is-fullscreen') @@ -81,6 +81,6 @@ function getModal (state) { } function getView (state) { - var url = state.location.current().url + var url = state.location.url() return Views[url](state) } diff --git a/renderer/views/header.js b/renderer/views/header.js index dc4c09dd..49ff6e3f 100644 --- a/renderer/views/header.js +++ b/renderer/views/header.js @@ -37,7 +37,7 @@ function Header (state) { } function getAddButton () { - if (state.location.current().url !== 'player') { + if (state.location.url() !== 'player') { return hx` Date: Thu, 19 May 2016 18:55:06 -0700 Subject: [PATCH 3/7] Cancel button on create torrent page should only go back one page --- renderer/views/create-torrent-page.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/renderer/views/create-torrent-page.js b/renderer/views/create-torrent-page.js index d1213d38..7b8663c0 100644 --- a/renderer/views/create-torrent-page.js +++ b/renderer/views/create-torrent-page.js @@ -123,7 +123,7 @@ function CreateTorrentPage (state) { } function handleCancel () { - dispatch('backToList') + dispatch('back') } function handleToggleShowAdvanced () { From 8f39f8a23ee2d9be7aba78dcaa5dabde8d6d6065 Mon Sep 17 00:00:00 2001 From: Feross Aboukhadijeh Date: Thu, 19 May 2016 18:55:49 -0700 Subject: [PATCH 4/7] After creating torrent, remove create torrent page from forward stack --- renderer/index.js | 3 +++ renderer/views/create-torrent-page.js | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/renderer/index.js b/renderer/index.js index 67a367c2..d1f2b3ee 100644 --- a/renderer/index.js +++ b/renderer/index.js @@ -807,6 +807,9 @@ function findFilesRecursive (fileOrFolder, cb) { function createTorrent (options) { var torrentKey = state.nextTorrentKey++ ipcRenderer.send('wt-create-torrent', torrentKey, options) + state.location.backToFirst(function () { + state.location.clearForward('create-torrent') + }) } function torrentInfoHash (torrentKey, infoHash) { diff --git a/renderer/views/create-torrent-page.js b/renderer/views/create-torrent-page.js index 7b8663c0..82fe517e 100644 --- a/renderer/views/create-torrent-page.js +++ b/renderer/views/create-torrent-page.js @@ -119,7 +119,6 @@ function CreateTorrentPage (state) { comment: comment } dispatch('createTorrent', options) - dispatch('backToList') } function handleCancel () { From 264c035ef7d512d0c5fad0dcbbd9927a4958108a Mon Sep 17 00:00:00 2001 From: Feross Aboukhadijeh Date: Thu, 19 May 2016 18:56:10 -0700 Subject: [PATCH 5/7] After deleting torrent, remove just the player from forward stack --- renderer/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/renderer/index.js b/renderer/index.js index d1f2b3ee..b3cde3df 100644 --- a/renderer/index.js +++ b/renderer/index.js @@ -1091,7 +1091,7 @@ function deleteTorrent (infoHash) { var index = state.saved.torrents.findIndex((x) => x.infoHash === infoHash) if (index > -1) state.saved.torrents.splice(index, 1) saveStateThrottled() - state.location.clearForward() // prevent user from going forward to a deleted torrent + state.location.clearForward('player') // prevent user from going forward to a deleted torrent sound.play('DELETE') } From 5dca89b61cf070f78a16e8636ff1dc6f56fee434 Mon Sep 17 00:00:00 2001 From: Feross Aboukhadijeh Date: Thu, 19 May 2016 18:56:41 -0700 Subject: [PATCH 6/7] When player is active, and magnet link is pasted, go back to list --- renderer/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/renderer/index.js b/renderer/index.js index b3cde3df..a30ba77d 100644 --- a/renderer/index.js +++ b/renderer/index.js @@ -213,7 +213,6 @@ function dispatch (action, ...args) { onOpen(args[0] /* files */) } if (action === 'addTorrent') { - backToList() addTorrent(args[0] /* torrent */) } if (action === 'showOpenTorrentFile') { @@ -620,6 +619,7 @@ function getTorrentSummary (torrentKey) { // Adds a torrent to the list, starts downloading/seeding. TorrentID can be a // magnet URI, infohash, or torrent file: https://github.com/feross/webtorrent#clientaddtorrentid-opts-function-ontorrent-torrent- function addTorrent (torrentId) { + backToList() var torrentKey = state.nextTorrentKey++ var path = state.saved.downloadPath if (torrentId.path) { From 3a81799828a74a71931a1193ac54661952dcfbd5 Mon Sep 17 00:00:00 2001 From: Feross Aboukhadijeh Date: Thu, 19 May 2016 19:03:47 -0700 Subject: [PATCH 7/7] Unify onOpen and onDrag, and support more cases I don't think it matters whether the open comes from onOpen (opening magnet, .torrent file, dragging file to dock, menu item) or from dragging to the window. These should use the same code path. The only relevant information is the page of the app that we're on. This change unifies the two methods, and supports dragging .torrent files or creating a torrent when the player is active, if the dragged files are not .srt or .vtt. We go back to the list, or to the create torrent page in these situations, so it's not confusing for the user. Always close open modals when handling an open. --- renderer/index.js | 62 ++++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/renderer/index.js b/renderer/index.js index a30ba77d..844787be 100644 --- a/renderer/index.js +++ b/renderer/index.js @@ -87,7 +87,7 @@ function init () { // OS integrations: // ...drag and drop a torrent or video file to play or seed - dragDrop('body', onDrag) + dragDrop('body', onOpen) // ...same thing if you paste a torrent document.addEventListener('paste', onPaste) @@ -273,15 +273,14 @@ function dispatch (action, ...args) { playPause() } if (action === 'play') { - if (state.location.pending()) return state.location.go({ url: 'player', onbeforeload: function (cb) { + play() openPlayer(args[0] /* infoHash */, args[1] /* index */, cb) }, onbeforeunload: closePlayer }) - play() } if (action === 'playbackJump') { jumpToTime(args[0] /* seconds */) @@ -431,18 +430,18 @@ function openSubtitles () { function backToList () { // Exit any modals and screens with a back button state.modal = null - while (state.location.hasBack()) state.location.back() + state.location.backToFirst(function () { + // If we were already on the torrent list, scroll to the top + var contentTag = document.querySelector('.content') + if (contentTag) contentTag.scrollTop = 0 - // If we were already on the torrent list, scroll to the top - var contentTag = document.querySelector('.content') - if (contentTag) contentTag.scrollTop = 0 - - // Work around virtual-dom issue: it doesn't expose its redraw function, - // and only redraws on requestAnimationFrame(). That means when the user - // closes the window (hide window / minimize to tray) and we want to pause - // the video, we update the vdom but it keeps playing until you reopen! - var mediaTag = document.querySelector('video,audio') - if (mediaTag) mediaTag.pause() + // Work around virtual-dom issue: it doesn't expose its redraw function, + // and only redraws on requestAnimationFrame(). That means when the user + // closes the window (hide window / minimize to tray) and we want to pause + // the video, we update the vdom but it keeps playing until you reopen! + var mediaTag = document.querySelector('video,audio') + if (mediaTag) mediaTag.pause() + }) } // Checks whether we are connected and already casting @@ -555,41 +554,29 @@ function saveState () { update() } -// Called when the user clicks a magnet link or torrent, or uses the Open dialog +// Called when the user drag-drops files onto the app function onOpen (files) { if (!Array.isArray(files)) files = [ files ] - // Return to the home screen - backToList() - - if (files.every(isTorrent)) { - // All .torrent files? Start downloading - files.forEach(addTorrent) - } else { - // Show the Create Torrent screen. Let's seed those files. - showCreateTorrent(files) + if (state.modal) { + state.modal = null } -} -// Called when the user drag-drops files onto the app -function onDrag (files) { - if (!Array.isArray(files)) files = [ files ] + var subtitles = files.filter(isSubtitle) - var isInPlayer = state.location.current().url === 'player' - var isHome = state.location.current().url === 'home' && !state.modal - - if (isInPlayer) { - // In the player, the only drag-drop function is adding subtitles - addSubtitles(files.filter(isSubtitle), true) - } else if (isHome) { - // Otherwise, you can only drag-drop onto the home screen + if (state.location.url() === 'home' || subtitles.length === 0) { if (files.every(isTorrent)) { - // All .torrent files? Start downloading + if (state.location.url() !== 'home') { + backToList() + } + // All .torrent files? Add them. files.forEach(addTorrent) } else { // Show the Create Torrent screen. Let's seed those files. showCreateTorrent(files) } + } else if (state.location.url() === 'player') { + addSubtitles(subtitles, true) } update() @@ -757,7 +744,6 @@ function startTorrentingSummary (torrentSummary) { // Shows the Create Torrent page with options to seed a given file or folder function showCreateTorrent (files) { if (Array.isArray(files)) { - if (state.location.pending() || state.location.current().url !== 'home') return state.location.go({ url: 'create-torrent', files: files