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.

Sign up for GitHub

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

Jump to bottom

Remember and Recall window layout/position state #3622

Merged
merged 9 commits into from
May 5, 2017
Merged

Remember and Recall window layout/position state #3622

merged 9 commits into from
May 5, 2017

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Apr 12, 2017

for #2959

Signed-off-by: Michael Telatynski 7t3chguy@gmail.com

@t3chguy
Remember and Recall window layout so that position+size persist
76008e8 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy changed the title Remember and Recall window layout so that position+size persist Remember and Recall window layout/position state Apr 12, 2017
@t3chguy
Fix packaged functionality
3fb5402 
Improve dependency management for Electron main process deps

Dependencies in /electron/package.json will be installed through a script in /package.json and will be bundled via electron-builder

Does not affect standard webapp whatsoever

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
ara4n
ara4n reviewed Apr 22, 2017
View reviewed changes
package.json
@@ -158,10 +160,10 @@
},
"win": {
"target": "squirrel"
},
"directories": {
Copy link
Member

Choose a reason for hiding this comment

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

hm, how come directories is moved up here?

Copy link
Member Author

Choose a reason for hiding this comment

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

C:\Users\t3chg\WebStormProjects\riot-web>node_modules\.bin\build
⚠️  "directories" in the root is deprecated, please specify in the "build"

ara4n
ara4n reviewed Apr 22, 2017
View reviewed changes
package.json Outdated
@@ -36,6 +36,7 @@
"build": "node scripts/babelcheck.js && npm run build:res && npm run build:bundle",
"build:dev": "node scripts/babelcheck.js && npm run build:res && npm run build:bundle:dev",
"dist": "scripts/package.sh",
"postinstall": "cd electron && npm i",
Copy link
Member

Choose a reason for hiding this comment

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

this looks weird - we surely don't want to try to install all the electron crap every time someone runs npm i on riot-web?

Copy link
Member Author

@t3chguy t3chguy Apr 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Currently npm i is only useful because it triggers prepublish, in a later version of npm this functionality will no longer work as prepublish will only be hit on npm publish. I guess the better alternative to installing the electron deps here is to define scripts for electron-builder which run npm i on the electron deps first.

@ara4n
Copy link
Member

ara4n commented Apr 22, 2017

looks plausible other than two questions - over to you. thanks!

@t3chguy t3chguy mentioned this pull request Apr 22, 2017
Closed
@ara4n
Copy link
Member

ara4n commented Apr 22, 2017 via email

@t3chguy
don't hook on postinstall, current dev cycle uses (abuses) npm i to b…
9e2324f 
…uild

so no point making it take longer than it has to for the devs only testing
webapp

build:electron - hook npm run install:electron
install:electron - run npm i for the electron deps
electron - start the app locally for testing

add to README

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Member Author

t3chguy commented Apr 27, 2017

Back to you :)

ara4n
ara4n reviewed Apr 27, 2017
View reviewed changes
README.md Outdated
@@ -135,6 +135,7 @@ To run as a desktop app:

```
npm install electron
npm run install:electron
node_modules/.bin/electron .
Copy link
Member

Choose a reason for hiding this comment

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

this feels fiddly and likely for folks to screw up. can't we replace all 3 lines with just npm run electron?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why electron isn't a devDependency? That'd get rid of line #1
the last two could be replaced with npm run electron which has already been defined

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel wrong putting npm i electron into an npm script, just feels really redundant, but if the majority agree then thats obviously the way to go

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like the correct way of doing this is with the electron-builder install-app-deps in postinstall (https://github.com/electron-userland/electron-builder/wiki/Two-package.json-Structure) (assuming the 2 package.json structure is what you were going for here)

Copy link
Member Author

Choose a reason for hiding this comment

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

@dbkr Matthew had a complaint when I did had linked installing these deps to postinstall: #3622 (comment)

@ara4n
Copy link
Member

ara4n commented Apr 27, 2017

@dbkr this looks okay to me, but i think i'd like a 2nd opinion as it's more your domain

@ara4n
Copy link
Member

ara4n commented Apr 27, 2017

@t3chguy there's one new comment here.

@ara4n ara4n assigned dbkr and unassigned t3chguy Apr 27, 2017
dbkr
dbkr reviewed Apr 27, 2017
View reviewed changes
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.

I was expecting this to be using the electron builder 2 package.json format, but you haven't specified the app directory (and it's not the default 'app' so it doesn't look like this is what's happening, which is surprising. Is there a reason for this?

README.md Outdated
@@ -135,6 +135,7 @@ To run as a desktop app:

```
npm install electron
npm run install:electron
node_modules/.bin/electron .
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like the correct way of doing this is with the electron-builder install-app-deps in postinstall (https://github.com/electron-userland/electron-builder/wiki/Two-package.json-Structure) (assuming the 2 package.json structure is what you were going for here)

@dbkr dbkr assigned t3chguy and unassigned dbkr Apr 27, 2017
@t3chguy
Proper two package.json format, release.sh bumps 2nd json ver too
f547082 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Change shebang to bash for string manipulations
8a50207 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Only install electron deps when we're bundling/running electron
499b04c 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Merge package.json, match version in electron/package.json
a09f890 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Update README with npm run electron
1ed1080 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
no point installing deps here, builder will do it for us
309afdb 
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
dbkr
dbkr approved these changes May 5, 2017
View reviewed changes
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.

OK, I think this looks good - let's give it a go.

@dbkr dbkr merged commit 2c36506 into element-hq:develop May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ara4n ara4n ara4n left review comments

@dbkr dbkr dbkr approved these changes

Assignees

@t3chguy t3chguy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t3chguy @ara4n @dbkr