P2P updater #473

team-pct · 2016-11-08T13:47:38Z

##P2P updater

Changes proposed in this pull request:

Update Updater.js
Add UpdateSeed on Settings
Add Webtorrent on package.json

I've read the Contributor License Agreement

P2P updater

* P2P updater * Update Updater.js * Add UpdateSeed on Settings * Add Webtorrent on package.json

xaiki

THANK YOU, for a very interesting PR.

i'm not sure why we change the way the update is installed once finished, i'd like this to be optional for now, and before we merge we need to setup the butter infrastructure for this.

xaiki · 2016-11-08T13:50:40Z

src/app/global.js

@@ -57,6 +57,6 @@ var

    // Torrent engines
    peerflix = require('peerflix'),
-
+    WebTorrent = require('webtorrent'),


we don't need this as a global, please keep it in updater.js

We will need it with the new streamer running on Webtorrent Please see #424 #470

we can change that when it gets merged.

xaiki · 2016-11-08T13:51:27Z

src/app/settings.js

@@ -149,6 +149,7 @@ Settings.tmpLocation = path.join(os.tmpDir(), Settings.projectName);
 Settings.databaseLocation = path.join(data_path, 'data');
 Settings.deleteTmpOnClose = true;
 Settings.automaticUpdating = true;
+Settings.UpdateSeed = true;


either Settings.update = {seed: true}, or Settings.seedUpdate

Settings.seedUpdate is a parameter on setting container ,
People can disable seeding if they are on the last version

it's just a name nitpick 'seedUpdate' isn't really clear to me.

how windowsy of you

naming convention is bad, you should use seedUpdate or redistributeUpdater, but don't start with an uppercase :)

team-pct · 2016-11-08T13:59:19Z

I am not sure what you mean by changing the updater behavior but we just included web torrent and kept the Flow of the current updater

xaiki · 2016-11-10T12:57:12Z

your PR changed a lot of things (more than can be reviewed) in updater.js, let's talk on IRC/Slack and find a way to make this mergeable.

vankasteelj

Some remarks

vankasteelj · 2016-11-12T18:36:09Z

src/app/settings.js

@@ -149,6 +149,7 @@ Settings.tmpLocation = path.join(os.tmpDir(), Settings.projectName);
 Settings.databaseLocation = path.join(data_path, 'data');
 Settings.deleteTmpOnClose = true;
 Settings.automaticUpdating = true;
+Settings.UpdateSeed = true;


naming convention is bad, you should use seedUpdate or redistributeUpdater, but don't start with an uppercase :)

vankasteelj · 2016-11-12T18:36:30Z

package.json

@@ -60,6 +60,7 @@
        "nodecast-js": "^1.0.1",
        "opensubtitles-api": "^3.1.0",
        "peerflix": "^0.35.0",
+        "webtorrent": "0.97.2",


better use 0.x.x or ^0.97.2

vankasteelj · 2016-11-12T18:38:26Z

src/app/templates/settings-container.tpl

-                <input class="settings-checkbox" name="events" id="cb6" type="checkbox" <%=(Settings.events? "checked='checked'":"")%>>
-                <label class="settings-label" for="cb6"><%= i18n.__("Celebrate various events") %></label>
+                <input class="settings-checkbox" name="UpdateSeed" id="cb6" type="checkbox" <%=(Settings.UpdateSeed? "checked='checked'":"")%>>
+                <label class="settings-label" for="cb6"><%= i18n.__("Activate Update Seeding") %></label>


It's personnal and doesn't really impair the merge of this, but I don't like the string. For starter, you don't need the word "activate" in it, the checkbox says that already.
How about:

"Seed update files to other users"

"Redistribute the update packages"

vankasteelj · 2016-11-12T18:39:05Z

src/app/updater.js

@@ -3,6 +3,8 @@

    var CHANNELS = ['stable', 'beta', 'nightly'],
        FILENAME = 'package.nw.new',
+        WebTorrent = require('webtorrent'),
+        client = new WebTorrent({dht: true,maxConns: '5'}),


Is the maxConns needed? It fallsback to 55, why forcing a lower number?

vankasteelj · 2016-11-12T18:40:46Z

src/app/updater.js

+                        win.debug('ERROR' + err.message);
+                    });
+                    torrent.on('done', function () {
+                        win.debug('Seeding the Current Update!');


Once 'done', it doesn't seed, according to my tests. You need to explicitly set it with the .seed() api call (I think?)

so it looks like seed() is to create a new torrent, i'm not sure how seed control works...

vankasteelj · 2016-11-12T18:42:54Z

src/app/updater.js

+        }, function (torrent) {
+            win.debug('Downloading update... Please allow a few minutes');
+            torrent.on('error', function (err) {
+                win.debug('ERROR' + err.message);


Shouldn't it be win.warn('Downloading update failed:', error) ? Make it visible but not a 'breaking' error, and also comprehensible.

vankasteelj · 2016-11-12T18:44:56Z

src/app/updater.js

+            });
+            torrent.on('done', function () {
+                win.debug('Update downloaded!');
+                defer.resolve(path.join(outputDir, torrent.name));


torrent.name contains the extension of the file in this case? Because there's a risk it's a directory

we create the update torrent.

xaiki · 2020-11-01T11:13:03Z

closing as #545 rebases it

team-pct added 2 commits November 8, 2016 14:45

P2p updater (#1)

d20ea51

* P2P updater * Update Updater.js * Add UpdateSeed on Settings * Add Webtorrent on package.json

Add Webtorrent in Global var

689b1af

xaiki requested changes Nov 8, 2016

View reviewed changes

team-pct closed this Nov 8, 2016

xaiki reopened this Nov 10, 2016

team-pct added 2 commits November 10, 2016 15:28

Update global.js

0c48559

Update updater.js

4a0583a

vankasteelj reviewed Nov 12, 2016

View reviewed changes

xaiki mentioned this pull request Dec 18, 2016

Xaiki/p2p update rebase #545

Open

xaiki closed this Nov 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P2P updater #473

P2P updater #473

team-pct commented Nov 8, 2016

xaiki left a comment

xaiki Nov 8, 2016

team-pct Nov 8, 2016

xaiki Nov 8, 2016

xaiki Nov 8, 2016

team-pct Nov 8, 2016

xaiki Nov 8, 2016

vankasteelj Nov 9, 2016

vankasteelj Nov 12, 2016

team-pct commented Nov 8, 2016

xaiki commented Nov 10, 2016

vankasteelj left a comment

vankasteelj Nov 12, 2016

vankasteelj Nov 12, 2016

vankasteelj Nov 12, 2016

vankasteelj Nov 12, 2016

vankasteelj Nov 12, 2016

xaiki Dec 18, 2016

vankasteelj Nov 12, 2016

vankasteelj Nov 12, 2016

xaiki Nov 13, 2016

xaiki commented Nov 1, 2020

P2P updater #473

P2P updater #473

Conversation

team-pct commented Nov 8, 2016

xaiki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

team-pct commented Nov 8, 2016

xaiki commented Nov 10, 2016

vankasteelj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xaiki commented Nov 1, 2020