Standard MasterChooser revamping

[jubeira]

Taken from the conversation in #239, these would be nice-to-have features in the MasterChooser:

• [x] Catching exception when master URI is a valid IP but no roscore is running. Currently in indigo the application crashes
• [x] Catch malformed URI
• [x] UI design update to include progress spinner when attempting connection
• [x] AppCompatActivity
• [ ] Automatic start settings (start with the last used URI); for that, catching connection errors is necessary.

From #192 & #193, there are a few more features that could be useful:

• [x] Remember recent hosts (useful when changing masters)
• [x] Add a default port if it is missing

[jubeira]

Any comments on this are welcome. @stratomda if you already implemented some of these and you want to contribute them, that would be awesome. /cc @dawonn

[stratomda]

@jubeira will do. I am currently getting my kinetic VM setup so I can work off of that.

[adamantivm]

@stratomda I've been using a docker image for that and has been working well for me. I'm happy to share the Dockerfile if you wish to try it out.

[stratomda]

@adamantivm Yeah that sounds great.

[adamantivm]

@stratomda here is my dockerfile FWIW https://gist.github.com/adamantivm/9def3ef44d8de6d6f1714e0e949d96ab

[stratomda]

@jubeira I will start working on the first two tasks to catch the exceptions.

[adamantivm]

awesome!

[stratomda]

Need advice. If the user inserts a URI that is valid but contains no running master (roscore) the following Exception is called:

java.lang.RuntimeException: java.net.ConnectException: failed to connect to /192.168.0.221 (port 11311) after 60000ms: isConnected failed: ECONNREFUSED (Connection refused)

The kicker is that I can't just catch a ConnectException because it has been rethrown as a RuntimeException deep in the MasterClient code. The compiler complains with: Exception 'java.net.ConnectException' is never thrown in the corresponding try block

I was thinking of doing the following in the MasterChooser okButtonClicked():

    new AsyncTask<Void, Void, Boolean>() {
      @Override
      protected Boolean doInBackground(Void... params) {
        try {
          toast("Trying to reach master...");
          MasterClient masterClient = new MasterClient(new URI(uri));
          masterClient.getUri(GraphName.of("android/master_chooser_activity"));
          toast("Connected!");
          return true;
        } catch (URISyntaxException e) {
          toast("Invalid URI.");
          return false;
        } catch (XmlRpcTimeoutException e) {
          toast("Master unreachable!");
          return false;
        }
        //Need Advice Here
        catch (Exception e)
        {
          if(e.getCause().getClass().toString().contains("ConnectException"))
          {
            toast("Unable to communicate with master!");
          }
          return false;
        }
      }

Thoughts? Is there a better way? @jubeira @adamantivm

[jubeira]

Given what you say, I think your proposal is not a bad idea. Another option would be using the exception message directly:

catch (Exception e) {
    if (e.getMessage().contains("ECONNREFUSED")) {  // store "ECONNREFUSED" in a final static member and reference that variable here
        toast("Unable to communicate with master!");
    }
}

I don't have a strong opinion about which one is the best though. In any case, I would add a comment explaining that deep down it's a connection exception.

[adamantivm]

I think that's an acceptable patch. Using instanceof instead would be my preference, but given the state things are at now, any of these would be better than nothing.

[stratomda]

@jubeira I can take the recent hosts and connection spinner tasks. I think the remaining tasks should be addressed in a new MasterChooser PreferenceActivity. This could get a little tricky due to the fact that the AppCompatPreferenceActivity requires a minSDK of 11. One way to go about this, would be to move the AppCompatRosActivity to android_15 and the PreferenceMasterChoose to android_15. We would need to use the custom masterchooser hooks in AppCompatRosActivty.

Does that sound like a good path forward?

/cc @adamantivm

[jubeira]

@stratomda great, sounds good! I already added all the constructors/ hooks in AppCompatRosActivity in #260 . I think android_15 depends on android_10, so perhaps it's not necessary to move AppCompatRosActivity to android_15. Other than that, I agree with your proposal; I'm already working in a PreferenceMasterChooser activity, it will take a while more. Once it's ready I will submit it; all comments are welcome of course.

[jubeira]

About the MasterChooserSettingsActivity... At least what I have in mind is a SettingsActivity (with app compat) which would be basically a list of settings, containing at least the Master URI and the option to start Master on device. Then, a user could extend it adding more app-specific settings, but without having to worry about the ROS settings. With #261 , the standard MasterChooser will have the capabilities of catching malformed URIs. I wonder if we could reuse those features for this MasterChooserSettingsActivity somehow. Does this make sense? Any thoughts?

[stratomda]

@jubeira @adamantivm thoughts on this layout for implementing the connection spinner? The LinearLayout is set to Visible when attempting connection and Gone when the connection AsyncTask completes.

[adamantivm]

I would be perfectly OK with that. It's definitely an improvement over what's there now.

[jubeira]

That is looking great @stratomda ! I think it's a good improvement

[stratomda]

I added a simple toast to inform the user when a new interface has been selected. Figured this falls within the Standard MasterChooser revamp.

[stratomda]

@jubeira Can we mark the AppCompatTask off. How do we want to handle the auto connect? Should we wait for this in the SettingsActivity (create a new issue for it) and mark it off? Also, what else can I help with?

[jubeira]

@stratomda sure! AppComnpat marked off. I can handle the last one. I was thinking of a basic settings activity with 3 settings to start with:

• Run Master on device
• Master URI
• Show settings on startup This last setting should allow the app to connect to the last configured master when the app starts if it is disabled. And if the user wants to go to the settings activity once again after starting the app, the activity should be started with a specific Intent Extra (so that the settings activity knows that it has to be shown even if the checkbox is disabled).

It would look something like this: The XML code should be able to be included in a larger settings activity, so that a user would not have to worry about the ROS settings, but with the possibility to add new settings in the same screen. What do you think? Perhaps I wouldn't include the auto-connect in the standard master chooser at least for now. The thing is that if the app auto-connects, there has to be some mechanism to force the master chooser to be shown again (that intent extra described above for example). But that would break the current master chooser logic. /cc @adamantivm

[jubeira]

@stratomda the code is ready, I need to separate it from my application to be able to upstream it. You can take a look, the code is public: https://github.com/ekumenlabs/tangobot/blob/master/tangobot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java