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

Bug: Changing a deck's reminder settings changes even other deck's ones, unexpectedly #4547

Closed
snowtimeglass opened this issue Jan 28, 2017 · 20 comments · Fixed by #5122
Closed
Assignees
Milestone

Comments

@snowtimeglass
Copy link
Contributor

Reproduction Steps (1)
  1. Create a deck and enable its reminder and specify its time.
    image
    image

  2. Create another new deck.
    image

  3. Check the second deck's [Deck Options]>[Reminders]

Expected Result

image

Actual Result

image

Reproduction Steps (2)
  1. Fix the time of the second deck to "02:22"
    image
    image

  2. Check the first deck's [Deck Options]>[Reminders]
    image

Its time has also been changed, unexpectedly.
image

These seem to be because the reminder settings belong to the option group of the deck, at present.
I think the reminder settings should be independent of the option group, that is, they should belong to the deck itself, as "Deck description" of the deck does.

Debug info

Android 4.4
AnkiDroid 2.8

@mikehardy
Copy link
Member

This seems like a pending question vs the patches merged:

Do we want to have deck reminders stored in the options group, or per deck?

a) If we want them in the options group, then this should be closed as not-a-bug with a reference to the documentation (the merged PR ankidroid/ankidroiddocs#29 there specifically says "options group" is where it is stored).

b) If we want them in the deck, then this should stay open and the changes from the merged PR #4450 should be modified to use and persist per-deck instead of per-options-group

I think "a" (per options group) is a little surprising so I'd vote for "b" myself and could look into it if it seemed like the right direction

@timrae
Copy link
Member

timrae commented Feb 23, 2018

If we go for b, wouldn't that be surprising as well, since all the other options presented there are for the options group?

@mikehardy
Copy link
Member

Hmm - I see what you mean. It's a little more mixed than that though:

  • the drop down says "Deck Options", not "Deck Options Group Options"
  • the "Deck description" is actually per-deck, if I understand it correctly

Some thoughts:
1- maybe rename the drop down "Deck options" to just "Options" (or split them completely)
2- maybe split the dialog into an "Options Group" section, then a hard rule, then a deck-specific section? (or have to different dialogs)
3- then (if per-deck reminders is wanted) move the reminders to deck-level storage and options area
4- alternatively, change the messaging from "Enable reminder notifications for this deck" to "Enable reminder notifications for this options group" and in the docs put a hint that people can clone options group and assign them to decks if they want per-deck options

The easy/immediate fix is to change the messaging (4 above) then have an enhancement request for whatever sounds good out of 1-3 for per-deck reminders in place of per options group

@snowtimeglass
Copy link
Contributor Author

snowtimeglass commented Feb 23, 2018

Thank you so much for your support.
I also vote "b".

I think renaming the item of the drop down list from "Deck options" to just "Options" is good. For one thing, it is a more accurate expression; for another, Anki Desktop uses the expression, "Options". Consitency is important.
image

"2" is not good. I feel it is a bit too much strict and troblesome classification. It seems to be enough to add a subtitle such as "current deck only", to the shortened title "Enable reminder notification", as Anki Desktop does on "Description" as follows.
image

Of course, the current title, "Enable reminder notification for this deck", tells users the point enough, but unfortunately it is a bit too long to display itself on portrait mode as follows. Consequently, shortening the current title and adding a subtitle would be good.)
image

@timrae
Copy link
Member

timrae commented Feb 23, 2018

Maybe rename the drop down "Deck options" to just "Options" (or split them completely)

I think we changed it to "deck options" on purpose in order to make it a bit more clear that there are more "options" available elsewhere in the program (the main settings screen).

I'd kind of love to remove all of this notification stuff from AnkiDroid if someone were to do something like this:

https://groups.google.com/forum/#!topic/anki-android/0LTx-avYWmg

@mikehardy mikehardy added this to the v2.9 release milestone Sep 14, 2018
@mikehardy
Copy link
Member

Marking this for v2.9 so it gets swept in with #4944

@timrae timrae added Accepted Maintainers welcome a PR implementing this feature and removed Accepted Maintainers welcome a PR implementing this feature labels Sep 14, 2018
@timrae
Copy link
Member

timrae commented Sep 14, 2018

I thought we just decided not to change that part in 2.9 lol

@mikehardy
Copy link
Member

In the thread above I think what we decided was mostly just to make sure the messaging was super clear. My 2.9 was to make sure I re-read this and whatever text I proposed was in line with the thinking above. Nothing more :-)

@mikehardy mikehardy self-assigned this Sep 14, 2018
@mikehardy
Copy link
Member

I want to nail the exact change for 2.9 - here's what I'm going to PR for consideration:

  • change the context / popup menu label to "Deck / Deck Group Options" so it's clear (based on the comment from Tim above) that deck-related options, but it starts messaging that there are two types
  • change the actual "deck options" menu to have three sections with a "Deck Group Options" first, and "Deck Options" second
  • If it isn't too hard, I'll add a header on the top of each sub-menu with the warnings from Anki Desktop (e.g., "Your changes will affect multiple decks....add new group ..." in red or "Current deck only")

