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

Track Current Deck Context from Browser #3385

Merged
merged 15 commits into from
Sep 22, 2024

Conversation

taylorobyen
Copy link
Contributor

This change will track the current deck as users navigate through decks via the browser.

Previously when adding cards via CTRL+E or Notes > Add Notes... the add menu would open the most recently studied deck. This previous behavior still exists with the addition of tracking browser interactions as well. This makes adding cards less disorienting by more accurately tracking what the user will want to interact with.

@brishtibheja
Copy link
Contributor

This is a fundamentally good change I think. The current behaviour is unituitive and it isn't documented in the manual. I planned to add it but hopefully app behaviour can be made more intuitive here.

I wonder though how does the add menu behave when I select a deck but don't study from it? Is that deck then gets chosen in Add screen? I'm not sure how desktop users use the Stats screen, but do we want something similar for Stats?

@taylorobyen
Copy link
Contributor Author

taylorobyen commented Aug 25, 2024

@brishtibheja Thanks for the feedback.

I wonder though how does the add menu behave when I select a deck but don't study from it?

When you select a deck at the main window it sets it as your current deck, bad phrasing on my part in the OP. It doesn't need to be explicitly studied.

Is that deck then gets chosen in Add screen?

Yes.

I'm not sure how desktop users use the Stats screen, but do we want something similar for Stats?

The Stats window references the same current deck, so this change actually already accounts for that.

@brishtibheja
Copy link
Contributor

When you select a deck at the main window it sets it as your current deck, bad phrasing on my part in the OP.

No worries! I've seen even dae saying those exact same words.

I can't review the code, but the idea itself is great. It's one less app behaviour in need of explanation.

@cfgnunes
Copy link

Hi @taylorobyen!

Thanks for this pull request! I think this feature is extremely important. I've also opened a bug report in https://forums.ankiweb.net/t/bug-report-deck-selection-mismatch-in-anki-browse-screen/48716 related to this behavior.

I'm look forward to the developers accept your pull request.
Best regards,

@dae
Copy link
Member

dae commented Aug 29, 2024

Sorry, I'm still not convinced this is a good idea. It doesn't make sense in multiple-selection mode, and I'm sure it will lead to complaints from other users who happen to be looking up some cards in the middle of a review session.

https://forums.ankiweb.net/t/in-browser-i-cant-select-a-deck-and-change-into-it-in-the-deck-is-it-correct/32197/2

@brishtibheja
Copy link
Contributor

I think many people would want to add cards from the browse screen. This is probably outside the scope of this one PR, but can we have a seperate deck dropdown. It's easier to switch decks in large deck trees and add cards, and do other operations on them, when in browse screen. AnkiDroid also allows this and is surely useful. AnkiDroid also has a seperate deck selector in Stats screen now. Plus, there seems to be a lot of feature requests concerning this so please consider it.

@taylorobyen
Copy link
Contributor Author

taylorobyen commented Aug 29, 2024

It is impossible to select multiple decks at once. This references the sidebar selection, not the notes.

Edit: Found what you were referring to, will address in next revision.

@taylorobyen
Copy link
Contributor Author

I was thinking about your second point last night of how we can accomplish this without updating the user's current deck. I think I may be able to tie into the method that decides which deck to select when creating the Add window. Perhaps it can query if the user has an active Browser window and will check if the user's selection is a deck. If this isn't met then it will default to use the current deck. This will prevent any interruptions if studying and browsing simultaneously.

Would this address your concerns?

@brishtibheja
Copy link
Contributor

This has to be predictable enough too otherwise a lot of people will get confused and think they've encountered some bug.

@taylorobyen
Copy link
Contributor Author

I think having the Add window use the context of the active window is basically as predictable as it can get.

@taylorobyen
Copy link
Contributor Author

taylorobyen commented Aug 31, 2024

In this latest revision, I have updated the open Add window call made from Browser to pass the currently selected deck id and note type id to the Add window after construction.

Based on feedback from @dae this is where we stand with current issues:

  • Don't interfere with the main window when selecting decks from within the browser
  • Set the Add window choosers to something that makes sense when multiple decks are selected

The current handling of multiple selected decks is to just grab the first index which ends up first in alphabetical order. Potential solutions:

  • Use the most recently interacted deck from the Browser, this would work identically how the Browser derives the current_card member when multiple cards are selected.
  • Default to main window current deck as before if multiple decks are selected.

My recommendation is the first option.

Another potential item we need to account for is the Default deck setting under the Editing tab in preferences:
image

I don't fully understand what the Change deck depending on note type setting is supposed to do, so I'm not sure how ensure that the Add window obeys these settings when opened from the Browser. I tried looking this up in the Anki manual, but couldn't find any references to it. If someone could point me in the right direct that would be helpful!

@brishtibheja
Copy link
Contributor

I don't fully understand what the Change deck depending on note type setting is supposed to do

If you last used a note type with a deck A, changing to that note type will also change the deck to deck A. (I don't remember where I read that).

@dae
Copy link
Member

dae commented Sep 10, 2024

I still think this is potentially confusing, as it's not clear that clicking on a sidebar entry will change the way subsequent actions perform. If we must add a shortcut to do this from the sidebar, maybe it could be via a right-click context menu action in the sidebar instead?

@taylorobyen
Copy link
Contributor Author

I think the context menu addition is a step in the right direction. I have modified the code to use the context menu approach and rolled back the add behavior through shortcut and notes menu back to how it was previously.

Would you be in favor of additional context items on the main window as well?
image

@dae
Copy link
Member

dae commented Sep 20, 2024

The current approach looks ok, though I think it would be clearer if any changes not required by the context menu were reverted - e.g. add_card() currently contains extra logic to determine the currently-selected deck, but we only ever call it with a deck id.

I don't object to an 'add notes' option in the deck options screen, but it might be better to hold off on that for now, and see how much demand there is for it.

qt/aqt/browser/sidebar/tree.py Outdated Show resolved Hide resolved
Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

The actions in the deck list screen weren't hooked up. I'll remove them for now, so this PR can be merged in, and I suggest we wait to see if anyone requests them in the future before we attempt to add them again. If we do add them in the future, we'll need to make sure all strings are translatable.

Thanks for contributing!

qt/aqt/deckbrowser.py Outdated Show resolved Hide resolved
qt/aqt/deckbrowser.py Outdated Show resolved Hide resolved
@dae dae merged commit 90661c4 into ankitects:main Sep 22, 2024
1 check passed
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