Refactor network connection setting on Android by mykola-mokhnach · Pull Request #865

Refactor network connection setting on Android by mykola-mokhnach · Pull Request #865 · appium/java-client

Change list

With the previous implementation it was only possible to enable connections, but not disable. I think the current approach will make the feature much more flexible than it was before.

Types of changes

No changes in production code.
Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)


		/**
		* Sets airplane mode to disabled state if it was enabled.
		* This option only works up to Android 6.

Choose a reason for hiding this comment

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

If i remember right, even data and wifi through set/get connection wasn't working with Android 7. Also i thought retrieving connection state was only possible through shell. may be i'm wrong. Let me try this today.

Choose a reason for hiding this comment

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

wifi should work. for data there is a comment, that it only works on emulator and on rooted devices

		* @return true if airplane mode is enabled.
		*/
		public boolean isAirplaneModeEnabled() {
		return (bitMask & AIRPLANE_MODE_MASK) != 0;

Choose a reason for hiding this comment

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

It seems that all these bitwise operations can be simplified via java.utils.BitSet

Choose a reason for hiding this comment

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

do you find the current implementation not readable enough? I think BitSet usage might add an unnecessary overhead, since the actual count of bitwise operations here is pretty limited anyway

Choose a reason for hiding this comment

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

ok, np. However from my point of view bs.set(AIRPLANE_MODE_BIT, false) looks more naturaly than this.bitMask = bitMask & ~AIRPLANE_MODE_MASK; (Maybe I just don't like bitwise logic :)

Mykola Mokhnach added 2 commits

April 11, 2018 13:49

		.withAirplaneModeDisabled()
		.build());
		ConnectionState state = driver.getConnection();
		assertTrue(!state.isAirplaneModeEnabled() && !state.isWiFiEnabled() && !state.isDataEnabled());

Choose a reason for hiding this comment

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

check for wifi and data is not necessary i believe as disabling airplane mode will only disable airplane mode incase if it was already enabled and leave's the rest of connection as is. Isn't it?

Enabling airplane mode will disable both data and wifi but disabling airplane will not disable/enable rest. Correct me if i'm wrong.

Choose a reason for hiding this comment

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

Here also we might need to remove the additional checks.

Choose a reason for hiding this comment

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

These I'd prefer to keep

Mykola Mokhnach added 3 commits

April 11, 2018 16:05

Choose a reason for hiding this comment

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

I have checked these changes on an emulator of Android 6. Tests were passed. The design is looking good to me.
@SrinivasanTarget if you are agree so lets merge it.