Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

feat: Add developer option for find and replace #17897

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Add developer option for find and replace #17897

wants to merge 3 commits into from
+879 −14

Conversation

lukstbit
Copy link
Member

@lukstbit lukstbit commented Jan 29, 2025 •
edited
Loading

Purpose / Description

Enables desktop's find and replace menu option(implemented as a dialog). The line count is big but it's mostly tests and code verbosity, the actual feature is small. Some notes:

  • decided against a viewmodel in the new dialog fragment, we should probably make these dialogs as simple as possible
  • added some tests, the tests related to the fragment's ui should probably be on their own but it would require extra work to make the activity viewmodel replaceable(and I didn't see it worth it to modify the production code for easier testing)
  • browser also allows find and replace on a field directly, see documentation on the new dialog fragment

How it looks:

find_replace.mp4


landscape

find_replace_desktop

Fixes

How Has This Been Tested?

Manually tested the new dialog in various scenarios. Added some tests. Ran all tests.

Checklist

  • 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

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@lukstbit lukstbit force-pushed the feat_findAndReplace branch from 377154c to b359ed0 Compare February 10, 2025 14:11
@lukstbit lukstbit force-pushed the feat_findAndReplace branch from b359ed0 to 3e52c07 Compare February 14, 2025 10:33
david-allison
david-allison approved these changes Feb 14, 2025
View reviewed changes
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.

LGTM, since it's a dev option, all are optional

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Feb 16, 2025
@lukstbit
Fix RustCleanup for Collection.findReplace()
781e4aa 
Note: I also changed the parameters names to match upstream as I
think they are more expressive. The matchCase also follows upstream's
default value, I think it's the job of the ui/tests to properly call the method.

See: https://github.com/ankitects/anki/blob/64ca90934bc26ddf7125913abc9dd9de8cb30c2b/pylib/anki/collection.py#L715
@lukstbit
Add Tags.findAndReplace() and Collection.fieldNamesForNoteIds() upstr…
6795273 
…eam methods

These two methods are used for the find and replace browser functionality.
@lukstbit lukstbit force-pushed the feat_findAndReplace branch from 3e52c07 to 71b37b9 Compare February 17, 2025 17:40
@lukstbit
Copy link
Member Author

I implemented all the review requests.

I also changed the UI a bit to align the title and content start:

image

@lukstbit lukstbit added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author Needs Review labels Feb 17, 2025
@david-allison
Copy link
Member

david-allison commented Feb 17, 2025 •
edited
Loading

Visually: still an approve. Comments:

  • I'd use Material Text Fields for the first 2 fields
  • Reduce the left padding on the checkboxes, to align them with the spinner
  • I personally prefer using the "?" in the title, rather than a 'help' neutral button

@lukstbit lukstbit force-pushed the feat_findAndReplace branch from 71b37b9 to d69222d Compare February 17, 2025 19:06
@lukstbit
Copy link
Member Author

I'd use Material Text Fields for the first 2 fields
Reduce the left padding on the checkboxes, to align them with the spinner

Done. Didn't use the above labels as hints, I'm not sure they are a match and the current design it's just good enough.

I personally prefer using the "?" in the title, rather than a 'help' neutral button

I don't think a ? it's appropriate in the title. Also, I value the help button and I think it's going to be useful especially for users that want to try some regex(thanks to the links in the docs).
image

david-allison
david-allison approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not here to naysay, and it's a developer option. Huge improvement!

EDIT: Unit test failure

@lukstbit
feat: Enable developer option for "Find and replace" browser dialog
216dfc9 
Added as a developer option for now.

Provides a dialog that follows the desktop ui to allow the user to
bulk change their notes. Ui follows the desktop code:

- offers all options that desktop offers
- shows feedback when done with the count of notes changed
- the operation is undoable(from DeckPicker)

If enabled, this option will always(no selection/multi select mode) be present
in the menu.
@lukstbit lukstbit force-pushed the feat_findAndReplace branch from d69222d to 216dfc9 Compare February 17, 2025 19:42
@lukstbit
Copy link
Member Author

Fixed the unit tests and also applied the patch!

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@david-allison david-allison david-allison approved these changes

Assignees
No one assigned
Labels
Needs Second Approval Has one approval, one more approval to merge Priority-Low Strings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement] Find & Replace
3 participants
@lukstbit @david-allison @mikehardy