Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically advance setup wizard on WiFi connection #92

Merged
merged 5 commits into from Mar 29, 2019

Conversation

@Arc676
Copy link
Contributor

commented Jan 24, 2019

Employs the fix suggested by @UniversalSuperBox to automatically advance the setup wizard once a connection to a WiFi network is established.

Tested by editing the file locally and re-running the setup wizard on Nexus 5 (hammerhead) running 16.04 (2019-01-11), devel channel.

Closes ubports/ubuntu-touch#845.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

Looks like one of the test cases (testWizardSystem) isn't expecting this behavior, unless that test case was already failing before.

@UniversalSuperBox UniversalSuperBox self-assigned this Feb 8, 2019

@UniversalSuperBox
Copy link
Member

left a comment

You should also check if this is the currently active page, else re-connections will cause the next pages to be skipped.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

Another tip, you can check if the page is visible or not using the visible property of any item on the page.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Does the visible property match whether the page is active? Or can a page be visible behind another view?

Sorry for the inactivity; currently on my other partition. I'll probably switch back to Ubuntu to test this soon.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

The visible property is set for each page, and it propagates to every item on the page when set. Visible is set to true when the page is focused, false when it is left.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Added additional condition that wifiPage must be visible.

Performed the following tests and observed the following behavior by turning the personal hotspot on my other phone on and off at various times:

  • If the device is connected to a network before reaching the WiFi screen, that screen is skipped
  • If the device becomes connected to a known network without user interaction, the language screen is not skipped
  • If the user manually connects to a network, there is no need to press the "next" button; the device setup progresses automatically
  • If the device becomes connected while the WiFi screen is active without user interaction, the screen is automatically skipped

It's worth noting that due to the network refresh rate, networks that suddenly become available like in this test will not immediately appear. I had to wait a few seconds for the device to connect to my hotspot after I turned it on while on the WiFi screen. But this seems like an unlikely scenario for most end users.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

While testing in a device with a SIM inserted, I realized another problem -- any data connection triggers Connectivity.online...

Something must know if Wi-Fi is connected or not, since updates only check over Wi-Fi. Let's investigate more...

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

On further investigation (read: poking Rodney), Connectivity.limitedBandwidth should be false for Wi-Fi connections.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Gotcha. Added the extra condition for WiFi connections. I can confirm that connecting to a network still skips the network selection screen (specifically the fourth scenario in my last comment). Unfortunately I don't have a SIM card in my device so I can't check mobile data connections.

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Oh, I didn't think of putting it there! That fixes a new bug that I would have introduced were I making the changes.

That condition also needs to be added to skip:

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Out of curiosity (and also for a better understanding of the code), where were you thinking of making the changes and what was the bug?

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

This page gets automatically skipped whenever you are connected to any network because the skip: property is just set to Connectivity.online. I mistakenly attributed that to your changes.

You don't have to change it if you don't want to, but I'd appreciate it.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I see. I changed skip to be the exact same expression as the skip condition in c6941b0. Still tested and observed same behavior (although again with no mobile network test).

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Alright, now I see what's going on here better. skip is used to decide whether the page should be skipped before it is loaded. If skip is true, the page will never be visible. So, one more change is needed.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

I must admit I'm not sure what the necessary change is. I'm assuming for readability, it shouldn't include wifiPage.visible, but this didn't affect functionality. Regardless of whether the device was already connected before beginning setup or became connected before reaching the network selection page, the page was always skipped.

What am I missing?

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

wifiPage.visible should always be false when it's evaluated (it's deciding whether to show the page or not), so the page is always shown.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

That's very weird because the page got skipped on my device when it was connected before reaching the network selection screen. I'll try testing it again tonight.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Tested by turning on the hotspot on my other phone while the language selection screen was present. The device connected automatically to the network and after hitting "skip" on the "no SIM card" screen, it jumped straight to the time zone selection.

How might I be able to debug this?

@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Oh, that's a strange behavior. Pages get set to visible before they are made visible, while the Wizard is trying to decide whether to skip them or not.

So yes, you're right, this works normally... however it relies on visible being set when it really shouldn't be (the page isn't being shown, we're trying to figure out if we should skip it!). Even though it isn't really necessary, I'd prefer if && visible was removed from the skip property.

@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Makes sense. Future-proofing the condition it is then, in case a future version doesn't set visible to true before checking whether to skip. I've removed the condition.

@UniversalSuperBox
Copy link
Member

left a comment

And after all that, it seems like Connectivity.limitedBandwidth doesn't actually become true when connected to 4G... oh well, that's no reason to keep this from the codebase any longer. The skip condition works as designed, as does the automatic advance.

Thank you so, so much for working with me on this. I've learned a lot about the Wizard.

@UniversalSuperBox UniversalSuperBox merged commit 77d118d into ubports:xenial Mar 29, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@UniversalSuperBox

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Would you like to be mentioned by name (as listed on GitHub) in our changelog? The changelog will appear on the post releasing OTA-9, similar to the OTA-8 post: https://ubports.com/blog/ubports-blog-1/post/ubuntu-touch-ota-8-release-207

UniversalSuperBox added a commit that referenced this pull request Mar 29, 2019

Automatically advance setup wizard on WiFi connection (#92)
Employs the fix suggested by @UniversalSuperBox to automatically advance the setup wizard once a connection to a WiFi network is established.
@Arc676

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

It's been a pleasure. Thanks to you as well for all the input! And coming up with the original fix :) Hopefully with this as a base I might be able to better understand the codebase and make more significant contributions in the future.

Per the changelog: It'd be an honor to be mentioned. Thanks!

@Arc676 Arc676 deleted the Arc676:wizard-advance-on-wifi branch Mar 29, 2019

UniversalSuperBox added a commit that referenced this pull request Apr 1, 2019

Automatically advance setup wizard on WiFi connection (#92)
Employs the fix suggested by @UniversalSuperBox to automatically advance the setup wizard once a connection to a WiFi network is established.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.