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:no warning on empty front template #16423

Closed
wants to merge 2 commits into from

Conversation

shrutigitte
Copy link
Contributor

Purpose / Description

Anki and Ankidroid both have same warning dialog when trying to save front card template

Fixes

How Has This Been Tested?

Google Emulator
image

Learning (optional, can help others)

Describe the research stage

Links to blog posts, patterns, libraries or addons used to solve this problem

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Thanks!!!

Could you also add a @NeedsTest annotation for each testable issue raised in this review

/** Checks if the front of the template is empty or not, and show a dialog if empty
* @return true if empty else false **/
fun isFrontTemplateEmpty(): Boolean {
if (currentTemplate?.front.isNullOrEmpty()) {
Copy link
Member

@david-allison david-allison May 19, 2024

Choose a reason for hiding this comment

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

This needs to match Anki Desktop, check their source code and link the relevant lines

The test is for "No Fields", so non-blank content: AAA should also fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anki uses update_notetype_legacy and to show errors anki uses AnkiErrors but I couldn't find something as such so I need help as I am a new contributor .

Copy link
Member

Choose a reason for hiding this comment

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

Best to check in Discord to see if anyone has capacity to bring this to completion

Copy link
Member

Choose a reason for hiding this comment

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

Could you also mark this as a draft if it's not ready for review

private fun showEmptyTemplateDialog() {
AlertDialog.Builder(templateEditor).show {
templateEditor.tempModel?.let { tempModel ->
val templateCount = tempModel.templateCount
Copy link
Member

Choose a reason for hiding this comment

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

If I have two templates, and the problem is on the first template, this should be 1, not 2

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label May 19, 2024
@shrutigitte shrutigitte marked this pull request as draft May 22, 2024 10:07
Copy link
Contributor

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jun 17, 2024
@github-actions github-actions bot closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Conflicts Needs Author Reply Waiting for a reply from the original author Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Empty the Front template and click save, no warning
2 participants