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.

Sign up for GitHub

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

Jump to bottom

WIP : Allow Multiple Selected Note to delete #15914

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

WIP : Allow Multiple Selected Note to delete #15914

wants to merge 4 commits into from
+133 −7

Conversation

neeldoshii
Copy link
Contributor

@neeldoshii neeldoshii commented Mar 16, 2024 •
edited

Fix

Todo

  • 1. Deletion seems to work properly, but if a new note is added it doesn't considers old notes it only works on the selection works on newly added note only. Somehow the getSelectedItems() doesn't return old cards selected value.
  • 2. Enhance the ui (remove checkbox & change background color as discussed in discord).
  • 3. Add snackbar to show "x selected cards are deleted" for feedback to user.
  • 4. Refactor unused code for review.

Video

Working of Multiple selection and allowing to delete

working_video.mp4

Current bug in the code

cf77cce5-0268-41e5-b115-49879ea69d22.mp4

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

@github-actions GitHub Actions
Copy link
Contributor

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

SanjaySargam
SanjaySargam reviewed Mar 16, 2024
View reviewed changes
AnkiDroid/src/main/res/values/02-strings.xml Outdated
@@ -433,4 +433,5 @@ opening the system text to speech settings fails">
<string name="card_browser_unavailable_when_notes_mode">Unavailable in ‘Notes’ mode</string>

<string name="card_template_reposition_template" comment="move a card template to a new position">Reposition</string>
<string name="title_delete" comment="content description (title) of the delete icon in menu">Delete</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete string already exists, so remove this string

@neeldoshii neeldoshii changed the title Feat : Allow Multiple Selected Note to delete #15885 WIP : Allow Multiple Selected Note to delete #15885 Mar 17, 2024
@david-allison
Copy link
Member

Bring this one to #ankidroid-design on Discord for discussion and list the results on the issue.

Specifically: I feel we can get away with highlighting (as long as it meets accessibility guidelines) and we don't want any layout shift when performing a selection

@neeldoshii neeldoshii force-pushed the Multiselect-Note-type-and-delete-#15885 branch from e6dc486 to dc9b832 Compare March 31, 2024 00:08
neeldoshii
neeldoshii commented Mar 31, 2024
View reviewed changes
AnkiDroid/src/main/java/com/ichi2/anki/notetype/ManageNotetypes.kt
@@ -125,9 +127,25 @@ class ManageNotetypes : AnkiActivity() {
return true
}
})
// Menu for deleting multiple notes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer Note:

Is it okay having two menu inflater or lets get it into one?

@neeldoshii
Copy link
Contributor Author

neeldoshii commented Apr 5, 2024 •
edited

Remove Tag : Conflict

Deletion seems to work properly, but if a new note is added it doesn't considers old notes it only works on the selection works on newly added note only. Somehow the getSelectedItems() doesn't return old cards selected value.

@david-allison can you help me where I am going wrong with logic? I am unable to fix this.

@david-allison
Copy link
Member

Could you rephrase? I don't understand the problem

@neeldoshii
Copy link
Contributor Author

neeldoshii commented Apr 5, 2024 •
edited

I have a video in description : working and when bug occurs.

  1. The normal multiple deletion works properly.
  2. The logic for deletion gets failed when you add a new note type. for ex in the bugged video when I added new note type : test1, test2. But the. 3 selected were not deleted only recently added new note was deleted.

@david-allison
Copy link
Member

@neeldoshii

Time to improve your debugging skills and see if you can replicate this screenshot:

Screenshot 2024-04-05 at 20 12 14

The screenshot also shows the issue: notetypesAdapter.currentList only has one isSelected, whereas the UI shows 3.

@neeldoshii neeldoshii changed the title WIP : Allow Multiple Selected Note to delete #15885 WIP : Allow Multiple Selected Note to delete Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanjaySargam SanjaySargam SanjaySargam left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Has Conflicts Strings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiselect Note type and delete
3 participants
@neeldoshii @david-allison @SanjaySargam