Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve update polling electron and provide a manual check for updates button #4176

Merged
merged 23 commits into from
Jun 22, 2017

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jun 3, 2017

Yet to be tested with Electron updating, don't see anything that could potentially break anything but still...

for #4173

t3chguy added 7 commits June 3, 2017 15:58
…al in Electron

Signed-off-by: Michael Telatynski <[email protected]>

(cherry picked from commit 24e8a30)
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>

(cherry picked from commit a871815)
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>

(cherry picked from commit 104c804)
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>

(cherry picked from commit d878c72)
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>
don't break when running a non Squirrel Windows build that has an update url

Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy t3chguy force-pushed the t3chguy/updating_stuff branch from b330968 to 93f148f Compare June 3, 2017 15:34
@ara4n
Copy link
Member

ara4n commented Jun 6, 2017

having an extra status bar for this feels a bit weird...

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments. Looks good I think but obviously I'm super paranoid about breaking auto-update so will test carefully.

@@ -120,6 +120,7 @@ process.on('uncaughtException', function(error) {
});

electron.ipcMain.on('install_update', installUpdate);
electron.ipcMain.on('checkForUpdates', pollForUpdates);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably keep the case consistent on these

render: function() {
let image;
if (this.state.done) {
image = <img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="/!\"/>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a double '' as this is an escaped quote mark (or possibly it should just be 'warning' which is better on screen readers)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it is both in MatrixToolbar and NewVersionBar right now, will change both to match PasswordNagBar (Warning)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t3chguy@DESKTOP-784D1OM:/mnt/c/Users/t3chg/WebstormProjects/riot-web/src$ grep -R mx_MatrixToolbar_warning
components/views/globals/MatrixToolbar.js:                <img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="/!\"/>
components/views/globals/NewVersionBar.js:                <img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="/!\"/>

},

componentWillMount: function() {
PlatformPeg.get().checkForUpdate().done((state) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not hugely keen on componentWillMount having side effects - I'd prefer if the cause & effect was the other way around, ie. firing off an update check causes the bar to appear (if it was a user-triggered one)

@t3chguy
Copy link
Member Author

t3chguy commented Jun 11, 2017

moved electron update logic out to updater.js in order to try and tidy electron-main.js

because _t called with undefined interpolation name: errorDetail
even though when its undef its not used to sprinf-js would have been fine...

Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy
Copy link
Member Author

t3chguy commented Jun 12, 2017

Tested with WebPlatform
Remains to be tested with ElectronPlatform
...edit...
Tested with Squirrel.Windows
works fine, but hell downloading those updates takes longer than I thought, a minute almost...

@t3chguy t3chguy force-pushed the t3chguy/updating_stuff branch 2 times, most recently from 388a282 to b6d2ba2 Compare June 12, 2017 13:46
@@ -233,7 +172,7 @@ electron.app.on('ready', () => {

if (vectorConfig.update_base_url) {
console.log(`Starting auto update with base URL: ${vectorConfig.update_base_url}`);
startAutoUpdate(vectorConfig.update_base_url);
updater.start(vectorConfig.update_base_url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed semicolon

},

render: function() {
const message = _t(statusText[this.props.status], { errorDetail: this.props.detail || '' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we prefer _t on literals wherever possible, so getStatusText() which returns the result of _t instead of the object literal

t3chguy added 4 commits June 20, 2017 14:32
…ector-im/riot-web into t3chguy/updating_stuff

Signed-off-by: Michael Telatynski <[email protected]>

# Conflicts:
#	electron_app/src/tray.js
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on windows & mac, seems to work nicely!

@dbkr dbkr merged commit fab50bc into develop Jun 22, 2017
@t3chguy t3chguy deleted the t3chguy/updating_stuff branch October 29, 2017 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants