Skip to content

Lock Screen Widgets: Enable feature - #21535

Merged
guarani merged 10 commits into
trunkfrom
task/lock-screen-widgets-enable-feature-flag
Sep 15, 2023
Merged

Lock Screen Widgets: Enable feature#21535
guarani merged 10 commits into
trunkfrom
task/lock-screen-widgets-enable-feature-flag

Conversation

@staskus

@staskus staskus commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

Enabling lock screen widgets by removing widget-related feature flags

To test:

All the functionality was already tested in other PRs. Confirm that lock widgets can be added after launching the app.

More testing steps: pc8eDl-14z-p2#comment-861

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@staskus
staskus marked this pull request as ready for review September 8, 2023 13:15
@wpmobilebot

wpmobilebot commented Sep 8, 2023

Copy link
Copy Markdown
Contributor
WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21535-7a3e9c9
Version23.2
Bundle IDorg.wordpress.alpha
Commit7a3e9c9
App Center BuildWPiOS - One-Offs #7107
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot

wpmobilebot commented Sep 8, 2023

Copy link
Copy Markdown
Contributor
Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21535-7a3e9c9
Version23.2
Bundle IDcom.jetpack.alpha
Commit7a3e9c9
App Center Buildjetpack-installable-builds #6151
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus
staskus requested a review from guarani September 12, 2023 06:48

@guarani guarani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @staskus! The PR itself looks great. I did some testing to get a sense of whether this feels ready to ship and ran into some issues that I'm not sure how to diagnose.

Testing debug builds from Xcode

Widget asking me to add a site

  1. Fresh install on iOS simulator after resetting the sim
  2. Log into a testing WP.com account
  3. Add widgets
  4. Widgets say "Create or add a site to see today's stats" despite being logged into a WP.com account

Reproduced on iPhone simulator (iOS 16.4) and iPhone physical device (iOS 17 beta). Switching the site in the widget's site list didn't work either to resolve this.

Untitled.mp4

Unable to add widget

  1. Fresh install on iOS device running iOS 17 beta
  2. Log into a testing WP.com account
  3. Tapped widget preview to add widget but it does nothing
  4. Also note the widget previews are either blank or show a ghost loading state
  5. Dragged widget preview over to widget area (widget or lock screen itself appears to crash, see below screen recording)
  6. Re-opening the widget list, Jetpack is no longer in the list of apps
  7. I restarted the device and I was able to add widgets

💡 Takeaway: iOS might "delist" apps from the list of available widgets and restarting the device seems to resolve this

RPReplay_Final1694558080.MP4

Widget showing zero stats

  1. Fresh install on iOS device running iOS 17 beta
  2. Log into a testing WP.com account
  3. Note that there was a "leftover" widget on the lock screen despite this being a fresh install (seems like an iOS bug unrelated to our widgets)
  4. Tapped widget preview to add the All-Time Views widget
  5. The widget was showing zero
  6. I tapped the widget, added the All-Time card, and noted there were 14 views
  7. Widget now shows 14 views

Testing release builds via App Center

