Feature: using custom master chooser #55

jubeira · 2017-03-16T21:28:50Z

This PR adds a settings activity that replaces the standard Master Chooser.
This allows connecting automatically to ROS master upon app start, as well as eventually adding more features to the Activity.

The MasterChooserSettingsActivity could be upstreamed to Rosjava once this is reviewed, corrected and ready.

-Toolbar working -Menu display added with no actions

… run functionality.

jubeira · 2017-03-16T21:30:00Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

+        mSharedPref = PreferenceManager.getDefaultSharedPreferences(getBaseContext());
+
+        // This activity requires a method to force its start when "show settings" is off.
+        boolean editSettings = getIntent().getBooleanExtra("edit_settings", false);


I'm not sure if this is the best way of doing this. Comments are welcome!

FYI, in TangoRosStreamer we overrided startMasterChooser in the RunningActivity to start the SettingsActivity when a certain boolean is true. In this case, this boolean corresponds to showSettings I believe?
Did you think of doing something similar?

Yes! Let me explain a bit further:
A few days ago we introduced some changes in RosActivity. The idea was to provide a standard method of using a custom MasterChooser without needing to override startMasterChooser or any other method.
That PR in android_core allows to set a custom Activity when the RosActivity is created, so instead of starting the standard MasterChooser, it will start a custom activity like this one. The "problem" here is that in your case, you have access to the shared preferences in the overridden startMasterChooser (so you can decide what to execute at that point). I don't have access to those variables in my MainActivity constructor, so I needed another mechanism to implement the auto-connection.
Then, I added a shared pref (show_settings_on_startup), so the user can skip the preferences activity the next time the app is launched. The problem is that this flag would prevent the settings activity to be launched ever again if there was no way to force it. And that's where this "edit_settings" extra comes in: if the "show_settings_on_startup" is off, it allows launching the settings activity.

I understand the issue. I think it's not super clean, but I think it is acceptable for now. Perhaps I'd change the name edit_settings to force_show_settings , ignore_show_settings_on_startup or something like that.

@jubeira Thanks for the explanation.

jubeira · 2017-03-16T21:32:31Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

