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

Custom app names, exec paths, etc. #37

Merged
merged 16 commits into from
Aug 27, 2014
Merged

Custom app names, exec paths, etc. #37

merged 16 commits into from
Aug 27, 2014

Conversation

adam-lynch
Copy link
Collaborator

Have a read through the updated readme to see how everything has changed.

The tests seem to be fine on Windows & Mac. I'm currently using this branch as my dependency in my own app (on Windows) and I haven't gotten it 100% correct (it's probably my own code, the final app isn't being opened).

@edjafarov could you test this for me please? It would be great if you could check that the unpacking of Mac & Linux packages are the same as Windows.

E.g. if you have a x.zip (stored wherever you downloaded it) then it'll get unpacked into <tmp>/x. So if there's a file a.js at the root, then it'll be at <tmp>/x/a.js.

Also it should be assumed that your app is based on manifest.name and at the root of your package, e.g. after unpackaging it would be located at <tmp>/<packageBaseName>/<manifest.name>.app, <tmp>/<packageBaseName>/<manifest.name>.exe or <tmp>/<packageBaseName>/<manifest.name> (linux), unless you manually override it.

Related: #23, #25, #35,

@adam-lynch
Copy link
Collaborator Author

Ok, great, I can confirm this works perfectly for my needs (on Windows). My problem was that I was quicking the intermediary app too quickly.

In the end, the code is:

gui.Shell.openItem execPath
setTimeout gui.App.quit, 1000

(execPath is gui.App.argv[1])

We're just using gui.Shell.openItem because it's simpler & we don't need to pass arguments to the final app.

@edjafarov
Copy link
Collaborator

Awesome finding! Could you also change exec to gui.Shell.openItem in the test app. I guess that would help everyone.
You rock 🤘!

@adam-lynch
Copy link
Collaborator Author

But that means replacing updater.run with it?

Thanks haha 😄

@edjafarov
Copy link
Collaborator

But that means replacing updater.run with it?

yeah:)

you want me to merge? Or you are not finished yet?

@adam-lynch
Copy link
Collaborator Author

Could you please test what I said (at the top of the PR)? I think it mightn't be right, but if it is, then it's ready to merge

@edjafarov
Copy link
Collaborator

I will test it and merge tomorrow. It's too late right now:)

@adam-lynch
Copy link
Collaborator Author

Thanks 👍

@adam-lynch
Copy link
Collaborator Author

But that means replacing updater.run with it?

yeah:)

So does this mean we should remove updater.run from the API?

@edjafarov
Copy link
Collaborator

I am not sure - the thing is that for linux for example extracted files lose their executable state. Maybe we will beat zip stuff eventually. Until then we need to do things before running stuff.

@adam-lynch
Copy link
Collaborator Author

I am not sure - the thing is that for linux for example extracted files lose their executable state. Maybe we will beat zip stuff eventually. Until then we need to do things before running stuff.

I don't understand sorry.

To be clear, I'm saying maybe we should just remove updater.run from the API and advocate gui.Shell.open instead. I'm not saying remove updater.runInstaller or even pRun.

If you want I could keep updater.run and just make it call gui.Shell.open underneath (but then it wouldn't accept any arguments anymore).

@edjafarov
Copy link
Collaborator

I don't understand sorry.

this https://github.com/edjafarov/node-webkit-updater/blob/master/app/updater.js#L281 line here.

when files are extracted they kinda text files. To make em executables that you can run you need to chmod them.

so there is stuff that is required to be done before run. So yes:

keep updater.run and just make it call gui.Shell.open underneath (but then it wouldn't accept any arguments anymore).

right now I am trying to make it work for ubuntu - for some reason it is not running so that is what I am working now.

@adam-lynch
Copy link
Collaborator Author

Ok thanks. So maybe in the end we'll still have updater.run but for Mac & Windows it'll just do gui.Shell.app.

@edjafarov
Copy link
Collaborator

gui.Shell.openItem is not working - it just opens stuff with current app assigned to this type

error opening location: No application is registered as handling this file

We would need to stick with exec thus I need to investigate why this stuff is dying in ubuntu.
BWT if you don't care of ubuntu - we can merge it. I will create a bug and will fix it separately.

@adam-lynch
Copy link
Collaborator Author

In my own use case, I only care about Windows because my app will be updated through the Mac app store. But I can change it to use gui.Shell.openItem for everything but Mac (only for updater.run) and then it'll still work for Ubuntu too.

@edjafarov
Copy link
Collaborator

it works on windows?

@adam-lynch
Copy link
Collaborator Author

Yep.

@adam-lynch
Copy link
Collaborator Author

It works on Mac too. Like I tested if gui.Shell.open opened _.apps too

@edjafarov
Copy link
Collaborator

Ok, since run implementations are OS dependent do it for win and mac without API change.
that would be enough.

@adam-lynch
Copy link
Collaborator Author

Ok, but I'll have to add a note that the args and options parameters aren't used for Mac & Windows.

…ater into custom-names

Conflicts:
	README.md
	app/package.json
	app/updater.js
	package.json
…ater into custom-names

Conflicts:
	README.md
	app/package.json
	app/updater.js
	package.json
@adam-lynch
Copy link
Collaborator Author

Ok, that's done. I've merged in those changes here (including the new checkNewVersion) into this branch. So I'm merging now.

@adam-lynch adam-lynch changed the title DON'T MERGE YET: Custom app names, exec paths, etc. Custom app names, exec paths, etc. Aug 27, 2014
adam-lynch added a commit that referenced this pull request Aug 27, 2014
Custom app names, exec paths, etc.
@adam-lynch adam-lynch merged commit ec9eaf2 into master Aug 27, 2014
@adam-lynch adam-lynch deleted the custom-names branch August 27, 2014 21:20
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.

2 participants