I felt like a lot of this could be related to debug builds so I downloaded the build from App Center (I vaguely remember Home screen widget issues on debug builds that weren't present on production builds). Oddly, no widgets appeared until I restarted the device (this again could be an iOS 17 beta issue).

Here things worked better but this issue of it showing zero for stats persisted. Here's a screen recording (same steps to reproduce as above "Widget showing zero stats"):

RPReplay_Final1694562402.MP4

What are your thoughts on this? I'm unsure if it's:

  • An iOS 17 beta issue (I don't have a non-beta device handy right now)
  • Reproducible by others (keen to hear if you can reproduce)
  • A temporary issue likely to go away after the user opens the app
  • An actual bug in the widget code (I haven't tried to debug this as I spent time testing to narrow things down to get steps to reproduce)

@peril-wordpress-mobile

Copy link
Copy Markdown
Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

…themselves

After removing iOS13 today widget-related code, we also removed a commonly reused credential storing functionality. Re-adding it to StatsWidgetsStore
@staskus

staskus commented Sep 14, 2023

Copy link
Copy Markdown
Contributor Author

@guarani

Widget showing zero stats

I was able to reproduce it and diagnose the issue. It is a regression from #21521. StatsWidgetService reused credentials stored by TodayExtensionService to make network calls to update widget data by itself. I added this functionality in this commit 81ce636 so the issue should be resolved.

Widget asking me to add a site

I still haven't been able to reproduce it. I remember similar issues previously with an account that didn't have a default site set but nothing beyond that. Could you try to see if it's reproducible with some other account?

Unable to add widget

I never encountered such a thing. I will try to look for some explanations online.

@staskus
staskus requested a review from guarani September 14, 2023 10:40
@guarani

guarani commented Sep 14, 2023

Copy link
Copy Markdown
Contributor

I was able to reproduce it and diagnose the issue. It is a regression from #21521. StatsWidgetService reused credentials stored by TodayExtensionService to make network calls to update widget data by itself. I added this functionality in this commit 81ce636 so the issue should be resolved.

It's great to hear you could reproduce and find a fix ❤️ Regarding the other issues, they were not reproducible on Release builds, so perhaps they are non-issues. I think this can be merged if the zero stats issue is fixed, I'll review it today and hopefully, we can get this merged for Monday's beta 🤞

@staskus

staskus commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

@guarani
I was testing widgets again.

One thing to note is that calling reloadTimelines does not mean that the widget will be immediately refreshed, that is for the system to decide. That is one thing I noticed when testing widgets, the time between calling reloadTimelines and widget reloading code can vary. Sometimes it happens immediately, sometimes it takes up to a minute.

I captured a sequence here:

  1. 00:00 Log out + reloadAllTimelines is called
  2. 🕥 00:27 widget reload happens to change it to loggedOut state
  3. 00:56 Log in + reloadAllTimelines is called
  4. 🕥 01:22 widget reload happens to change it to widgetData state
  5. 01:54 Log out + reloadAllTimelines is called
  6. ✅ 01:56 widget reload happens immediately to change it to loggedOut state
Reload.Timelines.Delay.720p.mov

I made little improvements based on Apple recommendations:

If your app uses WidgetBundle to support multiple widgets, you can use WidgetCenter to reload the timelines for all your widgets. For example, if your widgets require the user to sign in to an account but they have signed out, you can reload all the widgets by calling:
WidgetCenter.shared.reloadAllTimelines()

Since we're using WidgetBundle, I'm using reloadAllTimelines when we're reloading widgets due to account changing state.

- In rare cases with no default site, simply show the first site in the data
- Restructured WidgetDataReader to only use a single method for snapshot and timeline for consistent data loading logic
@staskus

staskus commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

I couldn't reproduce more issues myself but I just went through the code to see any other cases or code paths that could cause issues.

3d30474:

  • In rare cases with no default site, simply show the first site in the data
  • Restructured WidgetDataReader to only use a single method for snapshot and timeline for consistent data loading logic

@guarani

guarani commented Sep 15, 2023

Copy link
Copy Markdown
Contributor

Sorry I didn't review this yesterday!

Thanks for the changes you made since yesterday, I'll review this today. If it helps, I can merge this after reviewing to get this in the beta.

@staskus

staskus commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

Sorry I didn't review this yesterday!
Thanks for the changes you made since yesterday, I'll review this today.

@guarani thanks! I'm still trying to figure out if my changes are causing UI test failures or are they flaky. They haven't passed since I merged trunk.

Update:
Good news, it was just fixed: #21582

If it helps, I can merge this after reviewing to get this in the beta.

Of course. If any issues come up, then we'll delay for 23.4. I will then only make a PR on Monday to merge 81ce636 to 23.3.

@guarani guarani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the work you put into this! I re-tested and it's working well!
I used a Debug build built from Xcode to add widgets, switch sites, and log into different accounts and the widgets worked well.

@guarani

guarani commented Sep 15, 2023

Copy link
Copy Markdown
Contributor

I'll merge this later today unless you do first 🙇

@guarani
guarani merged commit 6ac3851 into trunk Sep 15, 2023
@guarani
guarani deleted the task/lock-screen-widgets-enable-feature-flag branch September 15, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants