Skip to content

Re-implement building page tree hierarchy - #22345

Merged
mokagio merged 10 commits into
release/24.0from
fix-issue-22283-page-list-hierachy
Jan 9, 2024
Merged

Re-implement building page tree hierarchy#22345
mokagio merged 10 commits into
release/24.0from
fix-issue-22283-page-list-hierachy

Conversation

@crazytonyli

Copy link
Copy Markdown
Contributor

Fixes #22283. This PR is a follow-up of #22338.

While working on the issue yesterday, probably due to having a holiday break which made me look back at the implementation, I realised that I had over-engineered the implementation and made it unnecessarily complicated.

There is a big undocumented intention of using and maintaining a PageTree struct. I thought it'd be nice to use a PageTree struct instance throughout the lifecyle of the Pages List, where we can add/remove pages to the instance as new pages are loaded from the server or when user deletes a page. That's why I didn't go with a pure function that turns a list of pages into another lists of pages.

But since we are really hold onto PageTree struct instances in the view controller, and I don't foresee we are going to do that in any time soon, I decided to scratch the implementation and re-implementing using a pure function, which only has 30 lines of code and is pretty easy to read.

Also, the new algorithm is much faster (about x3 times faster) and has 0(n) time complexity, because it builds an index of parent-child page before assembling them into a tree.

Regression Notes

  1. Potential unintended areas of impact
    None.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Verified Pages are not displayed in a hierarchy #22283

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

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: N/A

@crazytonyli crazytonyli added this to the 24.0 ❄️ milestone Jan 8, 2024
@crazytonyli
crazytonyli requested review from kean and mokagio January 8, 2024 22:31
@peril-wordpress-mobile

peril-wordpress-mobile Bot commented Jan 8, 2024

Copy link
Copy Markdown
Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone
⚠️

This PR contains changes to RELEASE_NOTES.txt.
Note that these changes won't affect the final version of the release notes as this version is in code freeze.
Please, get in touch with a release manager if you want to update the final release notes.

Generated by 🚫 dangerJS

@crazytonyli

Copy link
Copy Markdown
Contributor Author

Note that these changes won't affect the final version of the release notes as this version is in code freeze.
Please, get in touch with a release manager if you want to update the final release notes.

@mokagio , this PR targets 24.0 and I think it may be worthy while to update the release notes to include this bug fix?

@dangermattic

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ This PR is assigned to the milestone 24.0 ❄️. The due date for this milestone has already passed.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@crazytonyli crazytonyli mentioned this pull request Jan 8, 2024
13 tasks
@wpmobilebot

Copy link
Copy Markdown
Contributor
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 Numberpr22345-55fbfdb
Version23.9
Bundle IDorg.wordpress.alpha
Commit55fbfdb
App Center BuildWPiOS - One-Offs #8372
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot

Copy link
Copy Markdown
Contributor
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 Numberpr22345-55fbfdb
Version23.9
Bundle IDcom.jetpack.alpha
Commit55fbfdb
App Center Buildjetpack-installable-builds #7394
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

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

Screenshot 2024-01-09 at 12 07 00

👏

@mokagio

mokagio commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

@mokagio , this PR targets 24.0 and I think it may be worthy while to update the release notes to include this bug fix?

Sure. We're in luck in that the release notes haven't been sent to editorialization yet, so anything goes 😄

@mokagio
mokagio merged commit 37bc0df into release/24.0 Jan 9, 2024
@mokagio
mokagio deleted the fix-issue-22283-page-list-hierachy branch January 9, 2024 01:08
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.

4 participants