Skip to content

Conversation

MohamedAsli
Copy link
Contributor

Description

Fixes multiselect bug in SelectFromList where only selections from the currently active group were returned, losing selections from other groups.
This behavior seems unintended, as there's no logical reason for selections to be lost when switching between groups.

What I've changed:

  • Added _get_all_ctx() helper method to return items from all groups
  • Modified _get_options() to use _get_all_ctx() instead of _get_active_ctx() in multiselect mode
  • Preserves existing behavior for non-grouped contexts and single-select

No regressions detected as far as I tested


Checklist

  • Code follows the PEP 8 style guide.
  • Code has been formatted with Black
  • Changes are tested and verified to work as expected.

Related Issues

#2286

…ntexts (not only filtered or currently displayed group)
Copy link
Contributor

@devloai devloai bot left a comment

Choose a reason for hiding this comment

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

PR Summary:

Fixes multiselect bug in SelectFromList where only selections from the currently active group were returned, losing selections from other groups. Adds _get_all_ctx() helper method and modifies _get_options() to use it instead of _get_active_ctx() in multiselect mode.

Review Summary:

The fix correctly addresses the reported issue and the implementation is technically sound. However, this introduces a breaking change that contradicts explicit maintainer guidance from issue #2286, where @sanzoghenzo recommended implementing this via an optional parameter to maintain backward compatibility. The current approach changes default behavior for existing users.

Follow-up suggestions:

  • @devloai implement backward compatibility using an optional parameter as suggested by the maintainer
  • @devloai add the new parameter to class docstring and setup method

@MohamedAsli
Copy link
Contributor Author

MohamedAsli commented Sep 15, 2025

Got inspired by @onomou's snippet, with a minor difference :

we always should use _get_all_ctx() even if return_all == false, because this argument is only meant to return all items values even if they are unchecked, in order to get their respective states.

Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

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

Devlo misunderstood things a little, here's my two cents...

@MohamedAsli
Copy link
Contributor Author

@jmcouffin should we merge this ?

@jmcouffin
Copy link
Contributor

jmcouffin commented Sep 22, 2025

@MohamedAsli If you resolved the latest comment from Andrea, yes

@MohamedAsli
Copy link
Contributor Author

Sorry for the late reply, I got sidetracked, then forgot about the remaining modification

@sanzoghenzo
Copy link
Contributor

My focus on pythyon 3.10+ collides with pyRevit 😅 We can ignore my last comment and proceed with the merge!

@MohamedAsli
Copy link
Contributor Author

Your comment came in just as I pushed my commit :)
We can include the generator anyway now that it's done

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.

3 participants