-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Grouping into enum allows to switch on enum and ensure compiler notifies about any places that need to be updated
case AppConfiguration.Widget.Stats.lockScreenTodayViewsProperties: | ||
return .todayViewsLockScreenWidgetUpdated | ||
default: | ||
return .noEvent |
There was a problem hiding this comment.
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.
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr21533-6b88167 | |
Version | 23.2 | |
Bundle ID | org.wordpress.alpha | |
Commit | 6b88167 | |
App Center Build | WPiOS - One-Offs #7017 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr21533-6b88167 | |
Version | 23.2 | |
Bundle ID | com.jetpack.alpha | |
Commit | 6b88167 | |
App Center Build | jetpack-installable-builds #6061 |
There was a problem hiding this 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.
Thanks for testing, @guarani !
I agree that it's a tricky, always frustrating experience with debugging the extension. However, the debugger doesn't disconnect for me.
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
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. |
Lock Screen Widget are all of unique kind and only have one type for now
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 I also saw complaints about For now, I'd say it would be better not to track |
There was a problem hiding this 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 wrongrectangular_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 👍
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.
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 thisenum
need to be implemented. Now, when a new widgetKind
will be added,ExtensionEvents
will ask to provide additional track events.To test:
I used these steps for testing:
FeatureFlag.swift
to returntrue
for.lockScreenWidget
Regression Notes
Breaking current widget implementation. However, I didn't change any existing keys, only where they are stored.
Manual testing
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: