How do I disconnect without closing?

gooddosh commented 6 years ago

Firstly, this package is outstanding. Thank you.

I noticed the hostname is hardwired for dark themes, and I'm unsure of how to disconnect from a host without quitting Atom. Please note, I've only been using Atom for about 4 hours. Don't think I'll be using TextWrangler or Notepad++ again for as long as I live.

newinnovations commented 6 years ago

Please award a star if you like the package!

Indeed the ui is geared towards dark themes, which seem to be the most popular.

Connection management is something that has to be looked at. In the original package the ssh connection was made and closed on every load or save. Which was safe and efficient. The addition of the treeview made it desirable to keep the connection open all the time. Which may be fine in many situations but not all. We need a way to address this better, probably by implementing a timeout and/or a close button.

urban-1 commented 6 years ago

The idea for not closing connections was to make often "save" lighter. I will have a look into various alternatives and push a branch later today (code to add a new command/menu is minimal). I can also look into closing the connection after each operation. However, there are a few things that might need more work/thought:

What happens to already opened files? It will be up to the user to close them down?
If we automatically close all remote open files, how do we handle unsaved tabs?
In any case, after the connection is closed, if a "save" is done on a RemoteEdit tab then a new connection will be established. I am not sure if this is closed automatically or not...

I will experiment a bit and report here. Let me know if you have concerns or implementation ideas

gooddosh commented 6 years ago

I strongly prefer a disconnect button over a timeout and humbly request any timeout be an idle one, configurable in settings. :)

gooddosh commented 6 years ago

PS: I have bestowed a star! Again, great package!

urban-1 commented 6 years ago

Hi all,

I have pushed a branch at: https://github.com/urban-1/remote-edit-ni/tree/feature_disconnect

The connections we have are:

The files-view retains an open connection
Each RemoteEdit tab has its own connection (this allows for editing on multiple remote servers simultaneously)

Now, my changes:

