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/Reschedule doesn't work on cards in filtered deck #3441

Conversation

L-M-Sherlock
Copy link
Contributor

fix #3439

A similar function may also need to be fixed:

fn write_deck_id_with_children(&mut self, deck_id: DeckId) -> Result<()> {
if let Some(parent) = self.col.get_deck(deck_id)? {
let ids = self.col.storage.deck_id_with_children(&parent)?;
let mut buf = String::new();
ids_to_string(&mut buf, &ids);
write!(self.sql, "c.did in {}", buf,).unwrap();
} else {
self.sql.push_str("false")
}
Ok(())
}

@dae
Copy link
Member

dae commented Sep 27, 2024

These are currently documented as not matching cards in filtered decks:

    /// Matches cards in a list of decks (original_deck_id is not checked).
    DeckIdsWithoutChildren(String),
    /// Matches cards in a deck or its children (original_deck_id is not
    /// checked).
    DeckIdWithChildren(DeckId),

I've done a brief scan over the existing usages, and didn't notice any cases where the change is likely to break things, but it's not how they were originally intended to behave, so I get a bit nervous about changing them. Any thoughts on this change @RumovZ?

@dae dae added this to the 24.10 milestone Sep 27, 2024
@RumovZ
Copy link
Collaborator

RumovZ commented Sep 28, 2024

Wouldn't that at least break (or alter) user searches containing did:...?

Looks like you've added DeckIdsWithoutChildren to avoid bugs by forcing explicitness. As it seems the original deck id is a similar source of issues, maybe it would be best to make that explicit, too:

DeckIds {
    ids: String,
    with_children: bool,
    with_filtered: bool,
}

@brishtibheja
Copy link
Contributor

Wouldn't that at least break (or alter) user searches containing did:...?

That isn't documented anywhere, so do users use this search? I think most wouldn't be aware.

@RumovZ
Copy link
Collaborator

RumovZ commented Sep 30, 2024

It's probably not used much, but might still be useful to add-ons and power users.
In any case, I don't see an argument for breaking any compatibility when it's so easy to support both use cases.

@dae
Copy link
Member

dae commented Oct 2, 2024

On making with_filtered explicit: my first thought was "good idea, why didn't I think of that?". :-) Then I noticed a few things:

  • One of our current two cases uses a number, not a string of numbers, so merging gets a little more complicated
  • The current behavior of not including filtered decks in a did:xxx search is inconsistent with a deck:xxx search
  • I can't think of many cases where one would want to exclude the filtered cards, and in those cases, it's easy to exclude them from the search. Whereas we provide no existing way to include filtered cards in the search when searching via deck id.

Given the above, I feel like changing the docs might be the simplest way forward. Merging without feedback to keep things moving forward, but happy to discuss/change/revert if you feel strongly.

@dae dae merged commit 159681d into ankitects:main Oct 2, 2024
1 check passed
@L-M-Sherlock L-M-Sherlock deleted the Fix/Reschedule-doesn't-work-on-cards-in-filtered-deck branch October 21, 2024 06:51
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.

[Bug] Reschedule doesn’t work on cards in filtered deck
4 participants