That will align us mostly with Anki Desktop, but with a little less confusion between the two types of edit (where for instance in the screenshot above there is the red affects-group warning at the same time editing description says "current deck only")

mikehardy added a commit to mikehardy/Anki-Android that referenced this issue Nov 19, 2018
Fixes ankidroid#4547 by grouping the preference items, and relabeling the context menu entry
to be unambiguous that there are two categories of option, and grouping which items
belong in which category
@timrae
Copy link
Member

timrae commented Nov 20, 2018

It sounds kind of overly complex from a UX perspective to be honest, I think it needs more discussion

@mikehardy
Copy link
Member

Check the screen shots on the PR - I would actually revert the menu change but just add the grouping and be done. Tiny change but signals the difference...

@mikehardy
Copy link
Member

I might add a dummy preference at top of each group explaining deck group vs single deck but no more. Headers and footers etc were impractical on investugation

@timrae
Copy link
Member

timrae commented Nov 21, 2018

I'm not really a fan of the "disambiguation" approach. What criteria should we use to decide if a given option is tied to a deck or an option group? And consequently why do we tie notifications to the deck? Are there other current or potential options that should be tied to decks?

In general I'm not a fan of mandatory option groups in the first place, it's been one of the biggest sources of confusion for new users. I've been periodically asking @dae to consider a conceptual change there, I'm not sure what his current thoughts are on this though, I guess he's still considering various possibilities.

@mikehardy
Copy link
Member

It's a storage issue and is why we have a second db for whiteboard etc so it can be deck specific. I did not want to go that far here, or graft deck specific stuff into collection storage (though I contemplated it for the reminders pr in general). This pr is just saying we have group things and we have deck things and desktop disambiguates because it's confusing, for now let's follow.

@timrae
Copy link
Member

timrae commented Nov 21, 2018

desktop disambiguates because it's confusing

I don't think desktop has any options that are applied to a deck rather than a group does it? Going back over the thread, it seems that @snowtimeglass also voiced opposition to the way you ended up implementing it

@mikehardy
Copy link
Member

Desktop has deck descriptions that specific. The rest is per group, and both have messaging per the screen shots here. I don't like it either I see deck group reminders as temporary for 2.9 before reminders go deck specific, to avoid the inevitable confusion from deck grouos. Really I just want to button everything down for 2.9. If 2.9 can go without this I'm fine dropping it

@mikehardy
Copy link
Member

To be clear (and the reason this was marked for 2.9 in the first place) is that I don't want to rip deck reminders out, I think they are useful (and use them myself) but implementing deck-specific storage is a big bite right now. There is already the deck-specific description which is called out in desktop vs group-related options, so this is simply trying to unify the messaging but I am still intending to do deck-specific reminders later. I re-read the comments as well and it is my understanding that everyone likes the idea of deck-specific reminders, but @snowtimeglass also liked the idea of having the menu separated to unify the messaging with desktop on whether a thing is deck-specific or not. So this is just half a loaf but it's something, maybe enough for 2.9 to avoid a torrent of issues in the area.

@timrae
Copy link
Member

timrae commented Nov 21, 2018

I think the way it's implemented in desktop is not perfect but it's a reasonable compromise until something better is developed:

  • Call it "options" instead of "deck options"
  • Note that by default everything is applied to all decks in the group apart from some specific exceptions
  • In the case of the specific exceptions, write a clear message that it's applied only to the current deck

@timrae
Copy link
Member

timrae commented Nov 21, 2018

@snowtimeglass

On desktop what are the Japanese translations for "options" vs. the main preferences?

@mikehardy
Copy link
Member

@timrae thanks for the clear direction - I can do those things, definitely. The more I see the menu with my disambiguation change in it, the more I don't like it - so I like the simple "Options"

I'll implement the "note by default" as a non-clickable preference because changing the layout of preferences screens is rather involved (a separate layout file and inflation etc etc) and in the end does ... the same thing.

The only exception is deck description and I'll do similar for that. Half a loaf for everyone :-)

mikehardy added a commit to mikehardy/Anki-Android that referenced this issue Nov 22, 2018
Fixes ankidroid#4547 (unexpected effect of deck reminders option change) by adopting
the Anki Desktop deck group options / deck options messaging style to advise users
that their changes will mostly affect multiple decks if they don't make a new group
mikehardy added a commit to mikehardy/Anki-Android that referenced this issue Nov 24, 2018
Fixes ankidroid#4547 (unexpected effect of deck reminders option change) by adopting
the Anki Desktop deck group options / deck options messaging style to advise users
that their changes will mostly affect multiple decks if they don't make a new group
timrae pushed a commit that referenced this issue Nov 24, 2018
Fixes #4547 (unexpected effect of deck reminders option change) by adopting
the Anki Desktop deck group options / deck options messaging style to advise users
that their changes will mostly affect multiple decks if they don't make a new group
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants