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

Correct edition of card in filtered deck #5531

Conversation

Arthur-Milchior
Copy link
Member

@Arthur-Milchior Arthur-Milchior commented Oct 16, 2019

I believe that this pull request corrects issue #3200, issue #3409, #5222 and
issue #5184(this is the best report, it gives a LOT of details) which
seems to be thrice the same trouble.

The note editor contains a list of deck. This allows to select in
which deck to put the cards of the new notes. It also allows to move
the current card if it already exists (i.e. if we edit a card while
reviewing it). The default value finally allows to knows the deck of
the card (assuming the card exists).

The trouble being that the deck of the card is not necessarily in the
list of decks. In this card, the editor believes that we want to
change the deck of the card; which is not the case.

Thus this pull request simply add the deck of the current card to the
list of deck. Note that instead the current deck is not anymore added
to the list. However, it creates no trouble because:

  • either the current deck is the deck of the card, in which case it is
    in the added to the list since it's the deck of the card.
  • it is a parent of the deck of the card, hence it's not filtered,
    hence it's already in the list.

So actually this pull request only ensure that one more deck is added
in the case where the card reviewed is in a filtered deck, which is a
children of the selected deck. In any other case it does not change
anything.

I must state that I believe that ankidroid should not behaves as it
behaves now. Indeed, the fact that there is a single filtered deck in
the list means that choosing this deck has a meaning different from
the meaning of choosing any other deck. However, the GUI does not
emphasize the fact that the meaning of the filtered deck is different
from the meaning of the other decks.

However, even if I don't like this behavior, it is the current
behavior when you edit a card and the selected deck is filtered. So I
believe that it should remains the behavior when the deck of the card
is filtered, even if it's not the selected deck.

Fixes

#3200, issue #3409 and issue #5184(this is the best report, it gives a LOT of details)

Approach

By ensuring that the deck of the current card is in the list of decks.

How Has This Been Tested?

I did test this commit by following the steps of
#5184 (comment)
and checking that the problem did disappear. Furthermore, editing any
other card does not create any trouble.

Learning (optional, can help others)

That writing a message in a bug report such as #3409 (comment) is not enough, and that I've got to correct the bug myself.

Not sure it can help others actually.

Checklist

Please, go through these checks before submitting the PR.

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code

I believe that this pull request corrects issue ankidroid#3200, issue ankidroid#3409 and
issue ankidroid#5184(this is the best report, it gives a LOT of details) which
seems to be thrice the same trouble.

The note editor contains a list of deck. This allows to select in
which deck to put the cards of the new notes. It also allows to move
the current card if it already exists (i.e. if we edit a card while
reviewing it). The default value finally allows to knows the deck of
the card (assuming the card exists).

The trouble being that the deck of the card is not necessarily in the
list of decks. In this card, the editor believes that we want to
change the deck of the card; which is not the case.

Thus this pull request simply add the deck of the current card to the
list of deck. Note that instead the current deck is not anymore added
to the list. However, it creates no trouble because:
* either the current deck is the deck of the card, in which case it is
  in the added to the list since it's the deck of the card.
* it is a parent of the deck of the card, hence it's not filtered,
  hence it's already in the list.

So actually this pull request only ensure that one more deck is added
in the case where the card reviewed is in a filtered deck, which is a
children of the selected deck. In any other case it does not change
anything.

I must state that I believe that ankidroid should not behaves as it
behaves now. Indeed, the fact that there is a single filtered deck in
the list means that choosing this deck has a meaning different from
the meaning of choosing any other deck. However, the GUI does not
emphasize the fact that the meaning of the filtered deck is different
from the meaning of the other decks.

However, even if I don't like this behaviour, it is the current
behaviour when you edit a card and the selected deck is filtered. So I
believe that it should remains the behavior when the deck of the card
is filtered, even if it's not the selected deck.

You can test this commit by following the steps of
ankidroid#5184 (comment)
and checking that the problem did disappear. Furthermore, editing any
other card does not create any trouble.
@timrae timrae merged commit c71c2fe into ankidroid:master Oct 16, 2019
@timrae
Copy link
Member

timrae commented Oct 16, 2019

Good job!

@Arthur-Milchior
Copy link
Member Author

Good job!

Thank you

@Arthur-Milchior Arthur-Milchior deleted the ChangeOriginalDeckInsteadOfCurrentDeckInReview branch October 17, 2019 01:03
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.

None yet

2 participants