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

Force single app instance #247

Merged
merged 7 commits into from
Aug 21, 2016
Merged

Force single app instance #247

merged 7 commits into from
Aug 21, 2016

Conversation

jgis
Copy link
Contributor

@jgis jgis commented Aug 16, 2016

  • Complete Mattermost Contributor Agreement
  • Execute npm run prettify to format codes
  • Write about environment which you tested
    • Operating system: Windows 10 64Bit

fixes issue #236

This fix may be problematic, if running multiple instances is required.

@razzeee
Copy link
Contributor

razzeee commented Aug 16, 2016

Thank you.

This should affect all operating systems, from reading the code. (which is good I guess)

@jgis jgis changed the title Force single app instance on windows Force single app instance Aug 16, 2016
if (app.makeSingleInstance((commandLine, workingDirectory) => {
// Someone tried to run a second instance, we should focus our window.
if (mainWindow) {
mainWindow.show()
Copy link
Contributor

@razzeee razzeee Aug 16, 2016

Choose a reason for hiding this comment

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

This might change the window state of the already running instance.

  • Try to maximize the window, then send it to the task bar (click the _ of the window), then try to start mattermost a second time.
  • The other case you need to check is maximize the window, then click the X to send it to the tray, then try to start mattermost a second time.

Something like this should work.

if (mainWindow.isMinimized()) mainWindow.restore();
else mainWindow.show();

@razzeee
Copy link
Contributor

razzeee commented Aug 17, 2016

@jgis can you please add your change to the changelog.md?

I just tested windows 10 and it seems to work flawless, good work.

For anyone willing to test, just grab a build from https://circleci.com/gh/mattermost/desktop/575#artifacts

@@ -30,6 +30,8 @@ Release date: TBD

### Bug Fixes

#### All platforms
- An existing application instance will be reused instead of starting another instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't have though of this as a bugfix, but as an improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My boss said, it's a bug :)
napkin 17 08 16 10 38 41 am

Copy link
Contributor

Choose a reason for hiding this comment

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

Well fine for me, let's wait, what the others say :)

Copy link
Contributor

Choose a reason for hiding this comment

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

In my feel, this PR is improvement. The behavior is normal for almost applications whether it’s good or bad. Just existing multiple instances at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change it to an improvement.

@razzeee
Copy link
Contributor

razzeee commented Aug 17, 2016

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 19, 2016

This doesn't affect OS X because originally OS X app works as a single app instance.

@jgis
Copy link
Contributor Author

jgis commented Aug 19, 2016

This affects Ubuntu Linux 14.04 (64Bit). Same change in behavior as in Windows.

@@ -16,10 +16,12 @@ Release date: TBD

#### Windows
- Update Mattermost icon for desktop notifications in Windows 10.

- An existing application instance will be reused instead of starting another instance.
-
Copy link
Contributor

Choose a reason for hiding this comment

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

You've slippen in an additional "-" here, which is unneeded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @razzeee! I'm learning...

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 21, 2016

I added a test for this in make-single-instance branch (3b1381b). The test succeeded in CircleCI, which is Linux. So it's ready to merge.

@jgis Please include the commit in this PR.

@yuya-oc yuya-oc added this to the v1.4.0 milestone Aug 21, 2016
@jasonblais
Copy link
Contributor

Fantastic!! Big thank you @jgis!

Would love to see you join our Desktop community channel if you'd like (and already haven't). We have lots of discussion there around the desktop app across the community and Mattermost team.

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 21, 2016

Thanks.

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.

None yet

4 participants