Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

App installation screen improvements #680

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

App installation screen improvements #680

wants to merge 9 commits into from

Conversation

rosenpin
Copy link
Contributor

@rosenpin rosenpin commented May 8, 2017

Improvements include:

  • Users can now choose multiple files to install at once
  • Progress bar and an appropriate message when an app is successfully/unsuccessfully installed
  • Replaced RelativeLayout with LinearLayout where it seems to be more appropriate (activity_appinstaller.xml)

preview

@danielegobbetti
Copy link
Contributor

This is really interesting, thanks!

Especially wrt the layout changes, this should be carefully tested also with firmware installation and for other devices as well.

@@ -65,8 +65,7 @@
android:parentActivityName=".activities.SettingsActivity" />
<activity
android:name=".activities.FwAppInstallerActivity"
android:label="@string/title_activity_fw_app_insaller"
android:parentActivityName=".activities.ControlCenterv2">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the parent activity because we don't want the back button to re-open the main activity, we want it to finish the current activity

@@ -128,6 +148,12 @@ private void validateInstallation() {
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
if (GBApplication.isDarkThemeEnabled()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the super.onCreate sets the theme to GadgetbridgeThemeDark with action bar, we don't want the action bar because we use a toolbar in the layout xml file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try directly extending the appcompat activity as it is done in ControlCenterv2? https://github.com/Freeyourgadget/Gadgetbridge/blob/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/ControlCenterv2.java#L63
It should allow you to avoid this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I also merged the toolbars by using a toolbar.xml file, I include that file both in the ControlCenterv2 activity and the FwAppInstallerActivity activity layouts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to check if the dark theme is enabled and change the theme if it is. But the else statement is redundant

IntentFilter intentFilter = new IntentFilter();
intentFilter.addAction("nodomain.freeyourgadget.gadgetbridge.INSTALLATION_FAILED");
intentFilter.addAction("nodomain.freeyourgadget.gadgetbridge.INSTALLATION_SUCCESS");
registerReceiver(installationResultReceiver, intentFilter);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Register the receiver to the failed/success installation intent, the receiver updates the views (progress bar/button)


//Set up the toolbar
Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
toolbar.setTitleTextColor(ContextCompat.getColor(getApplicationContext(), android.R.color.white));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the text color to white

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about extending AppCompat activity: I believe you will not need this after changing.
Since it took some time to get the colors under control (and is not all done yet :-) ) I really would like to avoid using colors outside of the theme-inherited ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it, BTW, I'm pretty sure we should no longer use the themes that include action bars, AFAIK toolbars are the way to go.
https://medium.com/@ZakTaccardi/goodbye-actionbar-apis-hello-toolbar-af6ae7b31e5d

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we didn't because of lack of time on our end, perhaps you can open a new PR after this one is merged to take care of the migration? It would be really great!

//Set up the toolbar
Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
toolbar.setTitleTextColor(ContextCompat.getColor(getApplicationContext(), android.R.color.white));
toolbar.setTitle(getResources().getString(R.string.installation_d_d, currentAppIndex, appsCount));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the title to installing ($index/$count)

toolbar.setTitle(getResources().getString(R.string.installation_d_d, currentAppIndex, appsCount));
toolbar.setNavigationIcon(VectorDrawableCompat.create(getResources(), R.drawable.ic_arrow_back, null));
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP)
toolbar.setElevation(16);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set elevation only on lollipop+ devices (the fancy shadow)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppCompat should take care of this as well without explicit version checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, right

toolbar.setNavigationOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
finish();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finish the current installation activity when the back button is clicked

for (int i = 0; i < appsCount; i++) {
startIntent.setDataAndType(clipData.getItemAt(i).getUri(), null);
startIntent.putExtra("APPS_COUNT", appsCount);
startIntent.putExtra("APP_INDEX", Math.abs(i - appsCount));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we want it to be (1/3) -> (2/3) -> (3/3), not (3/3) -> (2/3) -> (1/3)
Why not use for (int i = appsCount; i >=0 ; i--) you ask?
That would be a good question.

GB.updateInstallNotification(getContext().getString(R.string.installation_failed_), false, 0, getContext());
} else {
resultIntent = new Intent("nodomain.freeyourgadget.gadgetbridge.INSTALLATION_SUCCESS");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installation was successful, set the intent action to success

if (!mIsInstalling) {
return;
}
if (hadError) {
resultIntent = new Intent("nodomain.freeyourgadget.gadgetbridge.INSTALLATION_FAILED");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installation was unsuccessful, set the intent action to failed

@@ -646,6 +649,7 @@ private void finishInstall(boolean hadError) {
}
}
}
getContext().sendBroadcast(resultIntent);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Send the broadcast

@@ -1,77 +1,69 @@
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This layout is pretty much a list of views that goes from top to bottom, no reason to use RelativeLayout with so many rules when we can simply use a vertical LinearLayout

@rosenpin
Copy link
Contributor Author

rosenpin commented May 9, 2017

Also, if you think it's a good idea, I can easily make the selected file install automatically without the need to click the install button and then the back button every time

@cpfeiffer
Copy link
Contributor

Sweet!
I just tested Mi Band firmware installation. Installation worked, but the progress notification never finishes. Might well be that this is unrelated to this PR though.

@rosenpin
Copy link
Contributor Author

rosenpin commented May 9, 2017

Oops, that makes sense, as I only made it so the PebbleIoThread sends a broadcast when the installation finishes/fails.
Couldn't find any other devices with installation "trackers" already in place.
The progress bar should be hidden when any non pebble device is used.

@danielegobbetti
Copy link
Contributor

danielegobbetti commented May 10, 2017

@rosenpin I believe cf1451bde5f51678ff19b57fb9da5157a0668dcd should be reverted. As it is now the theme would remain dark until activity is destroyed / application is restarted.

The rest of the theming is very well done, so +1 from my side (with the aforementioned observation).

I'm wondering if the multiple file selection should be limited to pbw (apps), as it would not work (but could be dangerous if it worked) for firmwares.

@rosenpin
Copy link
Contributor Author

I'm wondering if the multiple file selection should be limited to pbw (apps), as it would not work (but could be dangerous if it worked) for firmwares.

Good point, though it might be a bit confusing for users if the app won't explain the special behavior, maybe there should be different installations screen for apps/software?

@rosenpin
Copy link
Contributor Author

@danielegobbetti
Unrelated question: is it currently possible to filter/blacklist certain apps from posting notifications? If it isn't I can implement this feature, if it is, please let me know how to do this because I couldn't find an option to do so

@ashimokawa
Copy link
Contributor

Yes there is a blacklist feature in settings

@rosenpin
Copy link
Contributor Author

Thanks!

@cpfeiffer
Copy link
Contributor

So how do we proceed with this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants