Skip to content

Initial changes to make the Today Home Widget customizable. - #15397

Merged
diegoreymendez merged 30 commits into
feature/ios-14-today-widgetfrom
try/customizable-today-widget
Dec 2, 2020
Merged

Initial changes to make the Today Home Widget customizable.#15397
diegoreymendez merged 30 commits into
feature/ios-14-today-widgetfrom
try/customizable-today-widget

Conversation

@diegoreymendez

@diegoreymendez diegoreymendez commented Nov 26, 2020

Copy link
Copy Markdown
Contributor

First attempt at making our Today Home Widget customizable.

In this PR you can:

  • Customize the widget to select which site to show the stats for.
  • See the site stats in the widget.

This PR also adds a new target (the Intents Extension) together with provisioning profiles and App ID.

About the number of lines changed:

The PR is quite large, with around 2000 lines added. Most of these, though, are changes to add the new intents target and the certificates. The code changes aren't that many.

Known limitations:

  • This PR doesn't change the stats refresh logic so the info may not be up to date at all times.
  • Remote icons for sites aren't loading yet: Apple's default mechanism for downloading remote images in the intents extension is not working at all, and I'm a bit stuck trying to figure out the best solution.
  • It seems we're showing hidden sites too. We should probably filter the list of sites shown to visible ones.

To test:

  1. Long tap on the widget.
  2. Tap "Edit Widget"
  3. Tap "Choose" in the "Site" row.
  4. Select a site from your list.

Make sure the site you selected is properly loaded in the widget.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@peril-wordpress-mobile

peril-wordpress-mobile Bot commented Nov 26, 2020

Copy link
Copy Markdown

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile

peril-wordpress-mobile Bot commented Nov 26, 2020

Copy link
Copy Markdown
Warnings
⚠️ PR is not assigned to a milestone.
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@diegoreymendez diegoreymendez changed the title Try/customizable today widget Initial changes to make the Today Home Widget customizable. Nov 26, 2020
@peril-wordpress-mobile

peril-wordpress-mobile Bot commented Nov 26, 2020

Copy link
Copy Markdown

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@diegoreymendez

Copy link
Copy Markdown
Contributor Author

Update: I've pushed more changes into this PR so that the site you select is shown in the widget.

@frosty frosty 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.

Functionally, this is working as expected – I was able to see my sites in the list and select one, and that selection was saved. I've left some code level comments, and I have a few other things:

  • I'm not sure we need the site icon on the configuration view – particularly if a site has a longer title, we might not have space anyway 🤔

image

  • The sites on the list don't appear to be in any specific order, and it seems to be randomized each time
  • In the main app, we show the site's domain as a subtitle on the cell – is that possible here?
  • Do we need the IntentsUI target for this? I was under the impression that was only for use with Siri or Maps, but I may not understand it correctly.

Comment thread WordPress/Info.plist Outdated
<key>CFBundleExecutable</key>
<string>${EXECUTABLE_NAME}</string>
<key>CFBundleIcons~ipad</key>
<key>CFBundleIcons</key>

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.

I think it's just moving items in the plist around automatically, but I'm not sure these icon changes really belong in here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've rolled back all changes to this Info.plist since none of these changes done manually. I believe the one to trigger all the changes was the NSUserActivityTypes one.

I've re-tested my PR without that change and it seems to work just fine. Can I ask you to double check this @frosty ?

Comment thread WordPress/Info.plist Outdated
Comment thread WordPress/WordPressHomeWidgetToday/WordPressHomeWidgetToday.swift Outdated
Comment thread WordPress/WordPressHomeWidgetToday/WordPressHomeWidgetToday.swift
Comment thread WordPress/WordPressIntents/IntentHandler.swift
@diegoreymendez

Copy link
Copy Markdown
Contributor Author

I'm temporarily converting this back to a draft and removing the reviewers, as I fix the merge issues. Please hold any reviews.

@diegoreymendez
diegoreymendez removed the request for review from Gio2018 November 30, 2020 16:29
@diegoreymendez

diegoreymendez commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

The merge conflicts are resolved, but I'll keep this as draft while I address @frosty 's feedback and also check why it is that site icons are NOT loading in the site selection screen for the widget.

@diegoreymendez

Copy link
Copy Markdown
Contributor Author

Gio - this is ready for review. I'll be addressing @frosty 's feedback in the meanwhile.

@@ -0,0 +1,96 @@
<?xml version="1.0" encoding="UTF-8"?>

@diegoreymendez diegoreymendez Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Folks, I'm not sure if this scheme should be shared or not. IMHO there's no reason why schemes shouldn't be shared but if either of you knows better, can you let me know?

PS: Regardless of my doubt, I still haven't shared the others on purpose. Not sure what happened there!

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.

I'm not sure either, although it appears we don't currently share any schemes (although it's also not in our gitignore)... @jkmassel any ideas?

Comment thread WordPress/WordPressIntents/IntentHandler.swift Outdated
Comment thread WordPress/WordPressIntents/IntentHandler.swift Outdated
Comment thread WordPress/WordPressIntents/IntentHandler.swift Outdated
@diegoreymendez

diegoreymendez commented Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Just for context, the rendering issue in the main Widget customization screen seems to be a bug.

I'll try to work around it. That just didn't work. I'm turning off site icons for now.

@diegoreymendez
diegoreymendez requested a review from frosty December 1, 2020 15:25
@diegoreymendez

Copy link
Copy Markdown
Contributor Author

This is ready for a full review. Please make sure to test the Widget is updating properly, following the steps described here.

@diegoreymendez

Copy link
Copy Markdown
Contributor Author

Replying to @frosty :

I'm not sure we need the site icon on the configuration view – particularly if a site has a longer title, we might not have space anyway 🤔

I just removed the images for now since I couldn't fix the bug you reported. It seems to be an SDK bug.

The sites on the list don't appear to be in any specific order, and it seems to be randomized each time

Will work on this in another PR. We also have to remove the hidden sites.

In the main app, we show the site's domain as a subtitle on the cell – is that possible here?

Done.

Do we need the IntentsUI target for this? I was under the impression that was only for use with Siri or Maps, but I may not understand it correctly.

Removed.

@Gio2018 Gio2018 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.

Looks good to me! I installed the widget, chose different sites and observed stats updating for that site either when the app received new stats or when the widget refreshed the timeline (had to tweak the refresh interval for that).

just left a few minor comments in the code, otherwise :shipit:!

Comment thread WordPress/Classes/Stores/StatsInsightsStore.swift Outdated
Comment thread WordPress/WordPressHomeWidgetToday/WordPressHomeWidgetToday.swift Outdated
Comment thread WordPress/WordPressHomeWidgetToday/WordPressHomeWidgetToday.swift
@diegoreymendez

Copy link
Copy Markdown
Contributor Author

Hey @frosty , @Gio2018 - Since this is targetting a feature branch and Giorgio already approved, I'm moving ahead to merge.

Please feel free to add comments on this PR and I'll address them in a separate PR.

@jkmassel - Please let me know what you think about this: #15397 (comment)

@diegoreymendez
diegoreymendez merged commit a583f5f into feature/ios-14-today-widget Dec 2, 2020
@diegoreymendez
diegoreymendez deleted the try/customizable-today-widget branch December 2, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants