Skip to content
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

fix(listview): update index after items removed #5135

Conversation

TomJGooding
Copy link
Contributor

@TomJGooding TomJGooding commented Oct 17, 2024

Fixes #5114.

  • The pop and remove_items methods now return AwaitComplete rather than AwaitRemove to account for the index update
  • Previously trying to pop() from an empty list would raise a NoMatches error, this will now raise an IndexError for consistency.

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@TomJGooding
Copy link
Contributor Author

I've now also updated the remove_items method, so hopefully I was on the right track!

@TomJGooding TomJGooding marked this pull request as ready for review October 17, 2024 22:01
@TomJGooding
Copy link
Contributor Author

This now also reinstates always_update to the index reactive that was removed in recent changes. When items are removed, the index may not change but the highlighted item should be updated.

@willmcgugan
Copy link
Collaborator

I'd recommend against the always_update. Since it calls watchers you can get unintended side effects, such as re-sending messages.

Better to factor out the specific functionality you need and call it directly.

TBH I regret adding always_update. I know we've recommended it in the past, but this will rightly confuse anything not intimately familiar with reactives:

self.index = self.index

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Just a request for docstrings...

item_to_remove = self.query("ListItem")[index]
normalized_index = index if index >= 0 else index + len(self)

async def do_pop():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring and typing por favor. Since its internal, and essentially private, the docstrings can be written for future Textual core-devs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added docstrings in 9314cde, but feel free to tweak!

index if index >= 0 else index + len(self) for index in indices
)

async def do_remove_items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstrings please

@willmcgugan
Copy link
Collaborator

Hi Tom, is this ready to go?

@TomJGooding
Copy link
Contributor Author

@willmcgugan If you're happy with my approach and the added tests cover all cases, I think this should be ready to go.

@darrenburns
Copy link
Member

darrenburns commented Nov 28, 2024

@willmcgugan I was going to merge, but it's blocked until you approve.

@darrenburns darrenburns merged commit 8c8057b into Textualize:main Nov 28, 2024
20 checks passed
@TomJGooding TomJGooding deleted the fix-listview-update-index-after-items-removed branch November 30, 2024 09:58
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.

Calling remove_items or pop on a ListView does not update index or highlighting
3 participants