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 Support for JuiceSSH #204

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pc-coholic
Copy link

This PR adds support for JuiceSSH as requested in #141.

This might not be the most elegant way, seeing that we're adding another icon and settings-holder for JuiceSSH next to IrssiConnectbot - when even more clients are added, this probably won't be scaling very well...

But for the meantime, where we just have 2 clients, this should do just fine :-)

Unfortunately, JuiceSSH does not offer a way as easy as IrssiConnectbot does for retrieving and using connections - we have to use their plugin-system - this also means, we have two new permissions on board (com.sonelli.juicessh.api.v1.permission.READ_CONNECTIONS and com.sonelli.juicessh.api.v1.permission.OPEN_SESSIONS). But as they are non-vital to the functioning of the app, they will only be requested during runtime when JuiceSSH is installed on the device and the user requests to link his device to JuiceSSH.

@murgo
Copy link
Owner

murgo commented Dec 3, 2018

Thanks, this looks pretty swell! I'll take test it a bunch in the near future but it seems pretty OK, and it's quite often requested feature.

@murgo
Copy link
Owner

murgo commented Dec 3, 2018

I tested it out and it works pretty ok, but there is one issue. I usually have JuiceSSH mosh connection running in the background, and it would be better if the icon in IrssiNotifier would use that connection if it's already open. Now it tries to open a new connection even when I have the same connection selected in settings, which fails for my setup since I have just one mosh port open.

IIRC ConnectBot just brought the existing connection to view if it was already open.

This PR also fixes the missing ICB icon which is nice bonus, so thanks for that too.

@pc-coholic
Copy link
Author

Unfortunately, the JuiceSSH Plugin API only lets you interact with sessions that you opened through the plugin... :-/ Not sure how we can get around this...

@murgo
Copy link
Owner

murgo commented Dec 4, 2018

I haven't played around with the JuiceSSH plugin API, but are you sure? It says on https://juicessh.com/faq/what-can-plugins-do "Attach to a background JuiceSSH session (show a terminal)" which sounds like what we need.

@murgo
Copy link
Owner

murgo commented Dec 4, 2018

From a quick read it seems like PluginClient.attach() is worth a shot: https://juicessh.com/faq/pluginclient-launching-and-interacting-with-connections

It doesn't specify if this can only be used with sessions launched from the app, but at least it could be used so when opening the SSH client from the app many times it would reuse the connection, which would mitigate the issue.

After the attach() has been implemented, we could also send an email JuiceSSH or contact them otherwise to ask if it's even possible to attach to previously opened sessions.

@murgo
Copy link
Owner

murgo commented Dec 4, 2018

Oh the PR already uses the attach() it seems, but there is still a bug that subsequent openings do not reuse same connection.

@pc-coholic
Copy link
Author

pc-coholic commented Dec 4, 2018

Perhaps I am misunderstanding the proper way to use it...

Right now, I client.start() to get a connection to the JuiceSSH-plugin manager, client.connect() with the UUID of the connection the user has chosen and then client.attach() to the session and thus bring it into the foreground.

public void attach(int sessionId, String sessionKey);
The sessionId and sessionKey are passed to the OnSessionStartedListener and are independent of the connection UUID.

Just keeping sessionId and sessionKey in memory and reusing them would probably work to reattach an open session. However - as client.attach() is void, there is no way to tell if the session was attached or not. So we can't really tell if we need to just attach() or connect() a fresh session. Trying to connect an unconnected session will throw a ServiceNotConnectedException - so I was wrong for that last part...

I think, there might be a way around it - but I'm not really happy with this idea...
Right now, everytime the user clicks on the launch-icon, I open a connection to the JuiceSSH plugin-manager, connect to the desired host, attach the session/bring it into the foreground, and then disconnect again from the plugin-manager.

By placing the whole interaction with the JuiceSSH pluginmanager into the MainActivity and placing the app into a wakelock and not closing the connection to the plugin-manager, we would be able to keep an eye out for the OnSessionFinishedListener. That way, we can assume that as long as this listener hasn't been called, that our session is still running and can be reattached. But - of course - this would also only work for sessions that we started in the first place. A session started by the user manually would still trigger a second connection to the host.

And - of course - there is that really unpleasant thing of the wakelock which will probably cause additional battery drain :-(

The proper way to handle this kind of scenario without wakelocks, etc. would probably by using broadcasts - but unfortunately JuiceSSH does not provide them :-/ (And to be 100% frank: I am not even sure if placing the whole activity in a wakelock would even work with the changes introduced in Android P... But that's a whole different story)

I just want to say, that I'm not trying to talk you out of your (very reasonable) feature-request... But unless there is some other way that I haven't seen, I don't this will be easy to achieve... Sorry...

@pc-coholic
Copy link
Author

After looking at
https://github.com/Sonelli/juicessh-pluginlibrary/blob/cf1a9c3494f15c324fbb1e373b49b1b3a79b1926/src/main/java/com/sonelli/juicessh/pluginlibrary/ConnectionPluginClient.java#L72-L96 , I realized that we can probably get rid of most of the plugin-stuff and the opening/attaching/closing, etc and just plainly send out an Intent with the connection UUID.

I am not 100% sure - but if we are lucky, setting the flag Intent.FLAG_ACTIVITY_REORDER_TO_FRONT might just do what we are looking for...

@pc-coholic
Copy link
Author

I verified my idea - and unfortunately I was not successful :-(

The ServiceNotConnectedException is not thrown because the session could not be attached - instead it is a generic exception when there is no connection to the JuiceSSH plugin-manager. So not useful for us :-(

Seeing this, I reworked the code to only use the generic Uri-intent to launch the session. It still won't reattach an already running session (not even with Intent.FLAG_ACTIVITY_REORDER_TO_FRONT) as it seems like JuiceSSH is just set on creating a new session on every call.

Even though I'm out of ideas at this point, I would assume that there is some convoluted way to observe the activity-stack and reorder it to bring an existing session into the foreground again...

@murgo
Copy link
Owner

murgo commented Dec 5, 2018

I'll throw email to JuiceSSH folks if they have any ideas if it could be done somehow.

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