Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Conversation

@hassaansaleh
Copy link
Contributor

@hassaansaleh hassaansaleh commented Dec 3, 2021

Description

There was some code targeting iOS versions prior to iOS 13. And since the deployment target is iOS 13, these instances can safely be removed. This PR removes these instances. The changes in this PR introduces no visible changes, it's more of a cleanup.
The changes include:

  • Removal of unneeded @available and if #available instances.
  • Refactoring GravatarEmailTableViewCell and TextFieldTableViewCell to remove onChangeSelectionHandler and instead depend on UITextFieldDelegate method textFieldDidChangeSelection which is available staring iOS 13.

References: wordpress-mobile/WordPress-iOS#17516

Testing

To test this use the WordPress iOS App, but make sure to replace this line in the Podfile
pod 'WordPressAuthenticator', '~> 1.42.1'
With this
pod 'WordPressAuthenticator', :git => 'https://github.com/hassaansaleh/WordPressAuthenticator-iOS.git', :branch => 'issue/17516-remove-iOS12-code'

What to test

  1. Check that interacting with textfields is working correctly in the following screens:
    1. GetStartedViewControler
    2. PasswordViewController
    3. SiteCredentialsViewController
    4. SiteAddressViewController
  2. For PasswordViewController, check when the email is changed from a Password Manager, that it's handled correctly.

@diegoreymendez diegoreymendez self-assigned this Dec 7, 2021
@diegoreymendez diegoreymendez self-requested a review December 7, 2021 09:44
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

I think the cleanup of the code that's for versions of iOS earlier than 13 looks good. It's also in-line with the main intent behind the PR.

I'm not so sure about the change from onChangeSelectionHandler to textFieldDidChangeSelection, for a few reasons:

  • This goes beyond cleaning up code that's not used anymore, as it's modifying live code.
  • Tracking editing-changed is not semantically equivalent to tracking selection-changed. While both may coincidentally work for us, the first type of event is what we're aiming to track. As an example: I can trigger textFieldDidChangeSelection by moving the caret around in the text field, but that shouldn't result in a call to handleEmailFieldDidChange.
  • And last but not least: this code is a bit of a critical part of our Apps, and the pre-existing code works well. I'd be wary to make changes that could introduce problems.

Let me know what you think.

@hassaansaleh
Copy link
Contributor Author

@diegoreymendez Well the motivation behind these changes were based on a comment I found in the code, it felt like this was the original intent for whoever wrote this part.

When we no longer support iOS 12, textFieldDidChangeSelection, and onChangeSelectionHandler can be deleted in favor of adding the delegate method to view controllers.

However I feel that your concerns are totally valid, these changes do diverge from the current task at hand. So will revert them 👍

@diegoreymendez
Copy link
Contributor

Thanks @hassaansaleh: that comment perfectly explains the direction you took in the PR.

Also thanks for understanding and rolling the code back.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

This PR is good to go, nice work! :)

@diegoreymendez diegoreymendez merged commit 01b3f61 into wordpress-mobile:feature/remove-1password-framework-usage Dec 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants