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

Hide on autostart #230

Merged
merged 1 commit into from
Sep 13, 2016
Merged

Hide on autostart #230

merged 1 commit into from
Sep 13, 2016

Conversation

razzeee
Copy link
Contributor

@razzeee razzeee commented Aug 2, 2016

This should hopefully address #228
Untested so far, will grab the artifacts from ci, as soon as they are up and test those.

Let's hope Teamwork/node-auto-launch#31 is solved.

@razzeee
Copy link
Contributor Author

razzeee commented Aug 2, 2016

Doesn't seem to work for me on windows 10.
Can other platforms please try?
https://circleci.com/gh/mattermost/desktop/539#artifacts

@razzeee
Copy link
Contributor Author

razzeee commented Aug 3, 2016

Also doesn't work on win7

@jasonblais
Copy link
Contributor

Any help from the community here?

@razzeee Teamwork/node-auto-launch#31 seems to reference isHidden function. Would that be of any use?

@razzeee
Copy link
Contributor Author

razzeee commented Aug 3, 2016

That's what we're trying to use. I'm just not sure if our exe understands --hidden or if they have a bug not even setting that

@jasonblais
Copy link
Contributor

@yuya-oc Would you have any thoughts?

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 8, 2016

Not applicable for OS X. In my thoughts, apps should handle --hidden flag because electron doesn't understand it, too.

@razzeee
Copy link
Contributor Author

razzeee commented Aug 8, 2016

Well the mac implementation isn't trying to use --hidden
https://github.com/Teamwork/node-auto-launch/blob/master/src/AutoLaunchMac.coffee

var appLauncher = new AutoLaunch({
name: 'Mattermost'
name: 'Mattermost',
isHidden: true
Copy link
Contributor

@yuya-oc yuya-oc Aug 9, 2016

Choose a reason for hiding this comment

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

I misunderstood the changelog. This part is called when uninstalling the application. So you should use isHidden in settings.jsx which enables auto-start. Then, possibly main.js have to handle the --hidden flag.

@razzeee
Copy link
Contributor Author

razzeee commented Aug 9, 2016

Changed and now it's writing the registry key correctly. But it's like you said, electron doesn't understand --hidden for windows

@jasonblais
Copy link
Contributor

Any thoughts from the community here?

@razzeee
Copy link
Contributor Author

razzeee commented Aug 12, 2016

Would be nice to know if this implementation works on osx and Linux. Then we just need to figure out why windows doesn't.

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 13, 2016

Not need to test on OS X because auto-start is not implemented. Please see #164.

@jasonblais
Copy link
Contributor

I see, thanks @yuya-oc ,

@timroes @jnugh Would love to have your help checking if the desktop app stays minimized after autostart when logging in to a Linux machine.

If either of you would like to try, you can download an artifact here: https://circleci.com/gh/mattermost/desktop/539#artifacts

@timroes
Copy link
Contributor

timroes commented Aug 14, 2016

Doesn't seem to work on my system (Arch x64, pretty custom window manager - so perhaps someone with a regular Desktop Environment should test).

But looking at the generated desktop file:

[Desktop Entry]
Type=Application
Vestion=1.0
Name=Mattermost
Comment=Mattermost startup script
Exec=/home/timroes/apps/mattermost-desktop-linux-x64/Mattermost
StartupNotify=false
Terminal=false

and looking at the source code of auto-launch when setting isHidden there should actually be the --hidden flag in the end of the Exec command, which isn't in my case.

So there might be some problems with the generation of this desktop file. Also I tried starting the above mattermost with --hidden directly which doesn't cause it to be hidden on my system either.

Could you point me to the source parts, where --hidden is read in mattermost and done somethign appropriate?

@razzeee
Copy link
Contributor Author

razzeee commented Aug 14, 2016

I thought the hidden would be handled by electron packager as that's responsible for building the executeables etc. But it seems like it doesn't or is not responsible for the handling.

@jnugh
Copy link
Contributor

jnugh commented Aug 14, 2016

Does not work for me (Unity).

auto-launch generates a desktop file in ~/.config/autostart which should contains the --hidden param. When the default desktop file, which is being generated by electron-(builder/packager?), contains the hidden flag, the app would start minimized even if it wasn't auto started.

@timroes
Copy link
Contributor

timroes commented Aug 14, 2016

Yeah. It should :-)

But apparently for me the hidden isn't in the generated desktop file. If the electron wrapper doesn't handle the hidden flag, and you don't do it, that might some the mystery why it isn't working.

@jnugh
Copy link
Contributor

jnugh commented Aug 14, 2016

https://github.com/Teamwork/node-auto-launch/blob/master/src/index.coffee#L14
isHidden is actually mapped to isHiddenOnLaunch.

When running this directly from your branch, without using the artifacts, things work like expected. auto-launch 3.0 is out now. maybe it's been fixed already?

@jasonblais jasonblais added this to the v1.4.0 milestone Aug 16, 2016
@razzeee razzeee force-pushed the hide-autostart branch 2 times, most recently from 563bcc3 to ef76628 Compare August 16, 2016 08:34
@razzeee
Copy link
Contributor Author

razzeee commented Aug 16, 2016

Version 3.0 got rid of the ES6-Promise dependency, not sure how to fix that for us and get it building again. Changelog doesn't seem to involve a fix for our problem.

@jasonblais
Copy link
Contributor

Adding a pending label until Teamwork/node-auto-launch#39 is merged

@jasonblais jasonblais removed this from the v1.4.0 milestone Sep 4, 2016
@razzeee
Copy link
Contributor Author

razzeee commented Sep 4, 2016

@timroes @jnugh
Would be nice to know if it already works for linux like this: https://circleci.com/gh/mattermost/desktop/649#artifacts

@jnugh
Copy link
Contributor

jnugh commented Sep 5, 2016

Minimized autostart works for me :)

@timroes
Copy link
Contributor

timroes commented Sep 5, 2016

@razzeee works for me too (custom window manager, Archlinux x64), but brings me back to another problem, which I will open another ticket for (skip taskbar hints).

@razzeee
Copy link
Contributor Author

razzeee commented Sep 12, 2016

Upstream just merged and release Teamwork/node-auto-launch#39
I've updated this PR to include it, so maybe we can still make it into 1.4 🚀

Let's wait for the artifacts

@jasonblais
Copy link
Contributor

Awesome! I tried looking for the artifacts earlier today in CircleCI but couldn't find them..

@yuya-oc are they located elsewhere?

Added for v1.4 so we can speed up the testing.

@jasonblais jasonblais added this to the v1.4.0 milestone Sep 13, 2016
@razzeee
Copy link
Contributor Author

razzeee commented Sep 13, 2016

@jasonblais
Copy link
Contributor

Thanks!

Just tested (Windows 10, 64-bit) and it didn't auto-start for me when I restarted my machine.

I have Start app on login. checked.

@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 13, 2016

I have tested .zip and setup.exe on Windows 10 64-bit, and .deb on Ubuntu 14.04 64-bit. They are working well.

@jasonblais Would you check again?

@jasonblais
Copy link
Contributor

Hmm okay, maybe the issue was on my end.

@jasonblais
Copy link
Contributor

Tested; works on Windows 10 after disabling and re-enabling the start app on login setting.

@yuya-oc helped troubleshoot and likely related to me running a v1.3 version where auto-start might have been disabled.

The setting is not overwritten unless you disable and re-enable it in the settings UI (if you simply click 'Save' on the settings page without a change, the option will remain to what it was before in the registry). Investigating whether an easy fix for v1.4/3.4 or if we leave it for the next version.

For this particular PR however, we're good to go. Thanks everyone!

@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 13, 2016

So I will change the behavior when saving options in settings. It would always update the registry info. Thanks!

@yuya-oc yuya-oc merged commit 7cfde1e into mattermost:master Sep 13, 2016
@razzeee
Copy link
Contributor Author

razzeee commented Sep 13, 2016

Please do that in a pr and give me some heads up, as I have some problems
with other apps that work like that. Due to macaffee doing stupid stuff.

On Tue, Sep 13, 2016, 18:03 Yuya Ochiai [email protected] wrote:

So I will change the behavior when saving options in settings. It would
always update the registry info. Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#230 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFqyZKVpOO8DsRNwW3Z9YzdgFGq9hRGAks5qpslrgaJpZM4Ja9x5
.

@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 13, 2016

@razzeee I created new PR #283. Would you write about the problem you met in detail?

@jasonblais
Copy link
Contributor

Thanks @razzeee, agree that this will need some careful testing. Let's take it in the next release.

Thanks @yuya-oc for the pull request! We'll test it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants