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

Add daemon process handling (fixes #48) #71

Merged
merged 7 commits into from
Aug 3, 2018
Merged

Conversation

calmh
Copy link
Member

@calmh calmh commented Jul 30, 2018

UI wise it adds a status item that is split into connection status and process status. It's possible and valid for the process to be down and the connection to be up - if the process is running but not controlled by us, or we're talking to a remote host. Depending on the process state there are start/stop/restart actions available.

All good:

screen shot 2018-07-30 at 11 32 02

Process down, connection up:

screen shot 2018-07-30 at 11 32 27

Fail:

screen shot 2018-07-30 at 11 32 42

Remaining points and questions:

  • We probably need a preferences flag for not attempting to start/manage the process at all, for cases where the user is not talking to localhost. However this isn't possible today, so maybe future enhancement?

  • UI wise, how do we want to talk about this. I say "daemon" above because that's technically correct, but will sound occult and/or meaningless to most presumptive users, probably. What should we call it?

  • Likewise, "connection". It's clear to me that this is towards the Syncthing daemon, but a regular user might not see the two things as separate at all... Maybe this whole PR is misdirected. :/

  • Cleanup remains, so far I've just added stuff and not removed the previous process monitoring / moved the "Executable" config, etc.

@calmh
Copy link
Member Author

calmh commented Jul 30, 2018

The alignment of the status item is ugly IMHO but this seems to be the way of things, and there is prior art.

screen shot 2018-07-30 at 11 57 30

Adding a separator improves it a little.

screen shot 2018-07-30 at 11 59 42

@xor-gate
Copy link
Member

Initial work looks good, need some more time to think forward on your question points. I also like a extra separator just as docker tray after the status.

@calmh
Copy link
Member Author

calmh commented Jul 31, 2018

Maybe call the daemon part "Background service", I think it's easier for users to digest that there is something they look at and a separate background service that shuffles the actual data. As for connection, maybe "API"? It might be just a mysterious thing for some users, but it if we put it below the background service it can be easily ignored while green and still be something mentioned in support requests while red... But maybe background service is too long?

screen shot 2018-07-31 at 09 50 17

@xor-gate
Copy link
Member

Is there any reason why we should suffix with the state? Is the image not enough? It makes the menu item very long and doesn't add any information.

@calmh
Copy link
Member Author

calmh commented Jul 31, 2018

Color blindness / accessibility (screen readers etc)

@calmh
Copy link
Member Author

calmh commented Jul 31, 2018

We could s/Background Service/Sync Service/ or even Sync Svc (with some lack of clarity; I think Sync Service would be concise enough).

@xor-gate
Copy link
Member

IMHO we should name it then by its name Syncthing Service a few letters doesn't matter with this long menu item text then.

* develop:
  Activate existing windows for About/Preferences (fixes syncthing#72) (syncthing#73)
@calmh calmh changed the title WIP: Add daemon process handling (fixes #48) Add daemon process handling (fixes #48) Aug 3, 2018
@calmh
Copy link
Member Author

calmh commented Aug 3, 2018

Renamed, cleaned up a bit. Please review :)

@xor-gate
Copy link
Member

xor-gate commented Aug 3, 2018

Thanks @calmh, for me it is fine now. We should pack it up and ship it!

@xor-gate xor-gate merged commit d07f8ae into syncthing:develop Aug 3, 2018
@xor-gate xor-gate added this to the Next milestone Aug 3, 2018
@calmh calmh deleted the daemon2 branch August 4, 2018 07:15
xor-gate added a commit that referenced this pull request Nov 7, 2018
* syncthing/Scripts/syncthing-resource.sh: Bundle with 0.14.47

* Get rid of syncthing/Scripts/syncthing-inotify-resource.sh as filesystem change notifications are builtin to syncthing since v0.14.47

* Delay reconnect attempts if the server returned an error (#47)

This should fix high CPU usage issue when API key is invalid. Partial fixes #46.

* Bump syncthing resource to v0.14.48 (#55)

* syncthing/STApplication.m: Sort folders submenu ascending (fixes #49) (#53)

* syncthing/STApplication.m: Sort folders submenu ascending (fixes #49)
* syncthing/STApplication.m: Fix review comment of @virusman

* Use CocoaPods for Sparkle framework dependency (fixes #57) (#60)

* Update syncthing resource to v0.14.49 (#64)

* Update syncthing resource to v0.14.49
* syncthing/Scripts/syncthing-resource.sh: Use new tarball name

* Adjust event request timeout (fixes #65) (#66)

This sets the event request a bit longer and, more importantly, informs
Syncthing of the timeout. The result is that the request returns
successfully without events, and we don't get a timeout error.

* Refactor HTTP(S) handling (fixes #24)

This moves event getting into the XGSyncthing library, while also
refactoring it to use NSURLSession, a delegate for allowing local TLS
certificates, and proper parameters.

The delegate simply skips HTTPS certificate checking on hosts called
"localhost", "127.0.0.1", or "::1". I think this is what most people
would want from this tool.

* Remove #24 from README too

* README.md: Update to use new URLs (#67)

Also add @calmh to AUTHORS.md

* Move LocalhostTLSDelegate files accidentally committed at root (#70)

* Generate appcast.xml from github releases (#63)

* script/ghreleases2appcast.go: Initial working github releases to sparkle framework appcast.xml converter (resolves #62)

* Activate existing windows for About/Preferences (fixes #72) (#73)

* Add daemon process handling (fixes #48) (#71)

* Add new submenu with status of the syncthing service
* Cleanup some code

* Copy executable from bundle (fixes #37) (#76)

This copies the bundled executable before executing it, if the target
executable doesn't already exist. This has several desirable properties:

- We do not mess up the application bundle by letting Syncthing upgrade

- The user can revert an undesired upgrade by deleting the binary and
  restarting the wrapper.

The binary is stored in ~/Application Support/Syncthing-macOS because I
don't want to put it in Syncthing's config directory. That directory is
in principle managed by Syncthing and we can't be sure it won't be
removed or something.

* Prepare for v0.14.49-1 release (#75)

* syncthing.xcodeproj/project.pbxproj: Rename PRODUCT_BUNDLE_IDENTIFIER for Sparkle updater (see #74) (#79)

From com.github.syncthing.syncthing-macos back to com.github.xor-gate.syncthing-macosx to fix Sparkle update issue in #74.

* script/ghreleases2appcast.go: Use working blackfriday.v1 (see #77) (#78)

* script/ghreleases2appcast.go: Changes for upgrades.s.n deployment (#80)

Basically, don't alter the download URLs for now, indent the XML for
readability.

* Fixup project move leftovers and update some documentation (#81)

* Fixup project move leftovers and update some documentation and contributing
* Cleanup xcode project with useless xgsyncthing target
* README.md: Add note about homebrew cask (ref #23)

* readme: Update contribution guidelines link (fixes #83)

* Bump syncthing resource to 0.14.50

* Use latest syncthing v0.14.51 resource

* CocoaPods: Manual patch EXPANDED_CODE_SIGN_IDENTITY: unbound variable #7708 bug

* Use syncthing v0.14.52 resource
@syncthing syncthing locked and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants