From f4f3842b02262a2483b958aa4b1e9b9e8fc4fb7b Mon Sep 17 00:00:00 2001 From: Eric Schwarzenbach Date: Mon, 17 Jul 2017 16:45:04 -0700 Subject: [PATCH] Sort APs by Speed label value instead of ranking score. This prevents unintuitive sorting if the ranking score (somehow) sorts differently from the speed label. This change also fixes a broken test in AccessPointTest.java 'testSummaryString_showsSpeedLabel()'. Bug: 63116984 Test: runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java Change-Id: I4a9e55950541ddbf32e01508484bd40326ef228e (cherry picked from commit 0b431c9af2324f412edfb329047c5ae9f1ebccd8) --- .../android/settingslib/wifi/AccessPoint.java | 39 +++++-------------- .../wifi/TestAccessPointBuilder.java | 8 ++++ .../settingslib/wifi/AccessPointTest.java | 11 +++++- 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java index 01d901496125b..0f8096aa2086c 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java @@ -177,7 +177,6 @@ public class AccessPoint implements Comparable { private Object mTag; - private int mRankingScore = Integer.MIN_VALUE; private int mSpeed = Speed.NONE; private boolean mIsScoredNetworkMetered = false; @@ -283,7 +282,6 @@ public class AccessPoint implements Comparable { this.mId = that.mId; this.mSpeed = that.mSpeed; this.mIsScoredNetworkMetered = that.mIsScoredNetworkMetered; - this.mRankingScore = that.mRankingScore; } /** @@ -294,7 +292,7 @@ public class AccessPoint implements Comparable { * 1. Active before inactive * 2. Reachable before unreachable * 3. Saved before unsaved - * 4. (Internal only) Network ranking score + * 4. Network speed value * 5. Stronger signal before weaker signal * 6. SSID alphabetically * @@ -315,9 +313,9 @@ public class AccessPoint implements Comparable { if (isSaved() && !other.isSaved()) return -1; if (!isSaved() && other.isSaved()) return 1; - // Higher scores go before lower scores - if (getRankingScore() != other.getRankingScore()) { - return (getRankingScore() > other.getRankingScore()) ? -1 : 1; + // Faster speeds go before slower speeds + if (getSpeed() != other.getSpeed()) { + return other.getSpeed() - getSpeed(); } // Sort by signal strength, bucketed by level @@ -376,9 +374,6 @@ public class AccessPoint implements Comparable { builder.append(',').append(securityToString(security, pskType)); } builder.append(",level=").append(getLevel()); - if (mRankingScore != Integer.MIN_VALUE) { - builder.append(",rankingScore=").append(mRankingScore); - } if (mSpeed != Speed.NONE) { builder.append(",speed=").append(mSpeed); } @@ -409,9 +404,7 @@ public class AccessPoint implements Comparable { */ private boolean updateScores(WifiNetworkScoreCache scoreCache) { int oldSpeed = mSpeed; - int oldRankingScore = mRankingScore; mSpeed = Speed.NONE; - mRankingScore = Integer.MIN_VALUE; if (isActive() && mInfo != null) { NetworkKey key = new NetworkKey(new WifiKey( @@ -419,9 +412,6 @@ public class AccessPoint implements Comparable { ScoredNetwork score = scoreCache.getScoredNetwork(key); if (score != null) { mSpeed = score.calculateBadge(mInfo.getRssi()); - if (score.hasRankingScore()) { - mRankingScore = score.calculateRankingScore(mInfo.getRssi()); - } } } else { for (ScanResult result : mScanResultCache.values()) { @@ -429,11 +419,6 @@ public class AccessPoint implements Comparable { if (score == null) { continue; } - - if (score.hasRankingScore()) { - mRankingScore = - Math.max(mRankingScore, score.calculateRankingScore(result.level)); - } // TODO(sghuman): Rename calculateBadge API mSpeed = Math.max(mSpeed, score.calculateBadge(result.level)); } @@ -443,7 +428,7 @@ public class AccessPoint implements Comparable { Log.i(TAG, String.format("%s: Set speed to %d", ssid, mSpeed)); } - return (oldSpeed != mSpeed || oldRankingScore != mRankingScore); + return oldSpeed != mSpeed; } /** @@ -799,12 +784,15 @@ public class AccessPoint implements Comparable { } } - // If Speed label is present, use the preference combination to prepend it to the summary. - if (mSpeed != Speed.NONE) { + // If Speed label and summary are both present, use the preference combination to combine + // the two, else return the non-null one. + if (getSpeedLabel() != null && summary.length() != 0) { return mContext.getResources().getString( R.string.preference_summary_default_combination, getSpeedLabel(), summary.toString()); + } else if (getSpeedLabel() != null) { + return getSpeedLabel(); } else { return summary.toString(); } @@ -834,9 +822,6 @@ public class AccessPoint implements Comparable { visibility.append(" rssi=").append(mInfo.getRssi()); visibility.append(" "); visibility.append(" score=").append(mInfo.score); - if (mRankingScore != Integer.MIN_VALUE) { - visibility.append(" rankingScore=").append(getRankingScore()); - } if (mSpeed != Speed.NONE) { visibility.append(" speed=").append(getSpeedLabel()); } @@ -1141,10 +1126,6 @@ public class AccessPoint implements Comparable { setRssi(AccessPoint.UNREACHABLE_RSSI); } - int getRankingScore() { - return mRankingScore; - } - int getSpeed() { return mSpeed;} @Nullable diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/TestAccessPointBuilder.java b/packages/SettingsLib/src/com/android/settingslib/wifi/TestAccessPointBuilder.java index 46a53a3bb4d4b..bb51330fdbcc2 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/TestAccessPointBuilder.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/TestAccessPointBuilder.java @@ -23,6 +23,7 @@ import android.net.wifi.ScanResult; import android.net.wifi.WifiConfiguration; import android.net.wifi.WifiInfo; import android.os.Bundle; +import com.android.settingslib.wifi.AccessPoint.Speed; import java.util.ArrayList; @@ -40,6 +41,7 @@ public class TestAccessPointBuilder { // set some sensible defaults private String mBssid = null; + private int mSpeed = Speed.NONE; private int mRssi = AccessPoint.UNREACHABLE_RSSI; private int mNetworkId = WifiConfiguration.INVALID_NETWORK_ID; private String ssid = "TestSsid"; @@ -78,6 +80,7 @@ public class TestAccessPointBuilder { bundle.putParcelableArrayList(AccessPoint.KEY_SCANRESULTCACHE, mScanResultCache); } bundle.putInt(AccessPoint.KEY_SECURITY, mSecurity); + bundle.putInt(AccessPoint.KEY_SPEED, mSpeed); AccessPoint ap = new AccessPoint(mContext, bundle); ap.setRssi(mRssi); @@ -127,6 +130,11 @@ public class TestAccessPointBuilder { return this; } + public TestAccessPointBuilder setSpeed(int speed) { + mSpeed = speed; + return this; + } + /** * Set whether the AccessPoint is reachable. * Side effect: if the signal level was not previously set, diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java index 9a3f21f8654e4..1fb805837f5ff 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java @@ -50,6 +50,7 @@ import android.text.style.TtsSpan; import com.android.settingslib.R; +import com.android.settingslib.wifi.AccessPoint.Speed; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -145,7 +146,13 @@ public class AccessPointTest { assertSortingWorks(savedAp, notSavedAp); } - //TODO: add tests for mRankingScore sort order if ranking is exposed + @Test + public void testCompareTo_GivesHighSpeedBeforeLowSpeed() { + AccessPoint fastAp = new TestAccessPointBuilder(mContext).setSpeed(Speed.FAST).build(); + AccessPoint slowAp = new TestAccessPointBuilder(mContext).setSpeed(Speed.SLOW).build(); + + assertSortingWorks(fastAp, slowAp); + } @Test public void testCompareTo_GivesHighLevelBeforeLowLevel() { @@ -505,7 +512,7 @@ public class AccessPointTest { } /** - * Assert that the first AccessPoint appears after the second AccessPoint + * Assert that the first AccessPoint appears before the second AccessPoint * once sorting has been completed. */ private void assertSortingWorks(AccessPoint first, AccessPoint second) {