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

Lock Screen Widgets: Add Tracks #21533

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Sep 8, 2023

Fixes #21531

Adding tracking for all lock screen widgets. Expanding current tracking implementation which tracks when widget is newly added.

As part of this PR, refactoring configuration so all the different widget configurations would be defined in enum. This gives the benefit of the compiler notifying when places relying on this enum need to be implemented. Now, when a new widget Kind will be added, ExtensionEvents will ask to provide additional track events.

To test:

I used these steps for testing:

  1. Launch Console app
  2. Action -> Enable Info messages
  3. Edit FeatureFlag.swift to return true for .lockScreenWidget
  4. Launch Jetpack
  5. Login
  6. Open Lock Screen
  7. Add Jetpack widget
  8. Confirm the event is logged in Console app

Regression Notes

  1. Potential unintended areas of impact

Breaking current widget implementation. However, I didn't change any existing keys, only where they are stored.

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

Manual testing

  1. 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.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

case AppConfiguration.Widget.Stats.lockScreenTodayViewsProperties:
return .todayViewsLockScreenWidgetUpdated
default:
return .noEvent
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 is one of the reasons of this refactoring. I want switch to happen on a type, such as Widget.Stats.Kind so compiler would notify about expanding tracking when we create a new widget kind.

@staskus staskus changed the title Lock Screen Widget: Add Tracks Lock Screen Widgets: Add Tracks Sep 8, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Sep 8, 2023

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 Numberpr21533-6b88167
Version23.2
Bundle IDorg.wordpress.alpha
Commit6b88167
App Center BuildWPiOS - One-Offs #7017
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Sep 8, 2023

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 Numberpr21533-6b88167
Version23.2
Bundle IDcom.jetpack.alpha
Commit6b88167
App Center Buildjetpack-installable-builds #6061
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@staskus staskus mentioned this pull request Sep 8, 2023
4 tasks
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I like how you've turned the widget identifiers into an enum so any new widgets get Tracks events added as you mention above.

It's turning out to be tricky to verify these events are logged correctly. After I attach the debugger to the extension, the breakpoint I have in the logging code is hit. However, as I add more widgets to check their logging, the debugger soon disconnects with the message Message from debugger: Terminated due to signal 9.

I tried instead to log the Tracks events:

print("🔵 Tracked: ", event, " Properties: ", properties as Any)

This print statement is fired correctly, but the counts are off. For example, adding a single widget results in a count of two instead of one for both for total_widgets as well as rectangular_widgets:

(lldb) po properties
▿ Optional<Dictionary<String, AnyObject>>
  ▿ some : 5 elements
    ▿ 0 : 2 elements
      - key : "total_widgets"
      - value : 2
    ▿ 1 : 2 elements
      - key : "small_widgets"
      - value : 0
    ▿ 2 : 2 elements
      - key : "large_widgets"
      - value : 0
    ▿ 3 : 2 elements
      - key : "rectangular_widgets"
      - value : 2
    ▿ 4 : 2 elements
      - key : "medium_widgets"
      - value : 0

When I add another widget or step execution to see where the value is coming from, the debugger often detaches or the breakpoint isn't hit. Is this also the case for you? I'm keen to hear if there's a better way to do this.

@staskus
Copy link
Contributor Author

staskus commented Sep 11, 2023

Thanks for testing, @guarani !

It's turning out to be tricky to verify these events are logged correctly. After I attach the debugger to the extension, the breakpoint I have in the logging code is hit. However, as I add more widgets to check their logging, the debugger soon disconnects with the message Message from debugger: Terminated due to signal 9.

I agree that it's a tricky, always frustrating experience with debugging the extension. However, the debugger doesn't disconnect for me.

I tried instead to log the Tracks events:

  • print statements don't work for me with a debugger attached and WidgetKit Developer Mode enabled in Developer settings.
  • OSLog does work for me, showing the logging in Console app

When looking at the original PR (#15456) all the testing was done directly in Tracks.

I could include OSLog in the code for easier testing. Then logging could be observed in the Console app without dealing with debugger issues in the extensions.

This print statement is fired correctly, but the counts are off. For example, adding a single widget results in a count of two instead of one for both for total_widgets as well as rectangular_widgets:

Yes, you're right. There's something wrong happening with this count.

First of all, home widgets behave a bit differently compared to lock screen widgets. With home widgets, we can have one widget kind with multiple sizes. However, with lock screen widgets we can have one size per widget kind.

I reproduced a higher count shown after changing the site. After that, it looks to come back to the correct value. I'll investigate the issue.

@staskus
Copy link
Contributor Author

staskus commented Sep 11, 2023

WooCommerce uses a different way to track installed widgets, by logging all the installed widgets at the same time when the application is opened

I see similar approaches in equivalent apps which log all the installed widgets in when getTimeline is triggered.

I also saw complaints about WidgetCenter.shared.getCurrentConfigurations returning not only currently visible configurations but all the configurations that were ever configured. It probably has a caching mechanism with which we also get the wrong rectangular_widgets count.

For now, I'd say it would be better not to track properties with lock screen widget events, given they wouldn't provide meaningful information. And going forwarding, it may be good to experiment with alternative widget information logging mechanisms.

@staskus staskus requested a review from guarani September 11, 2023 11:30
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

WooCommerce uses a different way to track installed widgets, by logging all the installed widgets at the same time when the application is opened

I see similar approaches in equivalent apps which log all the installed widgets in when getTimeline is triggered.

I also saw complaints about WidgetCenter.shared.getCurrentConfigurations returning not only currently visible configurations but all the configurations that were ever configured. It probably has a caching mechanism with which we also get the wrong rectangular_widgets count.

Good finds! Those alternative approaches look interesting.

I tested the change and see the events in the COnsole app without the properties, as expected 👍
Screenshot 2023-09-11 at 19 56 42

For some reason I haven't figured out, the events didn't get logged in Console app when I did more testing. The Console app seems to get stuck and changing the order of messages (e.g. by clicking the Time column header) works sometimes and not other times in my testing today. This suggests to me the Console app might not be working properly.

Given the events fired and the code changes to remove the properties from these events look good, I'm going to approve.

@staskus staskus merged commit 801a919 into trunk Sep 12, 2023
@staskus staskus deleted the task/21531-lock-screen-widgets-update-tracks branch September 12, 2023 05:57
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.

Lock Screen Widgets: Update Tracks
3 participants