+    }
+
+    @Override
+    public void onBackPressed() {


I implemented this functionality in the back button callback because it's the fastest way of using the existing functionality of the standard RosActivity (basically, returning what the standard onActivityResult expects).
The onActivityResult callback could be modified using setOnActivityResultCallback from RosActivity too as an alternative.

PerrineAguiar

It looks good, but I have some comments/questions about the MasterChooserSettingsActivity.

PerrineAguiar · 2017-03-17T10:42:48Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

+        // This activity requires a method to force its start when "show settings" is off.
+        boolean editSettings = getIntent().getBooleanExtra("edit_settings", false);
+        boolean showSettings = mSharedPref.getBoolean(getString(R.string.pref_show_settings_on_startup_key), true);
+        if (!showSettings && !editSettings) {


Can you explain the difference between showSettings and editSettings?

(see comment above).
Basically, showSettings is intended to skip the settings activity the next time the app is launched. And editSettings forces its execution if showSettings is disabled (for example, when you click the "settings" menu button in the MainActivity to edit the settings again).
Now that you mention it, perhaps the variable naming is not the best.

PerrineAguiar · 2017-03-17T10:46:57Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

+        mSharedPref = PreferenceManager.getDefaultSharedPreferences(getBaseContext());
+
+        // This activity requires a method to force its start when "show settings" is off.
+        boolean editSettings = getIntent().getBooleanExtra("edit_settings", false);


FYI, in TangoRosStreamer we overrided startMasterChooser in the RunningActivity to start the SettingsActivity when a certain boolean is true. In this case, this boolean corresponds to showSettings I believe?
Did you think of doing something similar?

adamantivm

some comments but from my perspective it's good for now.

adamantivm · 2017-03-17T16:53:45Z

tangobot_app/app/src/main/java/com/ekumen/tangobot/application/MainActivity.java

+
+    @Override
+    public boolean onOptionsItemSelected(MenuItem item) {
+        mLog.info("Option selected: " + item.getItemId());


do you need to leave this logging (also below)?

adamantivm · 2017-03-17T16:53:53Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

@@ -0,0 +1,153 @@
+package com.ekumen.tangobot.application;
+


nit: extra line

adamantivm · 2017-03-17T16:54:02Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

@@ -0,0 +1,153 @@
+package com.ekumen.tangobot.application;


missing license header

adamantivm · 2017-03-17T16:54:41Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

+
+                // Set the summary to reflect the new value.
+                preference.setSummary(
+                        index >= 0


the indentation here looks odd

adamantivm · 2017-03-17T17:01:15Z

...bot_app/app/src/main/java/com/ekumen/tangobot/application/MasterChooserSettingsActivity.java

+        mSharedPref = PreferenceManager.getDefaultSharedPreferences(getBaseContext());
+
+        // This activity requires a method to force its start when "show settings" is off.
+        boolean editSettings = getIntent().getBooleanExtra("edit_settings", false);


I understand the issue. I think it's not super clean, but I think it is acceptable for now. Perhaps I'd change the name edit_settings to force_show_settings , ignore_show_settings_on_startup or something like that.

adamantivm · 2017-03-17T17:03:15Z

tangobot_app/app/src/main/java/com/ekumen/tangobot/application/SettingsActivity.java

@@ -0,0 +1,22 @@
+package com.ekumen.tangobot.application;


missing copyright header

-Fix for preference change after attempting a connection for the first time.

jubeira · 2017-03-17T18:49:19Z

Thanks for your reviews, the comments have been addressed.
I also addressed a problem with the display of the Snackbar when changing the settings after a connection attempt (as the settings activity can be shown on startup even after the first run, there was case that wasn't being handled properly). Merging now.

Juan Ignacio Ubeira added 4 commits March 14, 2017 15:58

Added settings activity with a draft of MasterChooser.

5b57336

Separating Settings activity in two layers.

8d0793a

-Toolbar working -Menu display added with no actions

Basic functionality for MasterChooserSettingsActivity working - first…

9e33477

… run functionality.

Adding "Show settings" and Local master features to SettingsActivity.

9138e1b

jubeira requested review from adamantivm and PerrineAguiar March 16, 2017 21:28

jubeira commented Mar 16, 2017

View reviewed changes

PerrineAguiar reviewed Mar 17, 2017

View reviewed changes

adamantivm approved these changes Mar 17, 2017

View reviewed changes

Addressing comments.

84a01c9

-Fix for preference change after attempting a connection for the first time.

jubeira merged commit 212fe78 into ekumenlabs:master Mar 17, 2017

jubeira mentioned this pull request Mar 17, 2017

App design - Parameter configuration #52

Open

3 tasks

jubeira deleted the dev/using_custom_master_chooser branch March 22, 2017 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: using custom master chooser #55

Feature: using custom master chooser #55

jubeira commented Mar 16, 2017

jubeira Mar 16, 2017

PerrineAguiar Mar 17, 2017

jubeira Mar 17, 2017 •

edited

Loading

adamantivm Mar 17, 2017

PerrineAguiar Mar 20, 2017

jubeira Mar 16, 2017

PerrineAguiar left a comment

PerrineAguiar Mar 17, 2017

jubeira Mar 17, 2017

PerrineAguiar Mar 17, 2017

adamantivm left a comment

adamantivm Mar 17, 2017

adamantivm Mar 17, 2017

adamantivm Mar 17, 2017

adamantivm Mar 17, 2017

adamantivm Mar 17, 2017

adamantivm Mar 17, 2017

jubeira commented Mar 17, 2017

Feature: using custom master chooser #55

Feature: using custom master chooser #55

Conversation

jubeira commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PerrineAguiar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamantivm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira commented Mar 17, 2017

jubeira Mar 17, 2017 •

edited

Loading