Introduced a setting to allow the user to close the tab's connection after every "save". I wouldn't suggest this for people that save often since it slows down things. I also wouldn't suggest this over very slow networks. In general however I do not see a huge overhead when not saving often or working over fast/decent networks. Additionally, this would allow one to safely open 100s of files (not that I see a use for it... but you never know). Personally, I will keep this off since I see no harm at having 10-20 connections to a server (which have their idle timeout of course) but I have seen admins and services limiting the number of connections they allow.
Added a new command and menu: "Close all connections". This will disconnect all SSH connections from both files-view and any open tabs. Additionally, it will close the files-view panel, but will leave any tabs open (so we don't have to deal with unsaved tabs closing). If the user saves a remote-edit tab, a new connection will be established. Now, depending on the previous setting ("close on save") one might have to re-issue the command to terminate the new connection(s).
RemoteEditEditor now overrides the TextEditor destroy() in order to clean-up and close its connection (if any). This is more of a bug since it should have been the default case.
Experimenting with the above, I also hit a bug where we resolved a Promise early when writing the remote file. This has no impact on previous versions since the promise was not used but should now work as expected.

@newinnovations: let me know when reviewed and if/when I should make a PR

@gooddosh and @janstieler: Does the above behaviour work for you?

PS: I did not test with a connection timeout but I killed the ssh connections locally and none of the disconnects lead to a crash/log on console so there should be no problems with timeouts

gooddosh commented 6 years ago

Woot!

newinnovations commented 6 years ago

Woot indeed. Sounds good and I will start test driving it today.

My only concern is reliability. In the recent past I have had a situation where the connection had failed due to network problems and saving failed silently (the connection was not rebuilt). Which makes me a bit suspicious about the underlying library, but it may very well be with remote-edit. Problem is that I have not been able to reproduce that specific situation.

A situation where the connection is set up, file saved and closed and and where that progress is shown in some way may be more reliable. Browsing should not close the connection directly but could time out after a minute or so.

Can we find a way that we are absolutely sure that a save was successful?

gooddosh commented 6 years ago

I'm new to Git & Atom. How do I install this branch before it's published in apm?

urban-1 commented 6 years ago

@gooddosh You need to have git and apm installed and in your path (installation depends on your setup/system).

My setup is more or less the following:

# Go to the directory you like to have the source code
 cd /path/to/install/source

# Clone newinnovations' master since this is the upstream repo (and stable master)
 git clone git@github.com:newinnovations/remote-edit-ni.git

# go into the newly cloned repo
cd remote-edit-ni/

# Add my fork as a git remote named "urban-1" (you can change the name to what you like)
git remote add urban-1 git@github.com:urban-1/remote-edit-ni.git

# Fetch all of my tags and branches (changes nothing to the code)
git fetch urban-1

# Show all branches (you should see now: remotes/urban-1/feature_disconnect)
git branch -a

# Now "checkout" the branch (changes the source to that feature)
git checkout feature_disconnect 

# Install package dependencies
apm install

# Link this folder to atom installation - not sure this is allowed if 
# you already installed the package with apm install
apm link

#... test, test, test ...

# Unlink/uninstall the testing package - reinstall from atom.io if needed
apm unlink

I hope that helps - let us know if you manage to review the branch

urban-1 commented 6 years ago

@newinnovations: Regarding reliability I think we are good atm, however, I do remember cases where the connection was timing out and was not reconnecting (some versions ago). Not sure what caused it but remember https://github.com/sveale/remote-edit/issues/226 which made buffer.save() async.

Now, https://github.com/newinnovations/remote-edit-ni/commit/7a9f3aea93338c658c33368357d4ef65c8009504#diff-ad08e8b5a4ec10998ff9b3bdc131f954L97 (line 97) tried to fix the async save but the emitter is still wrong. We made in the past upload() and initiateUpload() returning Promises. This allows for more robust implementation. Pushed another commit moving the emitter to the correct place here (line 127).

I think we need better (more detailed) error handling at https://github.com/newinnovations/remote-edit-ni/blob/master/lib/model/remote-edit-editor.js#L214. I see two problems as is now:

Errors that come from writeFile() are not handled at all! Only errors from wrong password are
I don't see us rejecting the Promise on error (since we dont handle them..)

I would prefer to have this as a separate fix_ branch and proceed with feature_disconnect. Let me know

gooddosh commented 6 years ago

I have git and apm installed. Never the less, this happened.

$ git clone git@github.com mailto:git@github.com:newinnovations/remote-edit-ni.git Cloning into 'remote-edit-ni'... The authenticity of host 'github.com http://github.com/ (192.30.253.112)' can't be established. RSA key fingerprint is SHA256:nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8. Are you sure you want to continue connecting (yes/no)? yes Warning: Permanently added 'github.com http://github.com/,192.30.253.112' (RSA) to the list of known hosts. Permission denied (publickey). fatal: Could not read from remote repository.

Please make sure you have the correct access rights and the repository exists.

On Apr 17, 2018, at 6:28 AM, Andreas Bontozoglou <notifications@github.com mailto:notifications@github.com> wrote:

git clone git@github.com mailto:git@github.com:newinnovations/remote-edit-ni.git

newinnovations commented 6 years ago

To checkout Andreas' feature I did:

git clone https://github.com/urban-1/remote-edit-ni
cd remote-edit-ni
git checkout feature_disconnect

But referring to the instructions by Andreas, which are better and more complete:

git clone git@github.com:newinnovations/remote-edit-ni.git

can be replaced with

git clone https://github.com/newinnovations/remote-edit-ni

gooddosh commented 6 years ago

Thanks https://github.com/ https://github.com/ works. Why doesn't git@github.com mailto:git@github.com:?

Do I have to run apm rm remote-edit-ni before testing this variant? I would think so

What does apm link/unlink do?

On Apr 18, 2018, at 5:18 AM, Martin vanderWerff <notifications@github.com mailto:notifications@github.com> wrote:

To checkout Andreas' feature I did:

git clone https://github.com/urban-1/remote-edit-ni https://github.com/urban-1/remote-edit-ni cd remote-edit-ni git checkout feature_disconnect But referring to the instructions by Andreas, which are better and more complete:

git clone git@github.com mailto:git@github.com:newinnovations/remote-edit-ni.git can be replaced with

git clone https://github.com/newinnovations/remote-edit-ni https://github.com/newinnovations/remote-edit-ni — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/newinnovations/remote-edit-ni/issues/14#issuecomment-382321697, or mute the thread https://github.com/notifications/unsubscribe-auth/AH5P9uiwL2xrCXlZ9K3HwiW4CWju42FJks5tpwTPgaJpZM4TViG4.

https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png https://github.com/newinnovations/remote-edit-ni https://github.com/urban-1/remote-edit-ni/r/ncd mailto:git@github.com https://github.com/newinnovations/remote-edit-ni/r/n%60%60%60/r/n/r/n https://github.com/newinnovations/remote-edit-ni/issues/14#issuecomment-382321697

newinnovations commented 6 years ago

@urban-1 I have been testing for a while and just noticed something. When I split a remote file, saving in the split file does nothing and also alt-r+m does not work as expected. Saving in the original tab still works. Not checked, but I think this was working in the master branch.

urban-1 commented 6 years ago

HI @newinnovations, I cannot reproduce the above on feature_disconnect, how do you split? What I did was (in order):

Open 2 files and tile them left/right by mouse
Modify left and Ctrl+S (confirmed save)
Modify right and Ctrl+S (also saved)
Open a 3rd file, tile it (bottom left), modify and File->Save (also saved)
Alt-r+m, in any of the opened files changed the files-view to the correct folder
Issued a disconnect with Alt-r+d, edit the last file and Ctrl+S (saved)

I will make a PR for this branch - if you are able to reproduce the same behavior can you post the steps in a new issue?

newinnovations commented 6 years ago

The problem is also in the master branch. I will create an issue for this.

newinnovations / remote-edit-ni

How do I disconnect without closing? #14