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

Deck options without bridge #3571

Merged
merged 7 commits into from
Jan 8, 2025

Conversation

Arthur-Milchior
Copy link
Contributor

Thanks to @BrayanDSO I learned we were not supposed to have BridgeCommand anymore. So here I remove them in the deck options

@BrayanDSO
Copy link
Contributor

Reference: ankidroid/Anki-Android#14361 (comment)

@dae
Copy link
Member

dae commented Nov 17, 2024

Sorry for the delayed review. A couple of questions:

  • If 'requireClose' and 'confirmDiscardChanges' are intended only to be used by the deck options as the comment implies, I think they need a more specific name.
  • If they're intended to be used in other contexts in the future too, I think closeCurrentWindow might be a clearer name
  • Did you consider using a simple JavaScript confirm() instead?

@Arthur-Milchior
Copy link
Contributor Author

Regarding js's confirm. Do you want it to also be used in Anki?
If yes, I can try.
I would not have done it without your permission, because it'd be a user-facing change that does not seems to solve any actual user issue. More specifically, if I did that, the confirmation request window would be different from the one the user usually see when getting asked questions about the remaining of anki.

By the way, I guess the same problem would occur in ankidroid, with confirm we'd get a browser confirmation view. While the user is used to the Material dialog.

Regarding requireClose and confirmDiscardChanges, right now, it's only used by the deck options. I know anki less than you, and I've no idea whether any similar request may eventually be used.
I'd have thought maybe it'd be useful in the NoteEditor, given that it already show the same discard questions. But I see the "close" button is a PyQT button so I don't expect we'd actually need it.

Anyway, even if we end up with duplication because this feature is used in two views, that does not seems that bad.

@Arthur-Milchior Arthur-Milchior force-pushed the deck_options_without_bridge branch 2 times, most recently from 5d8d3ba to ef0c610 Compare December 7, 2024 04:31
@dae
Copy link
Member

dae commented Dec 9, 2024

We already use confirm() when the user removes a preset in the deck options, so there's precedent. I believe all platforms have the ability to intercept the confirm() request, and present it as they wish. The desktop does this with javaScriptConfirm()

https://stackoverflow.com/questions/2726377/how-to-handle-a-webview-confirm-dialog

As those methods will only be used for the deck options for now, I think adding 'deckOptions' to them might be a bit clearer.

@Arthur-Milchior
Copy link
Contributor Author

Arthur-Milchior commented Dec 28, 2024

And now it uses Confirm. It simplifies the code. Thanks.

I must note that "confirm" seems to only accept ok/cancel. So we don't have the "discard"/"keep editing" button anymore. But, at least in English, it's not ambiguous.

@Arthur-Milchior Arthur-Milchior force-pushed the deck_options_without_bridge branch 2 times, most recently from 7858603 to d1e94e0 Compare January 3, 2025 19:23
qt/aqt/deckoptions.py Outdated Show resolved Hide resolved
qt/aqt/deckoptions.py Outdated Show resolved Hide resolved
qt/aqt/deckoptions.py Outdated Show resolved Hide resolved
@abdnh
Copy link
Collaborator

abdnh commented Jan 4, 2025

Pylint check is failing due to an unused ask_user_dialog import.

@Arthur-Milchior Arthur-Milchior force-pushed the deck_options_without_bridge branch from d1e94e0 to 119d5b8 Compare January 4, 2025 23:22
@dae
Copy link
Member

dae commented Jan 8, 2025

Thanks for seeing this through Arthur! confirm() did make it a bit simpler.

@dae dae merged commit d7fc98d into ankitects:main Jan 8, 2025
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