Skip to content

Part-2: Compiler Warnings as Errors - WordPress Module - FragmentManager#19490

Merged
antonis merged 12 commits intowordpress-mobile:trunkfrom
07jasjeet:17253-compiler-warnings-part-2
Nov 7, 2023
Merged

Part-2: Compiler Warnings as Errors - WordPress Module - FragmentManager#19490
antonis merged 12 commits intowordpress-mobile:trunkfrom
07jasjeet:17253-compiler-warnings-part-2

Conversation

@07jasjeet
Copy link
Copy Markdown
Contributor

@07jasjeet 07jasjeet commented Oct 31, 2023

ONLY merge this after Part 1 has been merged! (Derivative branch)

Fixes #17253 partially. (Sub-ticket of #17246)

To Test:

  • AddCategoryFragment.java:

    1. Have a site with a post.
    2. Navigate to "My Site" and click on "Posts" to see all your posts. Open any post in edit mode.
    3. Click on dropdown button on top-left.
    4. Click "Post settings".
    5. Select/Open "Categories".
    6. Click on "+" symbol on top left.
    7. A dialog should open without errors.
  • CollapseFullScreenDialogFragment.java:

    1. Navigate to "Reader" tab.
    2. On any post, click on "Comment" to open comments.
    3. On the bottom of the screen, you will have the text field to reply. Beside that, you will have an "Arrow up" button. Click it.
    4. This is our full screen dialog that got changed. It should work as expected.
  • SignupEpilogueFragment.java:

    1. Just follow the signup flow till you reach a form where you have to fill your name, display name and password.
    2. This is hard to test (and I need help here). The changes in this file are minute and are certainly not going to break anything afaik.
  • SitePickerActivity.java:

    1. Navigate to "My Site" tab.
    2. Click on "Add Site" button if you have no sites else, click on current opened site's title on top of the page (with an arrow).
    3. You will have "Choose site" screen infront of you.
    4. Click on "+" button on top right. (on Action bar)
    5. Dialog should work as expected.
  • PersonDetailFragment.java:

    1. Have at least one site.
    2. Navigate to "My Site" tab.
    3. Click on "More".
    4. Click on "People"
    5. Click on any person.
    6. A detailed view of the person should pop up as expected.

Regression Notes

  1. Potential unintended areas of impact
    Mostly none because the newer API's are a direct replacement.

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

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

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.
  • Talkback.
  • 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)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@07jasjeet 07jasjeet changed the title Part-2: Compiler Warnings as Errors - WordPress Module - FragmentManage Part-2: Compiler Warnings as Errors - WordPress Module - FragmentManager Oct 31, 2023
@07jasjeet
Copy link
Copy Markdown
Contributor Author

@antonis Please add yourself as a reviewer.

@antonis antonis self-requested a review October 31, 2023 16:14
}
} else {
mDialog = (FullScreenDialogFragment) getFragmentManager().findFragmentByTag(FullScreenDialogFragment.TAG);
mDialog = (FullScreenDialogFragment) getParentFragmentManager().findFragmentByTag(FullScreenDialogFragment.TAG);
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've noticed a minor checkstyle issue on this line:

Line is longer than 120 characters (found 124). [LineLength]

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.

Oops looks like the checkstyle error might have went out of screen as well haha. Will fix it!

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.

Also i will be leaving test directions soon so I would request you to wait on this one until then.

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.

@antonis test instructions are up!

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.

Thank you for the heads up @07jasjeet 🙇
My plan is to review the PR later today or tomorrow.

Needs no testing. The behaviour should be same as the new API's will throw IllegalStateException instead of NullPointerException if activity was null.
@07jasjeet 07jasjeet requested a review from antonis November 3, 2023 16:57
@antonis antonis added this to the 23.7 milestone Nov 7, 2023
1) Used requireArguments() instead of assertion.
@antonis antonis self-requested a review November 7, 2023 11:40
Copy link
Copy Markdown
Contributor

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this @07jasjeet and for handling the review feedback 🙇
I've tested the implementation and everything worked as expected for me:

  • AddCategoryFragment.java
  • CollapseFullScreenDialogFragment.java
  • SignupEpilogueFragment.java
  • SitePickerActivity.java
  • PersonDetailFragment.java
  • FullScreenDialogFragment.java

The code also looks good 🎉

@antonis antonis merged commit a453626 into wordpress-mobile:trunk Nov 7, 2023
@07jasjeet 07jasjeet deleted the 17253-compiler-warnings-part-2 branch November 7, 2023 12:17
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.

Compiler Warnings as Errors - WordPress Module - FragmentManager

2 participants